[PATCH v1 0/3] Fixes and rework for paes_s390

[PATCH v1 0/3] Fixes and rework for paes_s390

[Harald Freudenberger]

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

- Fix skcipher_walk return code handling in paes_s390
- Add scrub of some temp buffers
- Shift from using a mutex to using a semaphore in PAES CTR
  Surprisingly clang code analysis is able to deal with semaphores and
  thus the shift also fixes the issue with CONTEXT_ANALYSIS enabled.

For more details please see patch headers.

Changelog:

v1: initial version - however, all these patches are follow up patches
    from a similar patch queue for aes_s390.c

Harald Freudenberger (3):
  s390/crypto: Fix return code handling at skcipher_walk_done in PAES
    algorithms
  s390/crypto: Fix missing scrub of temp buffers with PAES algorithm
  s390/crypto: Fix use of mutex in atomic context in PAES

 arch/s390/crypto/paes_s390.c | 56 ++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 19 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
-- 
2.43.0

[PATCH v1 1/3] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

[Harald Freudenberger]

All the 4 PAES cipher processing loops were not checking the return
value of skcipher_walk_done() immediately after calling it. This could
lead to error masking when both the walk operation failed and a
subsequent key conversion was needed (k < n condition).

Add immediate error checks after skcipher_walk_done() in all main
processing loops (ECB, CBC, CTR, XTS modes) to ensure walk errors are
properly propagated and not masked by subsequent operations.

With that comes a slight rework around the skcipher_walk_done()
invocation. It is now necessary to check if the walk has already been
finalized (walk->nbytes is then 0) or not to avoid double
de-allocation of resources held by the walk.

Fixes: 6cd87cb5ef6c ("s390/crypto: Rework protected key AES for true asynch support")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.16+
---
 arch/s390/crypto/paes_s390.c | 38 +++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 8cfe6166c193..ccc7da106c3a 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -432,8 +432,11 @@ static int ecb_paes_do_crypt(struct s390_paes_ctx *ctx,
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		k = cpacf_km(ctx->fc | req_ctx->modifier, param,
 			     walk->dst.virt.addr, walk->src.virt.addr, n);
-		if (k)
+		if (k) {
 			rc = skcipher_walk_done(walk, nbytes - k);
+			if (rc)
+				goto out;
+		}
 		if (k < n) {
 			if (!maysleep) {
 				rc = -EKEYEXPIRED;
@@ -495,7 +498,7 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
@@ -558,7 +561,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
 		cond_resched();
 		pr_debug("rescheduling request\n");
 		return -ENOSPC;
-	} else if (rc) {
+	} else if (rc && walk->nbytes) {
 		skcipher_walk_done(walk, rc);
 	}
 
@@ -699,6 +702,8 @@ static int cbc_paes_do_crypt(struct s390_paes_ctx *ctx,
 		if (k) {
 			memcpy(walk->iv, param->iv, AES_BLOCK_SIZE);
 			rc = skcipher_walk_done(walk, nbytes - k);
+			if (rc)
+				goto out;
 		}
 		if (k < n) {
 			if (!maysleep) {
@@ -761,7 +766,7 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
@@ -824,7 +829,7 @@ static int cbc_paes_do_one_request(struct crypto_engine *engine, void *areq)
 		cond_resched();
 		pr_debug("rescheduling request\n");
 		return -ENOSPC;
-	} else if (rc) {
+	} else if (rc && walk->nbytes) {
 		skcipher_walk_done(walk, rc);
 	}
 
@@ -986,6 +991,11 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 				       AES_BLOCK_SIZE);
 			crypto_inc(walk->iv, AES_BLOCK_SIZE);
 			rc = skcipher_walk_done(walk, nbytes - k);
+			if (rc) {
+				if (locked)
+					mutex_unlock(&ctrblk_lock);
+				goto out;
+			}
 		}
 		if (k < n) {
 			if (!maysleep) {
@@ -1079,7 +1089,7 @@ static int ctr_paes_crypt(struct skcipher_request *req)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
@@ -1132,7 +1142,7 @@ static int ctr_paes_do_one_request(struct crypto_engine *engine, void *areq)
 		cond_resched();
 		pr_debug("rescheduling request\n");
 		return -ENOSPC;
-	} else if (rc) {
+	} else if (rc && walk->nbytes) {
 		skcipher_walk_done(walk, rc);
 	}
 
@@ -1310,8 +1320,11 @@ static int xts_paes_do_crypt_fullkey(struct s390_pxts_ctx *ctx,
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		k = cpacf_km(ctx->fc | req_ctx->modifier, param->key + offset,
 			     walk->dst.virt.addr, walk->src.virt.addr, n);
-		if (k)
+		if (k) {
 			rc = skcipher_walk_done(walk, nbytes - k);
+			if (rc)
+				goto out;
+		}
 		if (k < n) {
 			if (!maysleep) {
 				rc = -EKEYEXPIRED;
@@ -1404,8 +1417,11 @@ static int xts_paes_do_crypt_2keys(struct s390_pxts_ctx *ctx,
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		k = cpacf_km(ctx->fc | req_ctx->modifier, param->key + offset,
 			     walk->dst.virt.addr, walk->src.virt.addr, n);
-		if (k)
+		if (k) {
 			rc = skcipher_walk_done(walk, nbytes - k);
+			if (rc)
+				goto out;
+		}
 		if (k < n) {
 			if (!maysleep) {
 				rc = -EKEYEXPIRED;
@@ -1512,7 +1528,7 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
@@ -1575,7 +1591,7 @@ static int xts_paes_do_one_request(struct crypto_engine *engine, void *areq)
 		cond_resched();
 		pr_debug("rescheduling request\n");
 		return -ENOSPC;
-	} else if (rc) {
+	} else if (rc && walk->nbytes) {
 		skcipher_walk_done(walk, rc);
 	}
 
-- 
2.43.0

Re: [PATCH v1 1/3] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

[sashiko-bot]

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

Pre-existing issues:
- [High] The crypto engine async callback functions (e.g., ecb_paes_do_one_request, cbc_paes_do_one_request) double-complete requests upon failure.
--

--- Patch [1]: [PATCH v1 1/3] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit 9fb1966e4b16413a77ebe1ccd447f231c3b30ec4
Author: Harald Freudenberger <freude@linux.ibm.com>
Subject: s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

This commit adds immediate error checks after skcipher_walk_done in the
main processing loops for PAES algorithms. It ensures walk errors are
propagated properly instead of being masked, and checks walk->nbytes to
avoid double de-allocation.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
[ ... ]
> @@ -558,7 +561,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
>  		cond_resched();
>  		pr_debug("rescheduling request\n");
>  		return -ENOSPC;
> -	} else if (rc) {
> +	} else if (rc && walk->nbytes) {
>  		skcipher_walk_done(walk, rc);
>  	}
>  
>  	memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
>  	pr_debug("request complete with rc=%d\n", rc);
>  	local_bh_disable();
>  	atomic_dec(&ctx->via_engine_ctr);
>  	crypto_finalize_skcipher_request(engine, req, rc);
>  	local_bh_enable();
>  	return rc;

[Severity: High]
This is a pre-existing issue, but can returning a negative rc here cause a
double-completion of the request?

If an error occurs during cryptographic processing (for example, if
paes_convert_key() fails and returns a negative rc), the code calls
crypto_finalize_skcipher_request() which completes the request once.

It then returns that same negative rc to the caller. Does the crypto engine
core (in crypto_pump_requests) see this negative return value and execute its
own error path, completing the exact same request a second time?

Could this result in a use-after-free or double-free, since callers might
drop references multiple times when a request is completed twice? This
identical pattern appears to exist across all the PAES cipher modes
(ecb_paes_do_one_request, cbc_paes_do_one_request, ctr_paes_do_one_request,
and xts_paes_do_one_request).

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

[PATCH v1 2/3] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

[Harald Freudenberger]

In function ctr_paes_do_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 function __xts_2keys_prep_param() change the existing scrub to
clean the whole param block instead of just the key field.

Fixes: 6cd87cb5ef6c ("s390/crypto: Rework protected key AES for true asynch support")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.16+
---
 arch/s390/crypto/paes_s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index ccc7da106c3a..5a7dc4651885 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -1041,6 +1041,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 		memcpy(walk->dst.virt.addr, buf, nbytes);
 		crypto_inc(walk->iv, AES_BLOCK_SIZE);
 		rc = skcipher_walk_done(walk, 0);
+		memzero_explicit(buf, sizeof(buf));
 	}
 
 out:
@@ -1377,7 +1378,7 @@ static inline int __xts_2keys_prep_param(struct s390_pxts_ctx *ctx,
 		memcpy(param->init, pcc_param.xts, 16);
 	}
 
-	memzero_explicit(pcc_param.key, sizeof(pcc_param.key));
+	memzero_explicit(&pcc_param, sizeof(pcc_param));
 	return rc;
 }
 
-- 
2.43.0

Re: [PATCH v1 2/3] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch adds a stack buffer scrub for `buf` in `ctr_paes_do_crypt()`, but misses the error paths, leaving sensitive cryptographic data exposed on the kernel stack.
- [High] Identical temporary stack buffer vulnerabilities exist in unprotected `aes_s390.c` functions (`ctr_aes_crypt` and `gcm_aes_crypt`), including raw AES key leaks on error paths.
--

--- Patch [2]: [PATCH v1 2/3] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm ---
commit d69919771b30f447650b0115c573c98b74a7ef91
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

This commit explicitly zeros out the buffer used for remaining bytes in
ctr_paes_do_crypt() to avoid exposing sensitive data. It also changes the
existing scrub in __xts_2keys_prep_param() to clean the entire parameter
block.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index ccc7da106c3a0..5a7dc46518854 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -1041,6 +1041,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  		memcpy(walk->dst.virt.addr, buf, nbytes);
>  		crypto_inc(walk->iv, AES_BLOCK_SIZE);
>  		rc = skcipher_walk_done(walk, 0);
> +		memzero_explicit(buf, sizeof(buf));
>  	}
>  
>  out:

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this fix still leave
sensitive cryptographic data exposed on the kernel stack during error paths?

In ctr_paes_do_crypt(), trailing partial bytes are copied into the stack
allocated buf array. Inside the subsequent while loop, if cpacf_kmctr() fails
and !maysleep is true, or if paes_convert_key() fails, the code executes a
goto out:

		while (1) {
			if (cpacf_kmctr(ctx->fc, param, buf,
					buf, AES_BLOCK_SIZE,
					walk->iv) == AES_BLOCK_SIZE)
				break;
			if (!maysleep) {
				rc = -EKEYEXPIRED;
				goto out;
			}
			rc = paes_convert_key(ctx, tested);
			if (rc)
				goto out;

Since the newly added memzero_explicit(buf, sizeof(buf)) is placed at the end
of the if (nbytes) block, the goto out jump bypasses the scrub entirely.
Would it be better to move the memzero_explicit() after the out label to
ensure the stack buffer is scrubbed even when returning an error?

[Severity: High]
This is a pre-existing issue, but there are similar temporary stack buffer
vulnerabilities in the unprotected aes_s390.c functions.

In ctr_aes_crypt(), a local buf array is used to process trailing partial
bytes but is never scrubbed before the function returns:

	if (nbytes) {
		memset(buf, 0, AES_BLOCK_SIZE);
		...
		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;

Additionally, in gcm_aes_crypt(), a param struct containing the raw AES key
(param.k) is constructed on the stack. During the scatterlist walk loop, if
either gcm_in_walk_done() or gcm_out_walk_done() fail, the function
immediately executes return -ENOMEM:

	if (gcm_in_walk_done(&gw_in, n) != n)
		return -ENOMEM;
	if (gcm_out_walk_done(&gw_out, n) != n)
		return -ENOMEM;

This early return bypasses the memzero_explicit(&param, sizeof(param))
cleanup at the end of the function, leaking the AES key on the stack. 
Could these similar cases in aes_s390.c be addressed as well?

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

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

[Harald Freudenberger]

The PAES 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: 6cd87cb5ef6c ("s390/crypto: Rework protected key AES for true asynch support")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.16+
---
 arch/s390/crypto/paes_s390.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 5a7dc4651885..4f65469d35fc 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -19,7 +19,7 @@
 #include <linux/init.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
+#include <linux/semaphore.h>
 #include <linux/spinlock.h>
 #include <crypto/aes.h>
 #include <crypto/algapi.h>
@@ -45,7 +45,7 @@ module_param_named(clrkey, pkey_clrkey_allowed, bool, 0444);
 MODULE_PARM_DESC(clrkey, "Allow clear key material (default N)");
 
 static u8 *ctrblk;
-static DEFINE_MUTEX(ctrblk_lock);
+static DEFINE_SEMAPHORE(ctrblk_sem, 1);
 
 static cpacf_mask_t km_functions, kmc_functions, kmctr_functions;
 
@@ -945,7 +945,8 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 	struct skcipher_walk *walk = &req_ctx->walk;
 	u8 buf[AES_BLOCK_SIZE], *ctrptr;
 	unsigned int nbytes, n, k;
-	int pk_state, locked, rc = 0;
+	int pk_state, rc = 0;
+	bool locked;
 
 	if (!req_ctx->param_init_done) {
 		/* fetch and check protected key state */
@@ -971,7 +972,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 	if (rc)
 		goto out;
 
-	locked = mutex_trylock(&ctrblk_lock);
+	locked = down_trylock(&ctrblk_sem) == 0;
 
 	/*
 	 * Note that in case of partial processing or failure the walk
@@ -993,21 +994,21 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 			rc = skcipher_walk_done(walk, nbytes - k);
 			if (rc) {
 				if (locked)
-					mutex_unlock(&ctrblk_lock);
+					up(&ctrblk_sem);
 				goto out;
 			}
 		}
 		if (k < n) {
 			if (!maysleep) {
 				if (locked)
-					mutex_unlock(&ctrblk_lock);
+					up(&ctrblk_sem);
 				rc = -EKEYEXPIRED;
 				goto out;
 			}
 			rc = paes_convert_key(ctx, tested);
 			if (rc) {
 				if (locked)
-					mutex_unlock(&ctrblk_lock);
+					up(&ctrblk_sem);
 				goto out;
 			}
 			spin_lock_bh(&ctx->pk_lock);
@@ -1016,7 +1017,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 		}
 	}
 	if (locked)
-		mutex_unlock(&ctrblk_lock);
+		up(&ctrblk_sem);
 
 	/* final block may be < AES_BLOCK_SIZE, copy only nbytes */
 	if (nbytes) {
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fixes a mutex used in atomic context for PAES, but leaves the exact same buggy pattern unfixed in the standard AES implementation.
- [High] Cryptographic keys, plaintext, and ciphertext leak onto the kernel stack due to missing zeroization on error paths and tail-block processing.
--

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

s390/crypto: Fix use of mutex in atomic context in PAES

This commit replaces a mutex with a binary semaphore for the PAES CTR
implementation. This resolves an issue where the mutex was unsafe to use in
atomic or interrupt contexts.

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

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 5a7dc46518854..4f65469d35fcd 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[Severity: High]
This is a pre-existing issue, but does the standard AES implementation need
the same fix?

The commit message notes that the mutex is not safe to use in atomic or
interrupt contexts. Looking at ctr_aes_crypt() in
arch/s390/crypto/aes_s390.c, it continues to use mutex_trylock(&ctrblk_lock)
in contexts where it may be called from softirq. Does that leave standard
AES CTR broken in atomic environments in the exact same way?

[ ... ]

> @@ -1016,7 +1017,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  		}
>  	}
>  	if (locked)
> -		mutex_unlock(&ctrblk_lock);
> +		up(&ctrblk_sem);
>  
>  	/* 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;
>  			if (!maysleep) {
>  				rc = -EKEYEXPIRED;
>  				goto out;
>  			}
>  			rc = paes_convert_key(ctx, tested);
>  			if (rc)
>  				goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path bypass
the scrubbing of sensitive stack buffers?

In ctr_paes_do_crypt(), if paes_convert_key() fails in the final block
processing loop, the code jumps to the out label, completely bypassing
memzero_explicit(buf, sizeof(buf)). Can this leave cryptographic material
exposed on the kernel stack?

There appear to be similar unscrubbed buffers in arch/s390/crypto/aes_s390.c,
such as ctr_aes_crypt() missing a memset on return entirely, and
gcm_aes_crypt() returning -ENOMEM directly when a scatterlist mapping fails,
which bypasses the zeroization of the master AES key.

>  			spin_lock_bh(&ctx->pk_lock);
>  			memcpy(param->key, ctx->pk.protkey, sizeof(param->key));
>  			spin_unlock_bh(&ctx->pk_lock);
>  		}
>  		memcpy(walk->dst.virt.addr, buf, nbytes);
>  		crypto_inc(walk->iv, AES_BLOCK_SIZE);
>  		rc = skcipher_walk_done(walk, 0);
>  		memzero_explicit(buf, sizeof(buf));
>  	}
> 
> out:
>  	pr_debug("rc=%d\n", rc);
>  	return rc;
>  }

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