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

[PATCH v4 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
- Shift from using a mutex to using a semaphore in AES CTR

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 yet another buffer which was not scrubbed.
    - New patch #3 which shifts from using a mutex to using a
      semaphore in AES CTR as this algorithm may be invoked in
      atomic/interrupt context.
    - With the shift to a semaphore now clang is able to do the
      context analysis. So the patch from v3 which reworked the CTR
      implementation is not needed any more. Note that paes still has
      the issue with clang context analysis, but this will be
      addressed in another patch series.

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: Fix use of mutex in atomic context

 arch/s390/crypto/aes_s390.c | 39 +++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
--
2.43.0

[PATCH v4 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Using a mutex (`ctrblk_lock`) in `ctr_aes_crypt`, which can be executed in interrupt/softirq context.
--

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

This patch adds proper return code checking from skcipher_walk_virt() in
the processing loops of several symmetric cipher modes to prevent operating
on an uninitialized walk structure if the initialization fails.

> 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 isn't a bug introduced by this patch, but can using a mutex here
corrupt the interrupted task's lock state?

Since ctr_aes_crypt() can be executed in softirq context (such as during
IPsec ESP receive processing via NET_RX_SOFTIRQ), calling mutex_trylock()
and later mutex_unlock() assigns ownership to the arbitrarily interrupted
task. Mutexes enforce strict task ownership and are explicitly forbidden
in interrupt context, even when using trylock.

Could this trigger lockdep splats, incorrect optimistic spinning behavior,
or hard panics on PREEMPT_RT kernels where mutexes are strictly task-bound?

>  
>  	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)

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

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

[Holger Dengler]

On 8/14/26 14:37, Harald Freudenberger wrote:
> 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+

Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler

[PATCH v4 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

Re: [PATCH v4 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:
- [Medium] Missing scrub of sensitive temporary buffer in `ctr_paes_do_crypt()` when handling remaining bytes < AES_BLOCK_SIZE.
--

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

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

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.

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

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

[ ... ]

> @@ -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,
> @@ -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;

[Severity: Medium]
This is a pre-existing issue, but while you are patching ctr_aes_crypt(),
could the same missing scrub vulnerability be present in the PAES equivalent?

Looking at ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c, when the
remaining bytes are less than AES_BLOCK_SIZE, it uses a local buf array in an
almost identical manner but never explicitly scrubs it before returning:

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(ctx->fc, param, buf,
					buf, AES_BLOCK_SIZE,
					walk->iv) == 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:
	pr_debug("rc=%d\n", rc);
	return rc;
}

Can this leak plaintext or ciphertext to the kernel stack where it might be
subsequently exposed to user space via uninitialized stack padding?

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

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

[Holger Dengler]

On 8/14/26 14:37, Harald Freudenberger wrote:
> 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+

Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler

[PATCH v4 3/3] s390/crypto: Fix use of mutex in atomic context

[Harald Freudenberger]

The AES CTR implementation used a mutex to lock one page of exclusive
memory for fast CTR processing. Unfortunately a mutex is not save to
use in atomic or interrupt context. So use a binary semaphore instead
which is save to use in such environments.

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

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 10561aa687c7..bd9815988c81 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -26,14 +26,14 @@
 #include <linux/module.h>
 #include <linux/cpufeature.h>
 #include <linux/init.h>
-#include <linux/mutex.h>
 #include <linux/fips.h>
+#include <linux/semaphore.h>
 #include <linux/string.h>
 #include <crypto/xts.h>
 #include <asm/cpacf.h>
 
 static u8 *ctrblk;
-static DEFINE_MUTEX(ctrblk_lock);
+static DEFINE_SEMAPHORE(ctrblk_sem, 1);
 
 static cpacf_mask_t km_functions, kmc_functions, kmctr_functions,
 		    kma_functions;
@@ -569,12 +569,13 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 	u8 buf[AES_BLOCK_SIZE], *ctrptr;
 	struct skcipher_walk walk;
 	unsigned int n, nbytes;
-	int ret, locked;
+	bool locked;
+	int ret;
 
 	if (unlikely(!sctx->fc))
 		return fallback_skcipher_crypt(sctx, req, 0);
 
-	locked = mutex_trylock(&ctrblk_lock);
+	locked = down_trylock(&ctrblk_sem) == 0;
 
 	ret = skcipher_walk_virt(&walk, req, false);
 	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
@@ -592,7 +593,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 		ret = skcipher_walk_done(&walk, nbytes - n);
 	}
 	if (locked)
-		mutex_unlock(&ctrblk_lock);
+		up(&ctrblk_sem);
 	/*
 	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
 	 */
-- 
2.43.0

Re: [PATCH v4 3/3] s390/crypto: Fix use of mutex in atomic context

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch completely misses the exact same preexisting invalid mutex usage in atomic context in the Protected AES (PAES) CTR implementation (`paes_s390.c`).
--

--- Patch [3]: [PATCH v4 3/3] s390/crypto: Fix use of mutex in atomic context ---
commit 709721b3784d3a0026753ef3b28100620270373d
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix use of mutex in atomic context

This commit replaces a mutex with a binary semaphore in the s390 AES CTR
implementation. This prevents unsafe locking when the crypto routines are
called from atomic or interrupt contexts.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 10561aa687c78..bd9815988c813 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
[ ... ]
> @@ -569,12 +569,13 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  	u8 buf[AES_BLOCK_SIZE], *ctrptr;
>  	struct skcipher_walk walk;
>  	unsigned int n, nbytes;
> -	int ret, locked;
> +	bool locked;
> +	int ret;
>  
>  	if (unlikely(!sctx->fc))
>  		return fallback_skcipher_crypt(sctx, req, 0);
>  
> -	locked = mutex_trylock(&ctrblk_lock);
> +	locked = down_trylock(&ctrblk_sem) == 0;

[Severity: High]
This is a pre-existing issue, but it appears the exact same pattern exists in
the Protected AES (PAES) CTR implementation which wasn't updated here.

Looking at ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c:

	locked = mutex_trylock(&ctrblk_lock);

Since the skcipher API can be invoked from atomic or interrupt contexts (like
network softirqs), does the PAES implementation also risk an atomic context
locking violation? Should arch/s390/crypto/paes_s390.c receive a similar
conversion to a binary semaphore?

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

Re: [PATCH v4 3/3] s390/crypto: Fix use of mutex in atomic context

[Holger Dengler]

On 8/14/26 14:37, Harald Freudenberger wrote:
> The AES CTR implementation used a mutex to lock one page of exclusive
> memory for fast CTR processing. Unfortunately a mutex is not save to
> use in atomic or interrupt context. So use a binary semaphore instead
> which is save to use in such environments.
> 
> Fixes: 7988fb2c03c8 ("crypto: s390/aes - convert to skcipher API")
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Cc: stable@vger.kernel.org # 5.5+

Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler