[PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[Harald Freudenberger]

The AES ctr implementation produces a warning when used with clang and
CONTEXT_ANALYIS enabled:

arch/s390/crypto/aes_s390.c:585:13: warning: mutex 'ctrblk_lock' is not held on every path through here [-Wthread-safety-analysis]
  585 |                 ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
      |                           ^
arch/s390/crypto/aes_s390.c:577:11: note: mutex acquired here
  577 |         locked = mutex_trylock(&ctrblk_lock);
      |                  ^

Rewrite and reorganize the code such that the clang compiler's needs
are fulfilled with keeping the compatibility, performance and
correctness of the crypto algorithm.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Suggested-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/crypto/aes_s390.c | 60 +++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 976f6f7257d5..e75f41e2bcb7 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -562,46 +562,62 @@ static unsigned int __ctrblk_init(u8 *ctrptr, u8 *iv, unsigned int nbytes)
 	return n;
 }
 
+static int __ctr_aes_crypt(struct s390_aes_ctx *sctx,
+			   struct skcipher_walk *walk, bool locked)
+{
+	unsigned int n, nbytes;
+	int ret = 0;
+	u8 *ctrptr;
+
+	while (!ret && ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE)) {
+		n = AES_BLOCK_SIZE;
+		if (nbytes >= 2 * AES_BLOCK_SIZE && locked)
+			n = __ctrblk_init(ctrblk, walk->iv, nbytes);
+		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk->iv;
+		cpacf_kmctr(sctx->fc, sctx->key, walk->dst.virt.addr,
+			    walk->src.virt.addr, n, ctrptr);
+		if (ctrptr == ctrblk) {
+			memcpy(walk->iv, ctrptr + n - AES_BLOCK_SIZE,
+			       AES_BLOCK_SIZE);
+		}
+		crypto_inc(walk->iv, AES_BLOCK_SIZE);
+		ret = skcipher_walk_done(walk, nbytes - n);
+	}
+
+	return ret;
+}
+
 static int ctr_aes_crypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct s390_aes_ctx *sctx = crypto_skcipher_ctx(tfm);
-	u8 buf[AES_BLOCK_SIZE], *ctrptr;
 	struct skcipher_walk walk;
-	unsigned int n, nbytes;
-	int ret, locked;
+	u8 buf[AES_BLOCK_SIZE];
+	int ret;
 
 	if (unlikely(!sctx->fc))
 		return fallback_skcipher_crypt(sctx, req, 0);
 
-	locked = mutex_trylock(&ctrblk_lock);
-
 	ret = skcipher_walk_virt(&walk, req, false);
-	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
-		n = AES_BLOCK_SIZE;
+	if (ret)
+		return ret;
 
-		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
-			n = __ctrblk_init(ctrblk, walk.iv, nbytes);
-		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
-		cpacf_kmctr(sctx->fc, sctx->key, walk.dst.virt.addr,
-			    walk.src.virt.addr, n, ctrptr);
-		if (ctrptr == ctrblk)
-			memcpy(walk.iv, ctrptr + n - AES_BLOCK_SIZE,
-			       AES_BLOCK_SIZE);
-		crypto_inc(walk.iv, AES_BLOCK_SIZE);
-		ret = skcipher_walk_done(&walk, nbytes - n);
-	}
-	if (locked)
+	if (mutex_trylock(&ctrblk_lock)) {
+		ret = __ctr_aes_crypt(sctx, &walk, true);
 		mutex_unlock(&ctrblk_lock);
+	} else {
+		ret = __ctr_aes_crypt(sctx, &walk, false);
+	}
+
 	/*
 	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
 	 */
-	if (!ret && nbytes) {
+	if (!ret && walk.nbytes) {
 		memset(buf, 0, AES_BLOCK_SIZE);
-		memcpy(buf, walk.src.virt.addr, nbytes);
+		memcpy(buf, walk.src.virt.addr, walk.nbytes);
 		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,
 			    AES_BLOCK_SIZE, walk.iv);
-		memcpy(walk.dst.virt.addr, buf, nbytes);
+		memcpy(walk.dst.virt.addr, buf, walk.nbytes);
 		crypto_inc(walk.iv, AES_BLOCK_SIZE);
 		ret = skcipher_walk_done(&walk, 0);
 		memzero_explicit(buf, sizeof(buf));
-- 
2.43.0

Re: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[sashiko-bot]

> The AES ctr implementation produces a warning when used with clang and
> CONTEXT_ANALYIS enabled:
> 
> arch/s390/crypto/aes_s390.c:585:13: warning: mutex 'ctrblk_lock' is not held on every path through here [-Wthread-safety-analysis]
>   585 |                 ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
>       |                           ^
> arch/s390/crypto/aes_s390.c:577:11: note: mutex acquired here
>   577 |         locked = mutex_trylock(&ctrblk_lock);
>       |                  ^
> 
> Rewrite and reorganize the code such that the clang compiler's needs
> are fulfilled with keeping the compatibility, performance and
> correctness of the crypto algorithm.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807135440.35888-1-freude@linux.ibm.com?part=3

[PATCH v3 0/3] Fixes and rework for aes_s390

[Harald Freudenberger]

Fix and rework some issues around arch/s390/aes_s390.c:

- Fix skcipher_walk return code handling in aes_s390
- Add scrub of some temp buffers
- Rework AES CTR to be prepared for context analysis

For more details please see patch headers.

Changelog:

v1: initial version
v2: Fix missing scrub on a temp buffer used to process left over bytes
    in CTR mode.
v3: - Yet another buffer scrubbing was missing. Added to the 2nd 
      patch and rephrased commit header.
    - Added patch to fix one warning with context analysis regarding
      the handling of a mutex with the AES CTR implementation.
v4: - Sashiko found two unscrubbed buffers in two variables in the GCM
      implementation. So handle them with memzero_explicit().
    - Added one condition to check for the CTR algorithm to invoke
      mutex_trylock() only in non-interrupt and non-atomic
      context (which comes down to process context only).

Harald Freudenberger (3):
  s390/crypto: Fix skcipher_walk return code handling in aes_s390
  s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm
    algorithm
  s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

 arch/s390/crypto/aes_s390.c | 84 ++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 30 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
--
2.43.0

[PATCH v3 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390

[Harald Freudenberger]

The return codes from skcipher_walk_virt() were not properly checked
before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
If skcipher_walk_virt() fails, the walk structure may be in an undefined
state, and attempting to process data could lead to incorrect behavior
or accessing uninitialized memory.

Add proper return code checking to ensure correct handling of the walk
initialization and walk advance and eventually return to the caller
with that return code.

Fixes: 7988fb2c03c8 ("crypto: s390/aes - convert to skcipher API")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 5.5+
---
 arch/s390/crypto/aes_s390.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 62edc66d5478..366ce22d3623 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -129,7 +129,7 @@ static int ecb_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 		return fallback_skcipher_crypt(sctx, req, modifier);
 
 	ret = skcipher_walk_virt(&walk, req, false);
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(sctx->fc | modifier, sctx->key,
@@ -233,7 +233,7 @@ static int cbc_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 		return ret;
 	memcpy(param.iv, walk.iv, AES_BLOCK_SIZE);
 	memcpy(param.key, sctx->key, sctx->key_len);
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_kmc(sctx->fc | modifier, &param,
@@ -359,7 +359,7 @@ static int xts_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 	memcpy(xts_param.key + offset, xts_ctx->key, xts_ctx->key_len);
 	memcpy(xts_param.init, pcc_param.xts, 16);
 
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(xts_ctx->fc | modifier, xts_param.key + offset,
@@ -487,7 +487,7 @@ static int fullxts_aes_crypt(struct skcipher_request *req,  unsigned long modifi
 	memcpy(fxts_param.tweak, req->iv, AES_BLOCK_SIZE);
 	fxts_param.nap[0] = 0x01; /* initial alpha power (1, little-endian) */
 
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(xts_ctx->fc | modifier, fxts_param.key + offset,
@@ -577,7 +577,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 	locked = mutex_trylock(&ctrblk_lock);
 
 	ret = skcipher_walk_virt(&walk, req, false);
-	while ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE) {
+	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
 		n = AES_BLOCK_SIZE;
 
 		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
@@ -596,7 +596,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 	/*
 	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
 	 */
-	if (nbytes) {
+	if (!ret && nbytes) {
 		memset(buf, 0, AES_BLOCK_SIZE);
 		memcpy(buf, walk.src.virt.addr, nbytes);
 		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,
-- 
2.43.0

[PATCH v3 2/3] s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm

[Harald Freudenberger]

In function ctr_aes_crypt() there is a buffer used to process
remaining bytes < AES_BLOCK_SIZE. This buffer was not scrubbed and
thus could lead to expose of unwanted data. When the buffer is used
explicitly scrub it at the end of the code block to avoid exposure of
maybe sensitive data.

In a similar way the function gcm_aes_crypt() hat an error path where
the CPACF param block was not scrubbed. Instead of return early now
these error paths go to end of function where explicit scrubbing is
done. Similar with the buffers which are part of the gcm_sg_walk
structs from the variables gw_in and gw_out.

Fixes: d07f951903fa ("crypto: s390/aes - Fix buffer overread in CTR mode")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.8+
---
 arch/s390/crypto/aes_s390.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 366ce22d3623..10561aa687c7 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -604,6 +604,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 		memcpy(walk.dst.virt.addr, buf, nbytes);
 		crypto_inc(walk.iv, AES_BLOCK_SIZE);
 		ret = skcipher_walk_done(&walk, 0);
+		memzero_explicit(buf, sizeof(buf));
 	}
 
 	return ret;
@@ -895,10 +896,14 @@ static int gcm_aes_crypt(struct aead_request *req, unsigned int flags)
 			  gw_in.ptr, aad_bytes);
 
 		n = aad_bytes + pc_bytes;
-		if (gcm_in_walk_done(&gw_in, n) != n)
-			return -ENOMEM;
-		if (gcm_out_walk_done(&gw_out, n) != n)
-			return -ENOMEM;
+		if (gcm_in_walk_done(&gw_in, n) != n) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		if (gcm_out_walk_done(&gw_out, n) != n) {
+			ret = -ENOMEM;
+			goto out;
+		}
 		aadlen -= aad_bytes;
 		pclen -= pc_bytes;
 	} while (aadlen + pclen > 0);
@@ -910,7 +915,10 @@ static int gcm_aes_crypt(struct aead_request *req, unsigned int flags)
 	} else
 		scatterwalk_map_and_copy(param.t, req->dst, len, taglen, 1);
 
+out:
 	memzero_explicit(&param, sizeof(param));
+	memzero_explicit(gw_in.buf, sizeof(gw_in.buf));
+	memzero_explicit(gw_out.buf, sizeof(gw_out.buf));
 	return ret;
 }
 
-- 
2.43.0

[PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[Harald Freudenberger]

The AES ctr implementation produces a warning when used with clang and
CONTEXT_ANALYIS enabled:

arch/s390/crypto/aes_s390.c:585:13: warning: mutex 'ctrblk_lock' is not held on every path through here [-Wthread-safety-analysis]
  585 |                 ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
      |                           ^
arch/s390/crypto/aes_s390.c:577:11: note: mutex acquired here
  577 |         locked = mutex_trylock(&ctrblk_lock);
      |                  ^

Rewrite and reorganize the code such that the clang compiler's needs
are fulfilled with keeping the compatibility, performance and
correctness of the crypto algorithm.

Also cover a finding from Sashiko about that the mutex_trylock() may
be invoked from a softirq context. So add a check to make sure only in
process context try to lock the mutex.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Suggested-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/crypto/aes_s390.c | 60 +++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 10561aa687c7..8295cb3fc56c 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -562,46 +562,62 @@ static unsigned int __ctrblk_init(u8 *ctrptr, u8 *iv, unsigned int nbytes)
 	return n;
 }
 
+static int __ctr_aes_crypt(struct s390_aes_ctx *sctx,
+			   struct skcipher_walk *walk, bool locked)
+{
+	unsigned int n, nbytes;
+	int ret = 0;
+	u8 *ctrptr;
+
+	while (!ret && ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE)) {
+		n = AES_BLOCK_SIZE;
+		if (nbytes >= 2 * AES_BLOCK_SIZE && locked)
+			n = __ctrblk_init(ctrblk, walk->iv, nbytes);
+		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk->iv;
+		cpacf_kmctr(sctx->fc, sctx->key, walk->dst.virt.addr,
+			    walk->src.virt.addr, n, ctrptr);
+		if (ctrptr == ctrblk) {
+			memcpy(walk->iv, ctrptr + n - AES_BLOCK_SIZE,
+			       AES_BLOCK_SIZE);
+		}
+		crypto_inc(walk->iv, AES_BLOCK_SIZE);
+		ret = skcipher_walk_done(walk, nbytes - n);
+	}
+
+	return ret;
+}
+
 static int ctr_aes_crypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct s390_aes_ctx *sctx = crypto_skcipher_ctx(tfm);
-	u8 buf[AES_BLOCK_SIZE], *ctrptr;
 	struct skcipher_walk walk;
-	unsigned int n, nbytes;
-	int ret, locked;
+	u8 buf[AES_BLOCK_SIZE];
+	int ret;
 
 	if (unlikely(!sctx->fc))
 		return fallback_skcipher_crypt(sctx, req, 0);
 
-	locked = mutex_trylock(&ctrblk_lock);
-
 	ret = skcipher_walk_virt(&walk, req, false);
-	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
-		n = AES_BLOCK_SIZE;
+	if (ret)
+		return ret;
 
-		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
-			n = __ctrblk_init(ctrblk, walk.iv, nbytes);
-		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
-		cpacf_kmctr(sctx->fc, sctx->key, walk.dst.virt.addr,
-			    walk.src.virt.addr, n, ctrptr);
-		if (ctrptr == ctrblk)
-			memcpy(walk.iv, ctrptr + n - AES_BLOCK_SIZE,
-			       AES_BLOCK_SIZE);
-		crypto_inc(walk.iv, AES_BLOCK_SIZE);
-		ret = skcipher_walk_done(&walk, nbytes - n);
-	}
-	if (locked)
+	if (in_task() && mutex_trylock(&ctrblk_lock)) {
+		/* process context and mutex acquired */
+		ret = __ctr_aes_crypt(sctx, &walk, true);
 		mutex_unlock(&ctrblk_lock);
+	} else {
+		ret = __ctr_aes_crypt(sctx, &walk, false);
+	}
 	/*
 	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
 	 */
-	if (!ret && nbytes) {
+	if (!ret && walk.nbytes) {
 		memset(buf, 0, AES_BLOCK_SIZE);
-		memcpy(buf, walk.src.virt.addr, nbytes);
+		memcpy(buf, walk.src.virt.addr, walk.nbytes);
 		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,
 			    AES_BLOCK_SIZE, walk.iv);
-		memcpy(walk.dst.virt.addr, buf, nbytes);
+		memcpy(walk.dst.virt.addr, buf, walk.nbytes);
 		crypto_inc(walk.iv, AES_BLOCK_SIZE);
 		ret = skcipher_walk_done(&walk, 0);
 		memzero_explicit(buf, sizeof(buf));
-- 
2.43.0

Re: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing `ctrblk_lock` in softirq context forces AES-CTR to process data one block (16 bytes) at a time, severely degrading performance for softirq crypto users like IPsec.
--

--- Patch [3]: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis ---
commit 7a2551a9e3023dcb2323f560164fee63c00a3278
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

This commit rewrites the AES CTR implementation to resolve a clang
thread-safety warning regarding the ctrblk_lock mutex. It splits the
cryptographic operations into __ctr_aes_crypt() and checks in_task()
before attempting mutex_trylock() to avoid locking in softirq context.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 10561aa687c78..8295cb3fc56c2 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -562,46 +562,62 @@ static unsigned int __ctrblk_init(u8 *ctrptr, u8 *iv, unsigned int nbytes)
[ ... ]
> +static int __ctr_aes_crypt(struct s390_aes_ctx *sctx,
> +			   struct skcipher_walk *walk, bool locked)
> +{
> +	unsigned int n, nbytes;
> +	int ret = 0;
> +	u8 *ctrptr;
> +
> +	while (!ret && ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE)) {
> +		n = AES_BLOCK_SIZE;
> +		if (nbytes >= 2 * AES_BLOCK_SIZE && locked)
> +			n = __ctrblk_init(ctrblk, walk->iv, nbytes);
> +		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk->iv;

[Severity: High]
Since locked is false for softirq contexts, does this code bypass
__ctrblk_init() and clamp n to AES_BLOCK_SIZE (16 bytes)?

This causes the loop to only process 16 bytes per iteration instead of
handling the payload in bulk.

[ ... ]
>  static int ctr_aes_crypt(struct skcipher_request *req)
>  {
[ ... ]
>  	ret = skcipher_walk_virt(&walk, req, false);
> -	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
> -		n = AES_BLOCK_SIZE;
> +	if (ret)
> +		return ret;
>  
> -		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
> -			n = __ctrblk_init(ctrblk, walk.iv, nbytes);
> -		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
> -		cpacf_kmctr(sctx->fc, sctx->key, walk.dst.virt.addr,
> -			    walk.src.virt.addr, n, ctrptr);
> -		if (ctrptr == ctrblk)
> -			memcpy(walk.iv, ctrptr + n - AES_BLOCK_SIZE,
> -			       AES_BLOCK_SIZE);
> -		crypto_inc(walk.iv, AES_BLOCK_SIZE);
> -		ret = skcipher_walk_done(&walk, nbytes - n);
> -	}
> -	if (locked)
> +	if (in_task() && mutex_trylock(&ctrblk_lock)) {

[Severity: High]
Does this in_task() check unconditionally prevent softirq contexts from
acquiring the mutex?

Network crypto frameworks (e.g. IPsec) typically execute AES-CTR 
operations within softirq contexts (NET_RX_SOFTIRQ, NET_TX_SOFTIRQ), where 
in_task() evaluates to false. This forces all such softirq users into the
fallback path calling __ctr_aes_crypt(sctx, &walk, false).

By forcing softirq contexts to process data 16 bytes at a time, does this
introduce a massive performance regression for network crypto workloads?

> +		/* process context and mutex acquired */
> +		ret = __ctr_aes_crypt(sctx, &walk, true);
>  		mutex_unlock(&ctrblk_lock);
> +	} else {
> +		ret = __ctr_aes_crypt(sctx, &walk, false);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813155406.50051-1-freude@linux.ibm.com?part=3

Re: [PATCH v3 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `mutex_trylock()` from softirq/interrupt context is unsafe and violates core kernel locking rules.
- [High] Stack buffer `buf` containing sensitive plaintext/ciphertext is not securely erased before returning from `ctr_aes_crypt`.
--

--- Patch [1]: [PATCH v3 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390 ---
commit 26c79c6f9fbbbd190ba8f14ce8cd5587ce6bbe31
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix skcipher_walk return code handling in aes_s390

The return codes from skcipher_walk_virt() were not properly checked
before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
If skcipher_walk_virt() fails, the walk structure may be in an undefined
state, and attempting to process data could lead to incorrect behavior
or accessing uninitialized memory.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 62edc66d54788..366ce22d36237 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c

[ ... ]

> @@ -577,7 +577,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  	locked = mutex_trylock(&ctrblk_lock);

[Severity: High]
This is a pre-existing issue, but calling mutex_trylock() from softirq or
interrupt context is unsafe and violates core kernel locking rules.

Because this crypto algorithm can be executed in softirq context (such as
when processing IPsec/ESP packets), the current task is arbitrary, which
will result in a lockdep splat.

Could this be fixed by checking if the execution context permits operating
on a sleepable lock, perhaps using in_task(), before attempting the trylock?

>  
>  	ret = skcipher_walk_virt(&walk, req, false);

[ ... ]

> @@ -596,7 +596,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  	/*
>  	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
>  	 */
> -	if (nbytes) {
> +	if (!ret && nbytes) {
>  		memset(buf, 0, AES_BLOCK_SIZE);
>  		memcpy(buf, walk.src.virt.addr, nbytes);
>  		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,

[Severity: High]
This is a pre-existing issue, but this function appears to return without
securely erasing the stack buffer buf.

When a partial block is processed, up to 15 bytes of sensitive cryptographic
material (plaintext or decrypted ciphertext) is left in this buffer. Could
this stack remnant be leaked to userspace by subsequent syscalls?

Other functions in this driver (e.g., cbc_aes_crypt) use memzero_explicit()
before returning. Should a similar cleanup be added here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813155406.50051-1-freude@linux.ibm.com?part=1

Re: [PATCH v3 2/3] s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing explicit scrubbing of stack buffer `buf` containing sensitive cryptographic data in `ctr_paes_do_crypt()`.
--

--- Patch [2]: [PATCH v3 2/3] s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm ---
commit 51e29c55dd2cdbff3549a70ef9403795fa389aa9
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm

This commit fixes missing scrubs of temporary stack buffers in the s390
AES CTR and GCM implementations. It adds memzero_explicit() calls to ensure
sensitive cryptographic data is not left on the stack after partial block
processing or error conditions.

Cc: stable@vger.kernel.org # 6.8+

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 366ce22d36237..10561aa687c78 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -604,6 +604,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  		memcpy(walk.dst.virt.addr, buf, nbytes);
>  		crypto_inc(walk.iv, AES_BLOCK_SIZE);
>  		ret = skcipher_walk_done(&walk, 0);
> +		memzero_explicit(buf, sizeof(buf));

[Severity: High]
This isn't a bug introduced by this patch, but should the same fix be applied
to ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c?

It appears that the protected key implementation handles the final block for
CTR mode using an identical approach, but the stack buffer is left unscrubbed:

arch/s390/crypto/paes_s390.c:ctr_paes_do_crypt() {
        ...
        /* final block may be < AES_BLOCK_SIZE, copy only nbytes */
        if (nbytes) {
                memset(buf, 0, AES_BLOCK_SIZE);
                memcpy(buf, walk->src.virt.addr, nbytes);
                while (1) {
                        if (cpacf_kmctr(...) == AES_BLOCK_SIZE)
                                break;
                        ...
                }
                memcpy(walk->dst.virt.addr, buf, nbytes);
                crypto_inc(walk->iv, AES_BLOCK_SIZE);
                rc = skcipher_walk_done(walk, 0);
        }
out:
        ...
        return rc;
}

Could this lead to similar exposure of decrypted plaintext or encrypted
ciphertext on the kernel stack?

>  	}
>  
>  	return ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813155406.50051-1-freude@linux.ibm.com?part=2

Re: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

[Heiko Carstens]

On Thu, Aug 13, 2026 at 05:54:06PM +0200, Harald Freudenberger wrote:
> Also cover a finding from Sashiko about that the mutex_trylock() may
> be invoked from a softirq context. So add a check to make sure only in
> process context try to lock the mutex.

...

> +	if (in_task() && mutex_trylock(&ctrblk_lock)) {
> +		/* process context and mutex acquired */
> +		ret = __ctr_aes_crypt(sctx, &walk, true);
>  		mutex_unlock(&ctrblk_lock);

Something similar came also up when converting the dasd code: better
convert to semaphore and simply use down_trylock() and up() instead.

Please make the conversion from mutex to semaphore a separate patch,
so it can be easily backported if required.