Skip to contentSkip to conversation

Fixes and rework for paes_s390 and phmac_s390

PATCH Superseded Harald Freudenberger 3 participants 20 messages Aug 19 – Aug 20

Subsystems (3)
Crypto API s390 s390 crypto modules, prng driver, arch random

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Harald Freudenberger

author maintainer
#0 lore raw

v4 cover Fixes and rework for paes_s390 and phmac_s390

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

author maintainer
#1 lore raw

PATCH v4 1/7 s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

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 <[email protected]>
Reviewed-by Holger Dengler <[email protected]>
Cc [email protected] # 6.16+
---
 arch/s390/crypto/paes_s390.c | 38 +++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)
arch/s390/crypto/paes_s390.c +27 -11
index 8cfe6166c193..ccc7da106c3a 100644
@@ -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);
 	}
Signature · 1 line
2.43.0
AI review
#2 lore raw

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

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

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 <[email protected]>
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.

Quoted 4 lines
arch/s390/crypto/paes_s390.c +0 -0
index 8cfe6166c193c..ccc7da106c3a0 100644

[ ... ]

arch/s390/crypto/paes_s390.c static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier) · line 502

Quoted 7 lines
arch/s390/crypto/paes_s390.c +1 -1
@@ -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?

[ ... ]

arch/s390/crypto/paes_s390.c static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier) · line 770

Quoted 7 lines
arch/s390/crypto/paes_s390.c +1 -1
@@ -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?

[ ... ]

arch/s390/crypto/paes_s390.c static int ctr_paes_crypt(struct skcipher_request *req) · line 1093

Quoted 7 lines
arch/s390/crypto/paes_s390.c +1 -1
@@ -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?

[ ... ]

arch/s390/crypto/paes_s390.c static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod · line 1532

Quoted 7 lines
arch/s390/crypto/paes_s390.c +1 -1
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

PATCH v4 2/7 s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

In reply to 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 <[email protected]>
Reviewed-by Holger Dengler <[email protected]>
Cc [email protected] # 6.16+
---
 arch/s390/crypto/paes_s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/s390/crypto/paes_s390.c +2 -1
index ccc7da106c3a..d075b0241f1f 100644
@@ -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;
 }
Signature · 1 line
2.43.0
AI review
#4 lore raw

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

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

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 <[email protected]>
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 [email protected] # 6.16+

arch/s390/crypto/paes_s390.c static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx, · line 1047

Quoted 9 lines
arch/s390/crypto/paes_s390.c +1 -0
index ccc7da106c3a0..d075b0241f1f5 100644
@@ -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?

Quoted 3 lines
arch/s390/crypto/paes_s390.c +0 -0
 	pr_debug("rc=%d\n", rc);
 	return rc;
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author maintainer
#5 lore raw

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

In reply to 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 <[email protected]>
Reviewed-by Holger Dengler <[email protected]>
Cc [email protected] # 6.16+
---
 arch/s390/crypto/paes_s390.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
arch/s390/crypto/paes_s390.c +9 -8
index d075b0241f1f..e8bbfc376ebb 100644
@@ -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) {
Signature · 1 line
2.43.0
AI review
#6 lore raw

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

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

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 <[email protected]>

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.

Quoted 4 lines
arch/s390/crypto/paes_s390.c +0 -0
index d075b0241f1f5..e8bbfc376ebb1 100644

[ ... ]

arch/s390/crypto/paes_s390.c module_param_named(clrkey, pkey_clrkey_allowed, bool, 0444); · line 48

Quoted 6 lines
arch/s390/crypto/paes_s390.c +1 -1
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
author maintainer
#7 lore raw

PATCH v4 4/7 s390/crypto: Fix missing cra_flags in paes_s390

In reply to 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 <[email protected]>
Reviewed-by Holger Dengler <[email protected]>
Cc [email protected] # 6.17+
---
 arch/s390/crypto/paes_s390.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/s390/crypto/paes_s390.c +4 -0
index e8bbfc376ebb..03446bfac37f 100644
@@ -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,
Signature · 1 line
2.43.0
AI review
#8 lore raw

Re: [PATCH v4 4/7] s390/crypto: Fix missing cra_flags in paes_s390

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

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 <[email protected]>

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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
author maintainer
#9 lore raw

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

In reply to 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 <[email protected]>
Cc [email protected] # 6.16+
---
 arch/s390/crypto/paes_s390.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
arch/s390/crypto/paes_s390.c +24 -12
index 03446bfac37f..2048327c1ffe 100644
@@ -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;
Signature · 1 line
2.43.0
AI review
#10 lore raw

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

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

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 <[email protected]>

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.

Quoted 5 lines
arch/s390/crypto/paes_s390.c +0 -0
index 03446bfac37f2..2048327c1ffe4 100644

arch/s390/crypto/paes_s390.c static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier) · line 502

Quoted 9 lines
arch/s390/crypto/paes_s390.c +3 -1
@@ -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()?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
maintainer Reviewed-by on v4 5/7
#11 lore raw

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

In reply to Harald Freudenberger
Quoted 9 lines · 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 <[email protected]>
Cc [email protected] # 6.16+
Reviewed-by Holger Dengler <[email protected]>
Signature · 2 lines
Mit freundlichen Grüßen / Kind regards
Holger Dengler
author maintainer
#12 lore raw

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

In reply to 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 <[email protected]>
Cc [email protected] # 6.17+
---
 arch/s390/crypto/phmac_s390.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)
arch/s390/crypto/phmac_s390.c +21 -9
index 03ca33ffe6cc..30c8a59af1a4 100644
@@ -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;
Signature · 1 line
2.43.0
AI review
#13 lore raw

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

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

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 <[email protected]>

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.

Quoted 4 lines
arch/s390/crypto/phmac_s390.c +0 -0
index 03ca33ffe6ccf..30c8a59af1a4e 100644

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_update(struct ahash_request *req) · line 636

Quoted 7 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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()?

Quoted 12 lines
arch/s390/crypto/phmac_s390.c +4 -2
+		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));
 	}

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_final(struct ahash_request *req) · line 678

Quoted 7 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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?

Quoted 10 lines
arch/s390/crypto/phmac_s390.c +2 -1
+		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;

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_finup(struct ahash_request *req) · line 733

Quoted 10 lines
arch/s390/crypto/phmac_s390.c +3 -1
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
author maintainer
#14 lore raw

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

Quoted 53 lines · On 2026-08-19 11:37, [email protected] 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 <[email protected]>

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.

Quoted 4 lines
arch/s390/crypto/phmac_s390.c +0 -0
index 03ca33ffe6ccf..30c8a59af1a4e 100644

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_update(struct ahash_request · line 638

Quoted 9 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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.

Quoted 6 lines

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.

Quoted 85 lines
Quoted 12 lines
arch/s390/crypto/phmac_s390.c +4 -2
+		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));

}

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_final(struct ahash_request · line 680

Quoted 9 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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?

Quoted 10 lines
arch/s390/crypto/phmac_s390.c +2 -1
+		else
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
arch/s390/crypto/phmac_s390.c +0 -0
 	pr_debug("rc=%d\n", rc);
 	return rc;

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_finup(struct ahash_request · line 735

Quoted 12 lines
arch/s390/crypto/phmac_s390.c +3 -1
@@ -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?

author maintainer
#15 lore raw

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

In reply to [email protected]
Quoted 93 lines · On 2026-08-19 11:37, [email protected] 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 <[email protected]>

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.

Quoted 4 lines
arch/s390/crypto/phmac_s390.c +0 -0
index 03ca33ffe6ccf..30c8a59af1a4e 100644

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_update(struct ahash_request · line 638

Quoted 9 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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()?

Quoted 12 lines
arch/s390/crypto/phmac_s390.c +4 -2
+		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));

}

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_final(struct ahash_request · line 680

Quoted 9 lines
arch/s390/crypto/phmac_s390.c +2 -1
@@ -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:

Quoted 8 lines
    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.

Quoted 38 lines
Quoted 11 lines
arch/s390/crypto/phmac_s390.c +2 -1
+		else
 			atomic_dec(&tfm_ctx->via_engine_ctr);
 	}
 
 out:
-	if (rc != -EINPROGRESS)
+	if (cleanup)
 		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
arch/s390/crypto/phmac_s390.c +0 -0
 	pr_debug("rc=%d\n", rc);
 	return rc;

[ ... ]

arch/s390/crypto/phmac_s390.c static int phmac_finup(struct ahash_request · line 735

Quoted 12 lines
arch/s390/crypto/phmac_s390.c +3 -1
@@ -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;
Quoted 5 lines

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.

maintainer Reviewed-by on v4 6/7
#16 lore raw

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

In reply to Harald Freudenberger
Quoted 9 lines · 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 <[email protected]>
Cc [email protected] # 6.17+
Reviewed-by Holger Dengler <[email protected]>
Signature · 2 lines
Mit freundlichen Grüßen / Kind regards
Holger Dengler
author maintainer
#17 lore raw

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

In reply to 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 <[email protected]>
Cc [email protected] # 6.16+
---
 arch/s390/crypto/paes_s390.c  | 8 ++++----
 arch/s390/crypto/phmac_s390.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)
arch/s390/crypto/paes_s390.c +4 -4
index 2048327c1ffe..0bc360c7ef15 100644
@@ -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 = {
arch/s390/crypto/phmac_s390.c +1 -1
index 30c8a59af1a4..89c5fa3dced2 100644
@@ -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)						\
Signature · 1 line
2.43.0
AI review
#18 lore raw

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

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

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.

Quoted 6 lines

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 <[email protected]>

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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
maintainer Reviewed-by on v4 7/7
#19 lore raw

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

In reply to Harald Freudenberger
Quoted 9 lines · 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.

Quoted 7 lines

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 <[email protected]>
Cc [email protected] # 6.16+
Reviewed-by Holger Dengler <[email protected]>
Signature · 2 lines
Mit freundlichen Grüßen / Kind regards
Holger Dengler