[PATCH v4 0/7] Fixes and rework for paes_s390 and phmac_s390

[PATCH v4 0/7] Fixes and rework for paes_s390 and phmac_s390

[Harald Freudenberger]

Fix and rework some issues around arch/s390/paes_s390.c and
arch/s390/phmac_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 and phmac (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.
v4: - Mapping EBUSY to EINPROGRESS is not the right approach. So
      reworked again more thoroughly.
    - There came another reply from Sashiko about a possible double
      processing maybe even double free as a result of wrongly
      returning a failure code in the callback functions back to the
      engine when a request has been completed. The callback must
      return 0 - so another patch for this.

Harald Freudenberger (7):
  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
  s390/crypto: Fix wrong return code to engine in asynch callbacks

 arch/s390/crypto/paes_s390.c  | 96 ++++++++++++++++++++++++-----------
 arch/s390/crypto/phmac_s390.c | 32 ++++++++----
 2 files changed, 87 insertions(+), 41 deletions(-)

--
2.43.0

[PATCH v4 1/7] 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>
Reviewed-by: Holger Dengler <dengler@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 v4 2/7] 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>
Reviewed-by: Holger Dengler <dengler@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 v4 3/7] 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>
Reviewed-by: Holger Dengler <dengler@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 v4 4/7] 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>
Reviewed-by: Holger Dengler <dengler@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 v4 5/7] 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 | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 03446bfac37f..2048327c1ffe 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -463,6 +463,7 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_walk *walk = &req_ctx->walk;
 	bool tested = crypto_skcipher_tested(tfm);
+	bool cleanup = true;
 	int rc;
 
 	/*
@@ -494,15 +495,17 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (cleanup && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -732,6 +735,7 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
 	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_walk *walk = &req_ctx->walk;
 	bool tested = crypto_skcipher_tested(tfm);
+	bool cleanup = true;
 	int rc;
 
 	/*
@@ -763,15 +767,17 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (cleanup && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -1059,6 +1065,7 @@ static int ctr_paes_crypt(struct skcipher_request *req)
 	struct s390_paes_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_walk *walk = &req_ctx->walk;
 	bool tested = crypto_skcipher_tested(tfm);
+	bool cleanup = true;
 	int rc;
 
 	/*
@@ -1089,15 +1096,17 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (cleanup && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -1498,6 +1507,7 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
 	struct s390_pxts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_walk *walk = &req_ctx->walk;
 	bool tested = crypto_skcipher_tested(tfm);
+	bool cleanup = true;
 	int rc;
 
 	/*
@@ -1529,15 +1539,17 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (cleanup && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
-- 
2.43.0

[PATCH v4 6/7] 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 | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
index 03ca33ffe6cc..30c8a59af1a4 100644
--- a/arch/s390/crypto/phmac_s390.c
+++ b/arch/s390/crypto/phmac_s390.c
@@ -62,8 +62,10 @@ static inline int hwh_prepare(struct ahash_request *req,
  */
 static inline int hwh_advance(struct hash_walk_helper *hwh, int n)
 {
-	if (n < 0)
+	if (n < 0) {
+		hwh->walkbytes = n;
 		return crypto_hash_walk_done(&hwh->walk, n);
+	}
 
 	hwh->walkbytes -= n;
 	hwh->walkaddr += n;
@@ -606,6 +608,7 @@ static int phmac_update(struct ahash_request *req)
 	struct phmac_tfm_ctx *tfm_ctx = crypto_ahash_ctx(tfm);
 	struct kmac_sha2_ctx *kmac_ctx = &req_ctx->kmac_ctx;
 	struct hash_walk_helper *hwh = &req_ctx->hwh;
+	bool cleanup = true;
 	int rc;
 
 	/* prep the walk in the request context */
@@ -629,12 +632,15 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS) {
-		hwh_advance(hwh, rc);
+	if (cleanup) {
+		if (hwh->walkbytes > 0)
+			hwh_advance(hwh, rc);
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	}
 
@@ -649,6 +655,7 @@ static int phmac_final(struct ahash_request *req)
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
 	struct phmac_tfm_ctx *tfm_ctx = crypto_ahash_ctx(tfm);
 	struct kmac_sha2_ctx *kmac_ctx = &req_ctx->kmac_ctx;
+	bool cleanup = true;
 	int rc = 0;
 
 	/* Try synchronous operation if no active engine usage */
@@ -667,12 +674,14 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -685,6 +694,7 @@ static int phmac_finup(struct ahash_request *req)
 	struct phmac_tfm_ctx *tfm_ctx = crypto_ahash_ctx(tfm);
 	struct kmac_sha2_ctx *kmac_ctx = &req_ctx->kmac_ctx;
 	struct hash_walk_helper *hwh = &req_ctx->hwh;
+	bool cleanup = true;
 	int rc;
 
 	/* prep the walk in the request context */
@@ -716,15 +726,17 @@ 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 != -EINPROGRESS)
+		if (rc == -EINPROGRESS || rc == -EBUSY)
+			cleanup = false;
+		else
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (cleanup && hwh->walkbytes > 0)
 		hwh_advance(hwh, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	pr_debug("rc=%d\n", rc);
 	return rc;
-- 
2.43.0

[PATCH v4 7/7] s390/crypto: Fix wrong return code to engine in asynch callbacks

[Harald Freudenberger]

When crypto_finalize_hash_request() or
crypto_finalize_skcipher_request() explicitly completes a request, the
do_one_request callback must return 0 to indicate successful
handling. Returning a negative error code causes the crypto engine to
assume the driver failed to take ownership and triggers a second
completion via crypto_request_complete(), resulting in a double
completion. This pattern occurs in paes_s390.c 4 times and once in
phmac_s390.c.

Fixed in phmac_do_one_request() and all four paes do_one_request
callbacks (ecb, cbc, ctr, xts) by returning 0 after explicit
finalization instead of propagating the error code.

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 ++++----
 arch/s390/crypto/phmac_s390.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 2048327c1ffe..0bc360c7ef15 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -574,7 +574,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
 	atomic_dec(&ctx->via_engine_ctr);
 	crypto_finalize_skcipher_request(engine, req, rc);
 	local_bh_enable();
-	return rc;
+	return 0;
 }
 
 static struct skcipher_engine_alg ecb_paes_alg = {
@@ -846,7 +846,7 @@ static int cbc_paes_do_one_request(struct crypto_engine *engine, void *areq)
 	atomic_dec(&ctx->via_engine_ctr);
 	crypto_finalize_skcipher_request(engine, req, rc);
 	local_bh_enable();
-	return rc;
+	return 0;
 }
 
 static struct skcipher_engine_alg cbc_paes_alg = {
@@ -1165,7 +1165,7 @@ static int ctr_paes_do_one_request(struct crypto_engine *engine, void *areq)
 	atomic_dec(&ctx->via_engine_ctr);
 	crypto_finalize_skcipher_request(engine, req, rc);
 	local_bh_enable();
-	return rc;
+	return 0;
 }
 
 static struct skcipher_engine_alg ctr_paes_alg = {
@@ -1618,7 +1618,7 @@ static int xts_paes_do_one_request(struct crypto_engine *engine, void *areq)
 	atomic_dec(&ctx->via_engine_ctr);
 	crypto_finalize_skcipher_request(engine, req, rc);
 	local_bh_enable();
-	return rc;
+	return 0;
 }
 
 static struct skcipher_engine_alg xts_paes_alg = {
diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
index 30c8a59af1a4..89c5fa3dced2 100644
--- a/arch/s390/crypto/phmac_s390.c
+++ b/arch/s390/crypto/phmac_s390.c
@@ -945,7 +945,7 @@ static int phmac_do_one_request(struct crypto_engine *engine, void *areq)
 	atomic_dec(&tfm_ctx->via_engine_ctr);
 	crypto_finalize_hash_request(engine, req, rc);
 	local_bh_enable();
-	return rc;
+	return 0;
 }
 
 #define S390_ASYNC_PHMAC_ALG(x)						\
-- 
2.43.0

Re: [PATCH v4 4/7] 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/20260819091734.7331-1-freude@linux.ibm.com?part=4

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The standard AES CTR implementation uses `mutex_trylock()` and `mutex_unlock()` in a context that may be atomic, which violates core kernel locking rules.
--

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

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

The PAES CTR implementation used a mutex to lock one page of exclusive
memory for fast CTR processing. This commit replaces the mutex with a
binary semaphore, since a mutex is not safe to use in atomic or interrupt
context.

> 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
[ ... ]
> @@ -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);

[Severity: High]
This is a pre-existing issue, but does the standard AES implementation in
arch/s390/crypto/aes_s390.c suffer from this exact same problem?

Looking at ctr_aes_crypt() in arch/s390/crypto/aes_s390.c:

	locked = mutex_trylock(&ctrblk_lock);

	ret = skcipher_walk_virt(&walk, req, false);
	while ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE) {
	...
	}
	if (locked)
		mutex_unlock(&ctrblk_lock);

Network traffic processing (e.g., IPsec) executes SKCIPHER operations from
a softirq context. Unlocking a mutex in an interrupt context triggers a
kernel panic or BUG on PREEMPT_RT kernels, and lockdep warnings on
standard kernels.

Since this patch successfully fixes the issue for PAES, should a similar
fix be applied to arch/s390/crypto/aes_s390.c?

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

Re: [PATCH v4 7/7] s390/crypto: Fix wrong return code to engine in asynch callbacks

[sashiko-bot]

> When crypto_finalize_hash_request() or
> crypto_finalize_skcipher_request() explicitly completes a request, the
> do_one_request callback must return 0 to indicate successful
> handling. Returning a negative error code causes the crypto engine to
> assume the driver failed to take ownership and triggers a second
> completion via crypto_request_complete(), resulting in a double
> completion. This pattern occurs in paes_s390.c 4 times and once in
> phmac_s390.c.
> 
> Fixed in phmac_do_one_request() and all four paes do_one_request
> callbacks (ecb, cbc, ctr, xts) by returning 0 after explicit
> finalization instead of propagating the error code.
> 
> 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/20260819091734.7331-1-freude@linux.ibm.com?part=7

Re: [PATCH v4 5/7] 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:

Pre-existing issues:
- [High] The patch corrects the handling of the `-EBUSY` return code for the crypto engine in `paes_s390.c` but fails to apply the identical fix to `phmac_s390.c`, leading to state corruption and Use-After-Free.
- [High] The `do_one_request` engine callbacks complete the crypto request twice if a synchronous error occurs during processing, leading to a double completion and Use-After-Free.
--

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

s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine
    
This commit adds -EBUSY as a successful return code when transferring
skcipher requests to the crypto engine in paes_s390.c. This prevents
state cleanup on requests that are successfully queued on the backlog.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..2048327c1ffe4 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,15 +495,17 @@ 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 != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;
> +		else
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but should the same fix be applied
to the hash requests in phmac_s390.c?

For example, in phmac_update():

arch/s390/crypto/phmac_s390.c:phmac_update() {
    ...
	if (rc == 0 || rc == -EKEYEXPIRED) {
		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 != -EINPROGRESS)
			atomic_dec(&tfm_ctx->via_engine_ctr);
	}

	if (rc != -EINPROGRESS) {
		hwh_advance(hwh, rc);
		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
	}
    ...
}

It looks like phmac_update, phmac_final, and phmac_finup also incorrectly
treat -EBUSY as a failure, which could dismantle the state needed by the
engine thread.

[ ... ]

[Severity: High]
This is a pre-existing issue, but could the do_one_request callbacks lead to
a double completion if a synchronous error occurs?

In ecb_paes_do_one_request():

arch/s390/crypto/paes_s390.c:ecb_paes_do_one_request() {
    ...
	crypto_finalize_skcipher_request(engine, req, rc);
	local_bh_enable();
	return rc;
}

If rc is negative, does returning it here after calling
crypto_finalize_skcipher_request() force the engine thread to incorrectly
complete the request a second time?

Could this also affect cbc_paes_do_one_request(), ctr_paes_do_one_request(),
xts_paes_do_one_request(), and phmac_do_one_request()?

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

Re: [PATCH v4 2/7] 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] Incomplete fix: stack buffers containing sensitive user plaintext/ciphertext are left unscrubbed in standard AES functions, identical to the pattern fixed in the PAES functions.
--

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

This commit explicitly scrubs a stack buffer used to process remaining bytes
in ctr_paes_do_crypt(), and broadens a scrub in __xts_2keys_prep_param() to
clear the entire parameter block. This prevents potentially sensitive data
from being left exposed.

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 this missing scrub pattern
also need to be fixed in the standard AES functions?

In arch/s390/crypto/aes_s390.c:ctr_aes_crypt(), the buf stack array
is used to process the final block data, but it doesn't appear to be
explicitly zeroed before the function returns:

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

    return ret;
}

Similarly, in arch/s390/crypto/aes_s390.c:gcm_aes_crypt(), the gw_in
and gw_out structs contain local buf arrays that store block data
and seem to remain on the stack without being scrubbed:

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

Could cryptographic operations using ctr(aes) or gcm(aes) with data lengths
not aligned to block boundaries leave unscrubbed plaintext or ciphertext on
the stack, similar to the vulnerability fixed here for PAES?

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

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

Re: [PATCH v4 1/7] 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:
- [Critical] Silent data corruption and broken serialization when hardware queue is full and backlog is enabled.
--

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

This patch adds immediate error checks after skcipher_walk_done() in all
main PAES cipher processing loops. It also checks if the walk has already
been finalized before calling skcipher_walk_done() to avoid double
de-allocation of resources.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 8cfe6166c193c..ccc7da106c3a0 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[ ... ]

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

[Severity: Critical]
This isn't a bug introduced by this patch, but does this logic safely handle
requests when the hardware queue is full?

If crypto_transfer_skcipher_request_to_engine() returns -EBUSY, the request
is successfully backlogged.

However, because rc is not -EINPROGRESS, it appears the driver decrements
the engine counter and calls skcipher_walk_done() here to tear down the
walk.

When the engine later processes this backlogged request, will it operate on
an empty walk and zeroed parameters, silently returning success without
processing the data? Could this lead to data corruption when under heavy I/O
load?

[ ... ]

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

[Severity: Critical]
This is a pre-existing issue, but does this cbc_paes_crypt() implementation
share the same -EBUSY fallback issue described above, where backlogged
requests are prematurely torn down?

[ ... ]

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

[Severity: Critical]
This is a pre-existing issue, but does ctr_paes_crypt() also suffer from the
same backlogged request cleanup problem when rc is -EBUSY?

[ ... ]

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

[Severity: Critical]
This is a pre-existing issue, but does xts_paes_crypt() similarly mask
-EBUSY errors, potentially leading to plaintext being written to disk under
heavy load?

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

Re: [PATCH v4 6/7] 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:

Pre-existing issues:
- [Critical] Thread-local memory mapping (kmap_local_page) is leaked across an asynchronous boundary, leading to an invalid cross-thread kunmap_local.
- [High] Double completion of asynchronous requests on error paths in `phmac_do_one_request`.
- [High] Persistent hardware busy state causes the driver to complete the request with `-EBUSY`, which the crypto API swallows, leading to an infinite wait.
--

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

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

This commit fixes the handling of the -EBUSY return code when a request is
transferred to the crypto engine via crypto_transfer_hash_request_to_engine().
It adds -EBUSY as a successful transfer indicator alongside -EINPROGRESS,
preventing premature cleanup of the request context.

> diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
> index 03ca33ffe6ccf..30c8a59af1a4e 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c

[ ... ]

> @@ -629,12 +632,15 @@ 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 != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;

[Severity: Critical]
This is a pre-existing issue, but does setting cleanup = false here leak a
thread-local memory mapping across an asynchronous boundary?

When the submitter thread maps the scatterlist page using kmap_local_page()
via hwh_prepare() calling into crypto_hash_walk_first(), the map is strictly
thread-local. If the request is pushed to the engine and the submitter thread
returns, the local memory remains mapped in its thread context.

Later, the crypto_engine kworker thread processes the queued request in
arch/s390/crypto/phmac_s390.c:phmac_do_one_request(), calling
crypto_hash_walk_done(), which invokes kunmap_local() cross-thread. Can this
cause a kernel BUG or panic by violating the thread-locality requirements of
kmap_local_page()?

> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS) {
> -		hwh_advance(hwh, rc);
> +	if (cleanup) {
> +		if (hwh->walkbytes > 0)
> +			hwh_advance(hwh, rc);
>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>  	}
>  

[ ... ]

> @@ -667,12 +674,14 @@ 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 != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;

[Severity: High]
This is a pre-existing issue, but does the driver properly translate persistent
-EBUSY hardware errors before completion?

If the hardware persistently returns -EBUSY when deriving a protected key in
arch/s390/crypto/phmac_s390.c:convert_key(), it exhausts its retries and
returns -EBUSY. This propagates to phmac_do_one_request(), which explicitly
finalizes the request:

    crypto_finalize_hash_request(engine, req, rc);

Since the crypto API's completion wrapper ahash_op_done() explicitly swallows
-EBUSY because it treats it as a queueing status, completing a request with
-EBUSY prevents the final completion callback from firing. Could this cause
callers using crypto_wait_req() to block forever?

> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
>  out:
> -	if (rc != -EINPROGRESS)
> +	if (cleanup)
>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>  	pr_debug("rc=%d\n", rc);
>  	return rc;

[ ... ]

> @@ -716,15 +726,17 @@ 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 != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;
> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}

[Severity: High]
This is a pre-existing issue, but is there a risk of double completion on error
paths in arch/s390/crypto/phmac_s390.c:phmac_do_one_request()?

If hardware execution fails and returns an error like -EIO,
phmac_do_one_request() manually completes the request:

    crypto_finalize_hash_request(engine, req, rc);
    local_bh_enable();
    return rc;

By returning the negative rc back to the crypto_engine core, and because the
engine allocates with retry_support=true, the core in crypto_pump_requests()
will treat this as a failure it must handle. Does this trigger an automatic
fallback crypto_request_complete() that invokes the user's completion callback
a second time?

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

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

[Holger Dengler]

On 8/19/26 11:17, Harald Freudenberger wrote:
> 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+

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

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

Re: [PATCH v4 7/7] s390/crypto: Fix wrong return code to engine in asynch callbacks

[Holger Dengler]

On 8/19/26 11:17, Harald Freudenberger wrote:
> When crypto_finalize_hash_request() or
> crypto_finalize_skcipher_request() explicitly completes a request, the
> do_one_request callback must return 0 to indicate successful
> handling. Returning a negative error code causes the crypto engine to
> assume the driver failed to take ownership and triggers a second
> completion via crypto_request_complete(), resulting in a double
> completion. This pattern occurs in paes_s390.c 4 times and once in
> phmac_s390.c.
> 
> Fixed in phmac_do_one_request() and all four paes do_one_request
> callbacks (ecb, cbc, ctr, xts) by returning 0 after explicit
> finalization instead of propagating the error code.
> 
> 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+

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

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

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

[Harald Freudenberger]

On 2026-08-19 11:37, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Thread-local memory mapping (kmap_local_page) is leaked
> across an asynchronous boundary, leading to an invalid cross-thread
> kunmap_local.
> - [High] Double completion of asynchronous requests on error paths in
> `phmac_do_one_request`.
> - [High] Persistent hardware busy state causes the driver to complete
> the request with `-EBUSY`, which the crypto API swallows, leading to
> an infinite wait.
> --
> 
> --- Patch [6]: [PATCH v4 6/7] s390/crypto: Fix handling of EBUSY in
> PHMAC when req is pushed to crypto engine ---
> commit 85264caf4d41b4adaeaed2f15efd8b287915969c
> Author: Harald Freudenberger <freude@linux.ibm.com>
> 
> s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to 
> crypto engine
> 
> This commit fixes the handling of the -EBUSY return code when a request 
> is
> transferred to the crypto engine via 
> crypto_transfer_hash_request_to_engine().
> It adds -EBUSY as a successful transfer indicator alongside 
> -EINPROGRESS,
> preventing premature cleanup of the request context.
> 
>> diff --git a/arch/s390/crypto/phmac_s390.c 
>> b/arch/s390/crypto/phmac_s390.c
>> index 03ca33ffe6ccf..30c8a59af1a4e 100644
>> --- a/arch/s390/crypto/phmac_s390.c
>> +++ b/arch/s390/crypto/phmac_s390.c
> 
> [ ... ]
> 
>> @@ -629,12 +632,15 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: Critical]
> This is a pre-existing issue, but does setting cleanup = false here 
> leak a
> thread-local memory mapping across an asynchronous boundary?
> 
> When the submitter thread maps the scatterlist page using 
> kmap_local_page()
> via hwh_prepare() calling into crypto_hash_walk_first(), the map is 
> strictly
> thread-local. If the request is pushed to the engine and the submitter 
> thread
> returns, the local memory remains mapped in its thread context.
> 
> Later, the crypto_engine kworker thread processes the queued request in
> arch/s390/crypto/phmac_s390.c:phmac_do_one_request(), calling
> crypto_hash_walk_done(), which invokes kunmap_local() cross-thread. Can 
> this
> cause a kernel BUG or panic by violating the thread-locality 
> requirements of
> kmap_local_page()?
> 

This is not exactly true. On s390 CONFIG_HIGHMEM is NOT set.

 From /ext/git/s390-linux/include/crypto/scatterwalk.h:

   static inline void scatterwalk_map(struct scatter_walk *walk)
   {
      if (IS_ENABLED(CONFIG_HIGHMEM)) {
          // Uses kmap_local_page() - THREAD-LOCAL
          addr = kmap_local_page(page) + offset;
      } else {
          // Direct mapping - NO thread restrictions
          addr = page_address(base_page) + offset;
      }
   }

So there is no thread restriction on s390 and paes and phmac are both 
only s390.

>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>> -	if (rc != -EINPROGRESS) {
>> -		hwh_advance(hwh, rc);
>> +	if (cleanup) {
>> +		if (hwh->walkbytes > 0)
>> +			hwh_advance(hwh, rc);
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	}
>> 
> 
> [ ... ]
> 
>> @@ -667,12 +674,14 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: High]
> This is a pre-existing issue, but does the driver properly translate 
> persistent
> -EBUSY hardware errors before completion?
> 
> If the hardware persistently returns -EBUSY when deriving a protected 
> key in
> arch/s390/crypto/phmac_s390.c:convert_key(), it exhausts its retries 
> and
> returns -EBUSY. This propagates to phmac_do_one_request(), which 
> explicitly
> finalizes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
> 
> Since the crypto API's completion wrapper ahash_op_done() explicitly 
> swallows
> -EBUSY because it treats it as a queueing status, completing a request 
> with
> -EBUSY prevents the final completion callback from firing. Could this 
> cause
> callers using crypto_wait_req() to block forever?
> 
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>>  out:
>> -	if (rc != -EINPROGRESS)
>> +	if (cleanup)
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	pr_debug("rc=%d\n", rc);
>>  	return rc;
> 
> [ ... ]
> 
>> @@ -716,15 +726,17 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
> 
> [Severity: High]
> This is a pre-existing issue, but is there a risk of double completion 
> on error
> paths in arch/s390/crypto/phmac_s390.c:phmac_do_one_request()?
> 
> If hardware execution fails and returns an error like -EIO,
> phmac_do_one_request() manually completes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
>     local_bh_enable();
>     return rc;
> 
> By returning the negative rc back to the crypto_engine core, and 
> because the
> engine allocates with retry_support=true, the core in 
> crypto_pump_requests()
> will treat this as a failure it must handle. Does this trigger an 
> automatic
> fallback crypto_request_complete() that invokes the user's completion 
> callback
> a second time?

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

[Harald Freudenberger]

On 2026-08-19 11:37, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Thread-local memory mapping (kmap_local_page) is leaked
> across an asynchronous boundary, leading to an invalid cross-thread
> kunmap_local.
> - [High] Double completion of asynchronous requests on error paths in
> `phmac_do_one_request`.
> - [High] Persistent hardware busy state causes the driver to complete
> the request with `-EBUSY`, which the crypto API swallows, leading to
> an infinite wait.
> --
> 
> --- Patch [6]: [PATCH v4 6/7] s390/crypto: Fix handling of EBUSY in
> PHMAC when req is pushed to crypto engine ---
> commit 85264caf4d41b4adaeaed2f15efd8b287915969c
> Author: Harald Freudenberger <freude@linux.ibm.com>
> 
> s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to 
> crypto engine
> 
> This commit fixes the handling of the -EBUSY return code when a request 
> is
> transferred to the crypto engine via 
> crypto_transfer_hash_request_to_engine().
> It adds -EBUSY as a successful transfer indicator alongside 
> -EINPROGRESS,
> preventing premature cleanup of the request context.
> 
>> diff --git a/arch/s390/crypto/phmac_s390.c 
>> b/arch/s390/crypto/phmac_s390.c
>> index 03ca33ffe6ccf..30c8a59af1a4e 100644
>> --- a/arch/s390/crypto/phmac_s390.c
>> +++ b/arch/s390/crypto/phmac_s390.c
> 
> [ ... ]
> 
>> @@ -629,12 +632,15 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: Critical]
> This is a pre-existing issue, but does setting cleanup = false here 
> leak a
> thread-local memory mapping across an asynchronous boundary?
> 
> When the submitter thread maps the scatterlist page using 
> kmap_local_page()
> via hwh_prepare() calling into crypto_hash_walk_first(), the map is 
> strictly
> thread-local. If the request is pushed to the engine and the submitter 
> thread
> returns, the local memory remains mapped in its thread context.
> 
> Later, the crypto_engine kworker thread processes the queued request in
> arch/s390/crypto/phmac_s390.c:phmac_do_one_request(), calling
> crypto_hash_walk_done(), which invokes kunmap_local() cross-thread. Can 
> this
> cause a kernel BUG or panic by violating the thread-locality 
> requirements of
> kmap_local_page()?
> 
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>> -	if (rc != -EINPROGRESS) {
>> -		hwh_advance(hwh, rc);
>> +	if (cleanup) {
>> +		if (hwh->walkbytes > 0)
>> +			hwh_advance(hwh, rc);
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	}
>> 
> 
> [ ... ]
> 
>> @@ -667,12 +674,14 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: High]
> This is a pre-existing issue, but does the driver properly translate 
> persistent
> -EBUSY hardware errors before completion?
> 
> If the hardware persistently returns -EBUSY when deriving a protected 
> key in
> arch/s390/crypto/phmac_s390.c:convert_key(), it exhausts its retries 
> and
> returns -EBUSY. This propagates to phmac_do_one_request(), which 
> explicitly
> finalizes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
> 
> Since the crypto API's completion wrapper ahash_op_done() explicitly 
> swallows
> -EBUSY because it treats it as a queueing status, completing a request 
> with
> -EBUSY prevents the final completion callback from firing. Could this 
> cause
> callers using crypto_wait_req() to block forever?

Yep, this may be an issue - never complete a request with -EBUSY.

> 
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>>  out:
>> -	if (rc != -EINPROGRESS)
>> +	if (cleanup)
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	pr_debug("rc=%d\n", rc);
>>  	return rc;
> 
> [ ... ]
> 
>> @@ -716,15 +726,17 @@ 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 != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
> 
> [Severity: High]
> This is a pre-existing issue, but is there a risk of double completion 
> on error
> paths in arch/s390/crypto/phmac_s390.c:phmac_do_one_request()?
> 
> If hardware execution fails and returns an error like -EIO,
> phmac_do_one_request() manually completes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
>     local_bh_enable();
>     return rc;
> 
> By returning the negative rc back to the crypto_engine core, and 
> because the
> engine allocates with retry_support=true, the core in 
> crypto_pump_requests()
> will treat this as a failure it must handle. Does this trigger an 
> automatic
> fallback crypto_request_complete() that invokes the user's completion 
> callback
> a second time?

Fixed in patch #8 of this series.