[PATCH v1 0/1] PAES - Fix rc handling at skcipher_walk_done

[PATCH v1 0/1] PAES - Fix rc handling at skcipher_walk_done

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

Changelog:

v1: initial patch

Harald Freudenberger (1):
  s390/crypto: Fix return code handling at skcipher_walk_done in PAES
    algorithms

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

--
2.43.0

[PATCH v1 1/1] 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.

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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index 973436592318..7e5cc39d4bda 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;
@@ -690,6 +693,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) {
@@ -968,6 +973,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) {
@@ -1283,8 +1293,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;
@@ -1377,8 +1390,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;
-- 
2.43.0

Re: [PATCH v1 1/1] 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 patch introduces a guaranteed double free of skcipher_walk resources when skcipher_walk_done() returns an error, due to redundant cleanup in the subsystem's wrapper functions.
- [High] The local stack buffer `buf` is not cleared before returning, potentially leaking plaintext or AES-CTR keystream bytes to the kernel stack.
--

--- Patch [1]: [PATCH v1 1/1] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit cbb88540726dcff1286760a1df99934f56760f60
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 the
PAES cipher processing loops (ECB, CBC, CTR, XTS modes). It ensures walk
errors are properly propagated and not masked by subsequent operations.

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

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -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;
> +		}

[Severity: Critical]
This isn't a bug introduced by this patch, but does returning this error
directly to the caller result in a double free?

Looking at the wrapper function ecb_paes_crypt (and similar wrappers in this
file), if skcipher_walk_done fails and we jump to out, the wrapper will see
the error and unconditionally call skcipher_walk_done again:

ecb_paes_crypt() {
    ...
    rc = ecb_paes_do_crypt(ctx, req_ctx, false, maysleep);
    ...
    if (rc != -EINPROGRESS)
        skcipher_walk_done(walk, rc);
}

Because skcipher_walk_done frees walk->buffer and walk->page on failure
but does not set them to NULL, won't this second call attempt to free the
same dangling pointers again?

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does this function leak plaintext or
keystream bytes to the kernel stack?

At the end of ctr_paes_do_crypt, the final partial block is handled using
a local stack buffer buf:

ctr_paes_do_crypt() {
    ...
    if (nbytes) {
        memset(buf, 0, AES_BLOCK_SIZE);
        memcpy(buf, walk->src.virt.addr, nbytes);
        ...
        memcpy(walk->dst.virt.addr, buf, nbytes);
        crypto_inc(walk->iv, AES_BLOCK_SIZE);
        rc = skcipher_walk_done(walk, 0);
    }

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

Because buf is abandoned on the stack without being cleared via
memzero_explicit, could subsequent unrelated syscalls leak these 
leftover bytes to user space?

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