Re: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking

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

Pre-existing issues:
- [High] Silent data corruption when skcipher_walk_done() mapping errors are overwritten by paes_convert_key() false success.
--

--- Patch [2]: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking ---
commit fd135aacfa5dd1086ab5711e4513f6e0edede1dc
Author: Heiko Carstens <[email protected]>

s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking

This commit extracts the core loop of ctr_paes_do_crypt() into a new
helper function, __ctr_paes_do_crypt(), to avoid conditionally taken locks
that trip up Clang's static analysis.

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

[ ... ]

> @@ -981,34 +955,76 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  		k = cpacf_kmctr(ctx->fc, param, walk->dst.virt.addr,
>  				walk->src.virt.addr, n, ctrptr);
>  		if (k) {
> -			if (ctrptr == ctrblk)
> +			if (ctrptr == ctrblk) {
>  				memcpy(walk->iv, ctrptr + k - AES_BLOCK_SIZE,
>  				       AES_BLOCK_SIZE);
> +			}
>  			crypto_inc(walk->iv, AES_BLOCK_SIZE);
>  			rc = skcipher_walk_done(walk, nbytes - k);
>  		}
>  		if (k < n) {
>  			if (!maysleep) {
> -				if (locked)
> -					mutex_unlock(&ctrblk_lock);
>  				rc = -EKEYEXPIRED;
>  				goto out;
>  			}
>  			rc = paes_convert_key(ctx, tested);

[Severity: High]
This is a pre-existing issue, but does this sequence overwrite an error
from skcipher_walk_done()?

If the hardware stops processing early (k < n) and skcipher_walk_done() fails
to map the next scatterlist chunk, it can return a negative error code
like -ENOMEM into rc.

However, execution then immediately enters the if (k < n) block where rc is
unconditionally overwritten. If !maysleep, rc becomes -EKEYEXPIRED. If
paes_convert_key() is called and succeeds, rc becomes 0.

Could this result in silent data corruption where an incomplete encryption is
treated as a success (rc = 0) by the caller?

> -			if (rc) {
> -				if (locked)
> -					mutex_unlock(&ctrblk_lock);
> +			if (rc)
>  				goto out;
> -			}
>  			spin_lock_bh(&ctx->pk_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2