[PATCH v3 0/6] Fixes and rework for paes_s390

[PATCH v3 0/6] 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.
- And some more fixes related to paes (see changelog).

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
v2: - On one error path in the CTR implementation the scrubbing of a
      temp buffer could be bypassed. Also checked Sashikos claim about
      possible double free but this is not the case.
    - All paes algorithms did not set any base.cra_flags. So now set
      the ASYNC and the NO_FALLBACK flag.
    - If a request is pushed to the crypto engine there is another
      return code EBUSY also indicating success but the code handled
      this as a failure.
    - The very same was with the phmac implementation. So corrected
      the return code handling there as well.   
v3: - Reworked the -EBUSY handling in paes and phmac again. The
      synchronous path could have emitted -EBUSY also and would have
      triggered the failure handling. So map -EBUSY to -EINPROGRESS.

Harald Freudenberger (6):
  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
  s390/crypto: Fix missing cra_flags in paes_s390
  s390/crypto: Fix handling of EBUSY in PAES when req is pushed to
    crypto engine
  s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to
    crypto engine

 arch/s390/crypto/paes_s390.c  | 68 +++++++++++++++++++++++++----------
 arch/s390/crypto/phmac_s390.c |  6 ++++
 2 files changed, 55 insertions(+), 19 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
--
2.43.0

[PATCH v3 1/6] 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

[PATCH v3 2/6] 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. Rework the code to
explicitly scrub the buffer at the end of the function 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..d075b0241f1f 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -1044,6 +1044,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
 	}
 
 out:
+	memzero_explicit(buf, sizeof(buf));
 	pr_debug("rc=%d\n", rc);
 	return rc;
 }
@@ -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

[PATCH v3 3/6] 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 d075b0241f1f..e8bbfc376ebb 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

[PATCH v3 4/6] s390/crypto: Fix missing cra_flags in paes_s390

[Harald Freudenberger]

The 4 algorithms implemented in paes_s390 never had any cra_flags
set. So add code which sets the cra_flag to CRYPTO_ALG_ASYNC and
CRYPTO_ALG_NO_FALLBACK.

Fixes: 4ccd065a69df ("crypto: ahash - Add support for drivers with no fallback")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.17+
---
 arch/s390/crypto/paes_s390.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index e8bbfc376ebb..03446bfac37f 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -579,6 +579,7 @@ static struct skcipher_engine_alg ecb_paes_alg = {
 		.base.cra_name	      = "ecb(paes)",
 		.base.cra_driver_name = "ecb-paes-s390",
 		.base.cra_priority    = 401,	/* combo: aes + ecb + 1 */
+		.base.cra_flags	      = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NO_FALLBACK,
 		.base.cra_blocksize   = AES_BLOCK_SIZE,
 		.base.cra_ctxsize     = sizeof(struct s390_paes_ctx),
 		.base.cra_module      = THIS_MODULE,
@@ -847,6 +848,7 @@ static struct skcipher_engine_alg cbc_paes_alg = {
 		.base.cra_name	      = "cbc(paes)",
 		.base.cra_driver_name = "cbc-paes-s390",
 		.base.cra_priority    = 402,	/* cbc-paes-s390 + 1 */
+		.base.cra_flags	      = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NO_FALLBACK,
 		.base.cra_blocksize   = AES_BLOCK_SIZE,
 		.base.cra_ctxsize     = sizeof(struct s390_paes_ctx),
 		.base.cra_module      = THIS_MODULE,
@@ -1162,6 +1164,7 @@ static struct skcipher_engine_alg ctr_paes_alg = {
 		.base.cra_name	      =	"ctr(paes)",
 		.base.cra_driver_name =	"ctr-paes-s390",
 		.base.cra_priority    =	402,	/* ecb-paes-s390 + 1 */
+		.base.cra_flags	      = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NO_FALLBACK,
 		.base.cra_blocksize   =	1,
 		.base.cra_ctxsize     =	sizeof(struct s390_paes_ctx),
 		.base.cra_module      =	THIS_MODULE,
@@ -1611,6 +1614,7 @@ static struct skcipher_engine_alg xts_paes_alg = {
 		.base.cra_name	      =	"xts(paes)",
 		.base.cra_driver_name =	"xts-paes-s390",
 		.base.cra_priority    =	402,	/* ecb-paes-s390 + 1 */
+		.base.cra_flags	      = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NO_FALLBACK,
 		.base.cra_blocksize   =	AES_BLOCK_SIZE,
 		.base.cra_ctxsize     =	sizeof(struct s390_pxts_ctx),
 		.base.cra_module      =	THIS_MODULE,
-- 
2.43.0

[PATCH v3 5/6] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine

[Harald Freudenberger]

When a request is transferred to the engine via
crypto_transfer_skcipher_request_to_engine() there are two return
codes signaling a successful transfer: EINPROGRESS and EBUSY. However
the correct handling of EBUSY was missing and has been added as a
return code indicating a successful transfer to the crypto engine.

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 03446bfac37f..1dd48f7a1bdf 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -494,6 +494,8 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 	if (rc == 0 || rc == -EKEYEXPIRED) {
 		atomic_inc(&ctx->via_engine_ctr);
 		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
@@ -763,6 +765,8 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 	if (rc == 0 || rc == -EKEYEXPIRED) {
 		atomic_inc(&ctx->via_engine_ctr);
 		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
@@ -1089,6 +1093,8 @@ static int ctr_paes_crypt(struct skcipher_request *req)
 	if (rc == 0 || rc == -EKEYEXPIRED) {
 		atomic_inc(&ctx->via_engine_ctr);
 		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
@@ -1529,6 +1535,8 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
 	if (rc == 0 || rc == -EKEYEXPIRED) {
 		atomic_inc(&ctx->via_engine_ctr);
 		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
-- 
2.43.0

[PATCH v3 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

[Harald Freudenberger]

When a request is transferred to the engine via
crypto_transfer_hash_request_to_engine() there are two return codes
signaling a successful transfer: EINPROGRESS and EBUSY. However the
correct handling of EBUSY was missing and has been added as a return
code indicating a successful transfer to the crypto engine.

Fixes: cbbc675506cc ("crypto: s390 - New s390 specific protected key hash phmac")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.17+
---
 arch/s390/crypto/phmac_s390.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
index 03ca33ffe6cc..9d77198b22ff 100644
--- a/arch/s390/crypto/phmac_s390.c
+++ b/arch/s390/crypto/phmac_s390.c
@@ -629,6 +629,8 @@ static int phmac_update(struct ahash_request *req)
 		req_ctx->async_op = OP_UPDATE;
 		atomic_inc(&tfm_ctx->via_engine_ctr);
 		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
@@ -667,6 +669,8 @@ static int phmac_final(struct ahash_request *req)
 		req_ctx->async_op = OP_FINAL;
 		atomic_inc(&tfm_ctx->via_engine_ctr);
 		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
@@ -716,6 +720,8 @@ static int phmac_finup(struct ahash_request *req)
 		/* req->async_op has been set to either OP_FINUP or OP_FINAL */
 		atomic_inc(&tfm_ctx->via_engine_ctr);
 		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
+		if (rc == -EBUSY)
+			rc = -EINPROGRESS;
 		if (rc != -EINPROGRESS)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
-- 
2.43.0

Re: [PATCH v3 4/6] s390/crypto: Fix missing cra_flags in paes_s390

[sashiko-bot]

> The 4 algorithms implemented in paes_s390 never had any cra_flags
> set. So add code which sets the cra_flag to CRYPTO_ALG_ASYNC and
> CRYPTO_ALG_NO_FALLBACK.
> 
> Fixes: 4ccd065a69df ("crypto: ahash - Add support for drivers with no fallback")
> 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/20260817141654.77940-1-freude@linux.ibm.com?part=4

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

[sashiko-bot]

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

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

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The unprotected AES implementation (`aes_s390.c`) suffers from the same missing stack buffer scrub issue that this patch fixes in `paes_s390.c`, leading to potential leakage of plaintext, ciphertext, and AES-CTR keystream bytes.
--

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

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

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. Rework the code to
explicitly scrub the buffer at the end of the function 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.

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

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index ccc7da106c3a0..d075b0241f1f5 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -1044,6 +1044,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  	}
>  
>  out:
> +	memzero_explicit(buf, sizeof(buf));

[Severity: High]
This isn't a bug introduced by this patch, but does the unprotected aes
implementation in arch/s390/crypto/aes_s390.c suffer from the same missing
stack buffer scrub issue?

Looking at ctr_aes_crypt() and gcm_aes_crypt() in aes_s390.c, there are stack
buffers used for partial blocks that appear to be left unscrubbed:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;
}

arch/s390/crypto/aes_s390.c:gcm_aes_crypt() {
    ...
	memzero_explicit(&param, sizeof(param));
	return ret;
}

The 16-byte stack buffer buf in ctr_aes_crypt() holds plaintext, ciphertext,
and aes keystream bytes from the zero-padding xored with the ctr keystream.
Similarly, the gw_in and gw_out stack structures in gcm_aes_crypt() contain
16-byte buf arrays that buffer partial plaintext and ciphertext blocks.

Since any userspace process (via af_alg) utilizing s390 hardware aes-ctr
for partial block lengths, or aes-gcm for unaligned data, could trigger
this, could this lead to a potential information leak of sensitive data?

Since this patch explicitly addresses this regression in the sibling
protected aes driver (paes_s390.c), should the unprotected aes driver be
patched as well?

>  	pr_debug("rc=%d\n", rc);
>  	return rc;
>  }

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

Re: [PATCH v3 3/6] 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:
- [Critical] Temporary stack buffer `buf` containing plaintext and keystream is not zeroed before returning from `ctr_aes_crypt`.
- [High] The identical `mutex_trylock()` in atomic context bug remains unfixed in `arch/s390/crypto/aes_s390.c` for standard AES CTR mode.
--

--- Patch [3]: [PATCH v3 3/6] s390/crypto: Fix use of mutex in atomic context in PAES ---
commit a7d845af838256909450e5168e0a9dc3dda32e5a
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 in the PAES CTR
implementation to allow fast CTR processing. This change avoids issues
when operating in atomic or interrupt contexts where mutexes are unsafe.

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

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index d075b0241f1f5..e8bbfc376ebb1 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does arch/s390/crypto/aes_s390.c also
need this exact same fix?

It looks like ctr_aes_crypt() in arch/s390/crypto/aes_s390.c still uses
mutex_trylock() in atomic context:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
    locked = mutex_trylock(&ctrblk_lock);
    ...
}

Since ctr_aes_crypt() implements standard AES CTR as part of the skcipher
API, it is frequently reached via network processing from softirq context.
Could this trigger lockdep splats or fail on RT kernels if not updated to
use a binary semaphore?

[ ... ]

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

[Severity: Critical]
This isn't a bug introduced by this patch, but while looking at the stack
buffer handling here, does arch/s390/crypto/aes_s390.c need to clear its
temporary buffer before returning?

In ctr_aes_crypt(), there is a temporary stack buffer buf that isn't
zeroed before the function returns:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
    if (nbytes) {
        ...
        ret = skcipher_walk_done(&walk, 0);
    }

    return ret;
}

During partial block processing, buf receives decrypted plaintext and the
remainder contains the raw AES CTR keystream. Since it isn't cleared explicitly,
could this leave cryptographic material on the kernel stack and expose
sensitive data to other components via uninitialized stack exposure vectors?

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

Re: [PATCH v3 5/6] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine

[sashiko-bot]

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

New issues:
- [High] The patch masks the `-EBUSY` backpressure signal by converting it to `-EINPROGRESS`, which breaks the crypto API's queue limit enforcement.

Pre-existing issues:
- [High] The module teardown in `paes_s390_fini` attempts to stop the crypto engine before unregistering the algorithms, potentially leaking the engine's kworker and causing a Use-After-Free.
--

--- Patch [5]: [PATCH v3 5/6] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine ---
commit 1a57782d904dcde97b280595a8b6669541d15c94
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine

When a request is transferred to the engine via
crypto_transfer_skcipher_request_to_engine() there are two return
codes signaling a successful transfer: EINPROGRESS and EBUSY. However
the correct handling of EBUSY was missing and has been added as a
return code indicating a successful transfer to the crypto engine.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..1dd48f7a1bdf2 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,6 +494,8 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does overwriting -EBUSY with -EINPROGRESS break the crypto API's queue limit
enforcement?

Callers like dm-crypt or IPsec rely on -EBUSY to signal that a request with
CRYPTO_TFM_REQ_MAY_BACKLOG was successfully enqueued, but the queue has
reached its maximum capacity.

By returning -EINPROGRESS instead, callers are falsely informed that the queue
is not full. This defeats the backpressure mechanism and allows callers to
continuously pump requests into the driver, potentially leading to unbounded
growth of the crypto engine queue and memory exhaustion.

Would it be better to preserve the -EBUSY return value but check
if (rc != -EINPROGRESS && rc != -EBUSY) before executing the local teardown
paths?

[ ... ]
> @@ -763,6 +765,8 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for CBC mode in the same way?

[ ... ]
> @@ -1089,6 +1093,8 @@ static int ctr_paes_crypt(struct skcipher_request *req)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for CTR mode?

[ ... ]
> @@ -1529,6 +1535,8 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for XTS mode?

As a general observation on the teardown path:

[Severity: High]
This isn't a bug introduced by this patch, but does paes_s390_fini() leak the
engine's kworker and cause a use-after-free by stopping the crypto engine
before unregistering the algorithms?

arch/s390/crypto/paes_s390.c:paes_s390_fini() {
	if (paes_crypto_engine) {
		crypto_engine_stop(paes_crypto_engine);
		crypto_engine_exit(paes_crypto_engine);
	}
	__crypto_unregister_skcipher(&ctr_paes_alg);
	...
}

If module initialization fails, paes_s390_fini() could be called while earlier
algorithms are already registered. In this window, another thread can
concurrently allocate a tfm and submit requests.

Because crypto_engine_stop() times out after 10 seconds if requests keep
arriving, it returns -EBUSY, causing crypto_engine_exit() to abort without
destroying the kworker thread.

After the algorithms are unregistered and the module memory is freed, the
surviving kworker will eventually process the queued requests and jump to the
unloaded do_one_request function, resulting in a kernel panic.

Should the algorithms be unregistered first to prevent new TFMs from being
allocated before tearing down the crypto engine?

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

Re: [PATCH v3 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

[sashiko-bot]

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

New issues:
- [High] The patch rewrites the `-EBUSY` return code from the crypto engine to `-EINPROGRESS`, improperly propagating a fake success state to the caller and breaking the crypto API backpressure mechanism.

Pre-existing issues:
- [Critical] `phmac_do_one_request` triggers a double completion when an asynchronous request fails with an error code.
- [Critical] Unconditional `hwh_advance()` on error paths leads to a double `kunmap_local()` or a NULL pointer dereference panic.
--

--- Patch [6]: [PATCH v3 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine ---
commit 049db089e02c25bd82461143ffc80704186a5ebb
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

When a request is transferred to the engine via
crypto_transfer_hash_request_to_engine() there are two return codes
signaling a successful transfer: EINPROGRESS and EBUSY. However the
correct handling of EBUSY was missing and has been added as a return
code indicating a successful transfer to the crypto engine.

> diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
> index 03ca33ffe6ccf..9d77198b22ff9 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c
> @@ -629,6 +629,8 @@ static int phmac_update(struct ahash_request *req)
>  		req_ctx->async_op = OP_UPDATE;
>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this break the crypto API backpressure mechanism?

When the engine's queue is full, crypto_enqueue_request() queues the
request to the backlog and returns -EBUSY. Callers like dm-crypt rely on
this -EBUSY signal to apply backpressure and throttle incoming requests.

By unconditionally mutating -EBUSY to -EINPROGRESS, do we deceive the caller
into thinking the request is processing normally, breaking the feedback loop
and allowing unbounded queue growth?

Furthermore, the crypto engine will later invoke the completion callback
with -EINPROGRESS when dequeued. Are callers who received -EINPROGRESS
initially instead of -EBUSY prepared for this state transition?

Would it be safer to decouple the local pending check from the return value?
For example:

if (rc != -EINPROGRESS && rc != -EBUSY)
	atomic_dec(&tfm_ctx->via_engine_ctr);

>  		if (rc != -EINPROGRESS)
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
>  	if (rc != -EINPROGRESS) {
>  		hwh_advance(hwh, rc);

[Severity: Critical]
This is a pre-existing issue, but does unconditional execution of
hwh_advance() on error paths risk a double kunmap_local() or a null
pointer dereference panic?

If a hash operation completes successfully locally but errors out later
(for instance, if -EKEYEXPIRED causes an -ENOSPC engine fallback), the walk
might already be completely consumed and successfully unmapped.

If we then call hwh_advance() on the already-closed walk, does it execute
crypto_hash_walk_done() again on the same address, resulting in a double
kunmap_local()?

Additionally, if the request size is 0 bytes, crypto_hash_walk_first()
might not map any memory at all. Could this trigger a panic by
unconditionally attempting to unmap a null address offset in the error path?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but looking at
phmac_do_one_request(), is there a risk of a double completion when an
asynchronous request fails?

arch/s390/crypto/phmac_s390.c:phmac_do_one_request() {
	...
	crypto_finalize_hash_request(engine, req, rc);
	local_bh_enable();
	return rc;
}

If an error occurs, the driver explicitly finalizes the request by calling
crypto_finalize_hash_request(), which synchronously completes the request
by invoking the caller's callback.

However, the function then returns the negative rc back to the crypto
engine. Seeing a negative return value and not -ENOSPC, will the crypto
engine core incorrectly assume the driver failed to take ownership of the
request and immediately invoke crypto_request_complete() a second time?

Should this function return 0 after explicitly finalizing a request?

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