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

[PATCH v2 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.   

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


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
--
2.43.0

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The wrapper functions (e.g., ecb_paes_crypt) incorrectly treat a return value of -EBUSY from crypto_transfer_skcipher_request_to_engine() as an enqueue failure, leading to a Use-After-Free when the crypto engine eventually processes the request.
- [Critical] The async request handler returns an error code after already finalizing the request, causing the crypto engine to complete the request a second time.
--

--- Patch [1]: [PATCH v2 1/6] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit 8ebdb4de82e72b4c0a21b2b0f1f7cb80c8245b4f
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

This commit adds immediate error checks after skcipher_walk_done()
in PAES algorithms to prevent error masking. It also reworks how
walk->nbytes is checked to avoid double de-allocation of walk resources.

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

> 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 is a pre-existing issue, but does this code properly handle backlog
queueing?

If crypto_transfer_skcipher_request_to_engine() returns -EBUSY when the
hardware queue reaches its soft limit, the request is still enqueued.

Because -EBUSY != -EINPROGRESS, this error path will execute and call
skcipher_walk_done(), destroying the scatterlist walk and zeroing
parameters.

Since the engine will eventually process the enqueued request, could this
result in a use-after-free when the engine accesses the destroyed walk
structure? This pattern also appears in cbc_paes_crypt(), ctr_paes_crypt(),
and xts_paes_crypt().

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

[Severity: Critical]
This is a pre-existing issue, but does returning the error code at the end
of this function cause the crypto engine to double-complete the request?

In ecb_paes_do_one_request() and the other async handlers, if
ecb_paes_do_crypt() returns a fatal error, the code finalizes the request
and later returns the error code:

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

The crypto_pump_requests() loop in the engine framework interprets any
returned error other than -ENOSPC as an execution failure and manually calls
crypto_request_complete() a second time.

Could this double completion lead to a use-after-free if the caller frees
the request after the first callback?

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

[PATCH v2 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+

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Missing memory scrub on stack temporary bounce buffer in `ctr_aes_crypt()`.
- [Medium] Missing memory scrub on stack bounce buffers in `gcm_aes_crypt()`.
--

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

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

This patch explicitly scrubs the stack bounce buffer used for partial block
processing in ctr_paes_do_crypt(). It also updates __xts_2keys_prep_param()
to scrub the entire pcc_param structure rather than just the key field,
preventing inadvertent exposure of sensitive cryptographic material.

> 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: Medium]
This is a pre-existing issue, but does ctr_aes_crypt() in
arch/s390/crypto/aes_s390.c have the same missing scrub on its temporary
bounce buffer?

Like the PAES algorithm fixed here, intermediate cryptographic data
appears to be left on the kernel stack when an AES-CTR operation
processes a final partial block that is not a multiple of AES_BLOCK_SIZE:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
	if (nbytes) {
		memset(buf, 0, AES_BLOCK_SIZE);
		memcpy(buf, walk.src.virt.addr, nbytes);
		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,
			    AES_BLOCK_SIZE, walk.iv);
		memcpy(walk.dst.virt.addr, buf, nbytes);
		crypto_inc(walk.iv, AES_BLOCK_SIZE);
		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;
}

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

[Severity: Medium]
This is also a pre-existing issue, but does gcm_aes_crypt() in
arch/s390/crypto/aes_s390.c also need its stack bounce buffers scrubbed?

It explicitly scrubs the param structure, but seems to omit scrubbing
gw_in and gw_out. This leaves intermediate cryptographic data on the stack
when an AES-GCM operation with scattered or unaligned data uses the internal
bounce buffers:

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

>  	return rc;
>  }

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

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

Re: [PATCH v2 3/6] 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 exact same `mutex_trylock()` in atomic context bug fixed by this patch in PAES is also present in standard AES CTR (`arch/s390/crypto/aes_s390.c`) but was left unpatched.
--

--- Patch [3]: [PATCH v2 3/6] s390/crypto: Fix use of mutex in atomic context in PAES ---
commit e6433be4cd856ce7a9d00fbe2d231629541a50c3
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. This prevents panics or lockdep warnings when the CTR
processing is called from atomic or interrupt contexts.

> 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 isn't a bug introduced by this patch, but does the same fix need to be
applied to the standard AES CTR implementation in
arch/s390/crypto/aes_s390.c?

Looking at ctr_aes_crypt() in that file, it uses the exact same locking
pattern:

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

Since skcipher algorithms can be called from atomic or softirq contexts
during operations like ipsec network packet processing, could this similarly
trigger a panic or lockdep warning?

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

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

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

[PATCH v2 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 | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 03446bfac37f..b60696c49fcc 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -494,15 +494,15 @@ 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)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -763,15 +763,15 @@ 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)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -1089,15 +1089,15 @@ 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)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -1529,15 +1529,15 @@ 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)
 			atomic_dec(&ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS && walk->nbytes)
+	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
 		skcipher_walk_done(walk, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
 	pr_debug("rc=%d\n", rc);
 	return rc;
-- 
2.43.0

Re: [PATCH v2 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 incorrectly skips cleanup on all -EBUSY return codes, failing to distinguish between asynchronous enqueue success and synchronous hardware errors.

Pre-existing issues:
- [Critical] The handling of -EBUSY when transferring hash requests to the crypto engine is broken in HMAC algorithms, leading to memory corruption and use-after-free.
--

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

This commit addresses a missing check for -EBUSY when a request is
successfully transferred to the crypto engine backlog in the PAES algorithms.
It updates the return code checks to treat -EBUSY similarly to -EINPROGRESS to
avoid incorrectly cleaning up the request while it is still queued.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..b60696c49fccf 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,15 +494,15 @@ 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)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS && walk->nbytes)
> +	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
>  		skcipher_walk_done(walk, rc);
>  
>  out:
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && rc != -EBUSY)
>  		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));

[Severity: High]
Does this bypass cleanup if ecb_paes_do_crypt() fails synchronously?

Looking at ecb_paes_crypt(), ecb_paes_do_crypt() can fail synchronously and
return -EBUSY due to hardware key conversion failures:

    if (!atomic_read(&ctx->via_engine_ctr)) {
        rc = ecb_paes_do_crypt(ctx, req_ctx, tested, false);
        if (rc == 0)
            goto out;
    }

If rc is -EBUSY, the fallback block is skipped:

    if (rc == 0 || rc == -EKEYEXPIRED) {
        atomic_inc(&ctx->via_engine_ctr);
        rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
        ...

Then we reach the updated cleanup logic, which now skips execution when rc is
-EBUSY. Could this lead to leaked resources from skcipher_walk_done() and an
indefinite wait, since the caller interprets -EBUSY as a successful queue to the
engine but no callback will ever arrive?

Similar logic is updated in cbc_paes_crypt(), ctr_paes_crypt(), and
xts_paes_crypt().


This is a pre-existing issue, but I noticed a similar pattern in phmac_s390.c
that might need addressing.

arch/s390/crypto/phmac_s390.c:phmac_update()
    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));
    }

[Severity: Critical]
If crypto_transfer_hash_request_to_engine() returns -EBUSY when the request is
successfully added to the backlog queue, will this incorrectly treat it as a
failure?

If so, this could unmap the scatterlist via hwh_advance() and zero the hash
context via memzero_explicit() while the crypto engine later operates on it
asynchronously.

Is it necessary to apply the same -EBUSY handling fixes to the HMAC algorithms?

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

[PATCH v2 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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
index 03ca33ffe6cc..adf14553ad39 100644
--- a/arch/s390/crypto/phmac_s390.c
+++ b/arch/s390/crypto/phmac_s390.c
@@ -629,11 +629,11 @@ 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)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS) {
+	if (rc != -EINPROGRESS && rc != -EBUSY) {
 		hwh_advance(hwh, rc);
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	}
@@ -667,12 +667,12 @@ 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)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	pr_debug("rc=%d\n", rc);
 	return rc;
@@ -716,15 +716,15 @@ 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)
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		hwh_advance(hwh, rc);
 
 out:
-	if (rc != -EINPROGRESS)
+	if (rc != -EINPROGRESS && rc != -EBUSY)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
 	pr_debug("rc=%d\n", rc);
 	return rc;
-- 
2.43.0

Re: [PATCH v2 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 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] API Violation: kmap_local_page() mapped in one thread and unmapped in another
- [High] Double-Unmap / Memory Corruption in `phmac_finup` error path
--

--- Patch [6]: [PATCH v2 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine ---
commit 937135347da9f51601b4d25da73d8f9de9ecb6c8
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..adf14553ad392 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c
> @@ -629,11 +629,11 @@ 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);

[Severity: Critical]
This is a pre-existing issue, but does deferring this request to the crypto
engine kworker thread cause a cross-thread kmap_local_page() violation?

Earlier in phmac_update(), hwh_prepare() maps memory into the local task.
If the request is deferred to the async engine, the local map context will
be leaked, causing kunmap_local() corruption when it is unmapped in the
worker thread.

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

[ ... ]

> @@ -716,15 +716,15 @@ 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);

[Severity: Critical]
This is a pre-existing issue, but does deferring this request to the crypto
engine kworker thread cause a cross-thread kmap_local_page() violation?

Similar to phmac_update(), hwh_prepare() maps memory into the local task.
If the request is deferred to the async engine, the local map context will
be leaked, causing kunmap_local() corruption when it is unmapped in the
worker thread.

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

[Severity: High]
This isn't a bug introduced by this patch, but could unconditionally calling
hwh_advance() here on an engine fallback error path cause a double-unmap?

On a fallback failure, unconditionally calling hwh_advance() causes
crypto_hash_walk_done() to call kunmap_local() on already unmapped
pointers, potentially resulting in memory corruption.

>  
>  out:

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