s390/crypto: Enable CONTEXT_ANALYSIS

Pre-existing issues:
- [High] Silent data corruption when skcipher_walk_done() mapping errors are overwritten by paes_convert_key() false success. --

--- Patch [2]: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking --- commit fd135aacfa5dd1086ab5711e4513f6e0edede1dc
Author: Heiko Carstens <[email protected]>

s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking

This commit extracts the core loop of ctr_paes_do_crypt() into a new helper function, __ctr_paes_do_crypt(), to avoid conditionally taken locks that trip up Clang's static analysis.

Quoted 4 lines

arch/s390/crypto/paes_s390.c +0 -0

index 8cfe6166c193c..d4a9108d39f5f 100644

[ ... ]

Quoted 16 lines

arch/s390/crypto/paes_s390.c +2 -3

@@ -981,34 +955,76 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 		k = cpacf_kmctr(ctx->fc, param, walk->dst.virt.addr,
 				walk->src.virt.addr, n, ctrptr);
 		if (k) {
-			if (ctrptr == ctrblk)
+			if (ctrptr == ctrblk) {
 				memcpy(walk->iv, ctrptr + k - AES_BLOCK_SIZE,
 				       AES_BLOCK_SIZE);
+			}
 			crypto_inc(walk->iv, AES_BLOCK_SIZE);
 			rc = skcipher_walk_done(walk, nbytes - k);
 		}
 		if (k < n) {
 			if (!maysleep) {
-				if (locked)
-					mutex_unlock(&ctrblk_lock);

arch/s390/crypto/paes_s390.c static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx, · line 970

Quoted 4 lines

arch/s390/crypto/paes_s390.c +0 -0

 				rc = -EKEYEXPIRED;
 				goto out;
 			}
 			rc = paes_convert_key(ctx, tested);

[Severity: High]
This is a pre-existing issue, but does this sequence overwrite an error from skcipher_walk_done()?

Harald, Holger,

Quoted 39 lines

Pre-existing issues:
- [High] Silent data corruption when skcipher_walk_done() mapping errors are overwritten by paes_convert_key() false success. --

--- Patch [2]: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking ---
commit fd135aacfa5dd1086ab5711e4513f6e0edede1dc
Author: Heiko Carstens <[email protected]>

s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking

This commit extracts the core loop of ctr_paes_do_crypt() into a new helper function, __ctr_paes_do_crypt(), to avoid conditionally taken locks that trip up Clang's static analysis.

Quoted 4 lines

arch/s390/crypto/paes_s390.c +0 -0

index 8cfe6166c193c..d4a9108d39f5f 100644

[ ... ]

Quoted 16 lines

arch/s390/crypto/paes_s390.c +2 -3

@@ -981,34 +955,76 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 		k = cpacf_kmctr(ctx->fc, param, walk->dst.virt.addr,
 				walk->src.virt.addr, n, ctrptr);
 		if (k) {
-			if (ctrptr == ctrblk)
+			if (ctrptr == ctrblk) {
 				memcpy(walk->iv, ctrptr + k - AES_BLOCK_SIZE,
 				       AES_BLOCK_SIZE);
+			}
 			crypto_inc(walk->iv, AES_BLOCK_SIZE);
 			rc = skcipher_walk_done(walk, nbytes - k);
 		}
 		if (k < n) {
 			if (!maysleep) {
-				if (locked)
-					mutex_unlock(&ctrblk_lock);

Quoted 8 lines

arch/s390/crypto/paes_s390.c static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx, · line 970

Quoted 4 lines

arch/s390/crypto/paes_s390.c +0 -0

 				rc = -EKEYEXPIRED;
 				goto out;
 			}
 			rc = paes_convert_key(ctx, tested);

[Severity: High]
This is a pre-existing issue, but does this sequence overwrite an error from skcipher_walk_done()?

What should happen with this? Can this be ignored? Or should this be fixed like

  			rc = skcipher_walk_done(walk, nbytes - k);
			if (rc) {
				if (locked)
					mutex_unlock(&ctrblk_lock);
 				goto out;
			}

before the conversion?

Even though I said I don't want to fix pre-existing issues, I broke a similar code path with the first patch, so...

Enable CONTEXT_ANALYSYS for s390's crypto code.

Static code checking for acquiring and releasing locks used to be done with sparse. That was removed with [1] and replaced with a clang based approach [2]. The new approach requires that each subsystem needs to be explicitly enabled for checking.

Do that for s390's crypto code. In order to avoid false positives the code has to be slightly reworked, since conditionally acquiring and releasing locks does not work with the checker (besides that this is sub optimal coding style).

[1] 5b63d0ae94cc ("compiler-context-analysis: Remove Sparse support")
[2] 3269701cb256 ("compiler-context-analysis: Add infrastructure for Context Analysis with Clang")

Note:

- "Pre-existing issues" reported by AI will not be addressed in the context of this series

Heiko Carstens (3):
s390/crypto: Rework ctr_aes_crypt() to remove conditional locking s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking s390/crypto: Enable CONTEXT_ANALYSIS

 arch/s390/crypto/Makefile    |   2 +
 arch/s390/crypto/aes_s390.c  |  54 +++++++++++-------
 arch/s390/crypto/paes_s390.c | 104 ++++++++++++++++++++---------------
 3 files changed, 97 insertions(+), 63 deletions(-)

Quoted 26 lines · Heiko Carstens

Enable CONTEXT_ANALYSYS for s390's crypto code.

Static code checking for acquiring and releasing locks used to be done with sparse. That was removed with [1] and replaced with a clang based approach [2]. The new approach requires that each subsystem needs to be explicitly enabled for checking.

Do that for s390's crypto code. In order to avoid false positives the code has to be slightly reworked, since conditionally acquiring and releasing locks does not work with the checker (besides that this is sub optimal coding style).

[1] 5b63d0ae94cc ("compiler-context-analysis: Remove Sparse support")
[2] 3269701cb256 ("compiler-context-analysis: Add infrastructure for Context Analysis with Clang")

Note:

- "Pre-existing issues" reported by AI will not be addressed in the context of this series

Heiko Carstens (3):
s390/crypto: Rework ctr_aes_crypt() to remove conditional locking s390/crypto: Rework ctr_paes_do_crypt() to remove conditional
locking
s390/crypto: Enable CONTEXT_ANALYSIS

Quoted 5 lines

 arch/s390/crypto/Makefile    |   2 +
 arch/s390/crypto/aes_s390.c  |  54 +++++++++++-------
 arch/s390/crypto/paes_s390.c | 104 
++++++++++++++++++++---------------
 3 files changed, 97 insertions(+), 63 deletions(-)

I presume this is going through the s390 tree?

Cheers,

Enable CONTEXT_ANALYSYS for s390's crypto code.

Static code checking for acquiring and releasing locks used to be done with sparse. That was removed with [1] and replaced with a clang based approach [2]. The new approach requires that each subsystem needs to be explicitly enabled for checking.

Do that for s390's crypto code. In order to avoid false positives the code has to be slightly reworked, since conditionally acquiring and releasing locks does not work with the checker (besides that this is sub optimal coding style).

[1] 5b63d0ae94cc ("compiler-context-analysis: Remove Sparse support")
[2] 3269701cb256 ("compiler-context-analysis: Add infrastructure for Context Analysis with Clang")

Note:

- "Pre-existing issues" reported by AI will not be addressed in the context of this series

Heiko Carstens (3):
s390/crypto: Rework ctr_aes_crypt() to remove conditional locking s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking s390/crypto: Enable CONTEXT_ANALYSIS

 arch/s390/crypto/Makefile    |   2 +
 arch/s390/crypto/aes_s390.c  |  54 +++++++++++-------
 arch/s390/crypto/paes_s390.c | 104 ++++++++++++++++++++---------------
 3 files changed, 97 insertions(+), 63 deletions(-)

Quoted 1 line · Heiko Carstens

Enable CONTEXT_ANALYSYS for s390's crypto code.

Quoted 9 lines

Heiko Carstens (3):
s390/crypto: Rework ctr_aes_crypt() to remove conditional locking s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking s390/crypto: Enable CONTEXT_ANALYSIS

 arch/s390/crypto/Makefile    |   2 +
 arch/s390/crypto/aes_s390.c  |  54 +++++++++++-------
 arch/s390/crypto/paes_s390.c | 104 ++++++++++++++++++++---------------
 3 files changed, 97 insertions(+), 63 deletions(-)

Applied, thank you!