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

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

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: [email protected] # 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/[email protected]?part=1