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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The error code returned by `skcipher_walk_virt()` is unconditionally overwritten by the return value of `__ctr_aes_crypt()`, silently dropping errors.
--

--- Patch [1]: [PATCH 1/3] s390/crypto: Rework ctr_aes_crypt() to remove conditional locking ---
commit 83af1e74cacdeddf34088df594504204b40a3d99
Author: Heiko Carstens <[email protected]>

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

This commit removes conditional locking in ctr_aes_crypt() to address a Clang
thread-safety warning. The block processing loop is moved into a new helper
function __ctr_aes_crypt().

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 62edc66d54788..4b7e47423e892 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c

[ ... ]

> @@ -589,37 +604,53 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  {
>  	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
>  	struct s390_aes_ctx *sctx = crypto_skcipher_ctx(tfm);
> -	u8 buf[AES_BLOCK_SIZE], *ctrptr;
>  	struct skcipher_walk walk;
> -	unsigned int n, nbytes;
> -	int ret, locked;
> +	u8 buf[AES_BLOCK_SIZE];
> +	unsigned int nbytes;
> +	int ret;
>  
>  	if (unlikely(!sctx->fc))
>  		return fallback_skcipher_crypt(sctx, req, 0);
>  
> -	locked = mutex_trylock(&ctrblk_lock);
> -
>  	ret = skcipher_walk_virt(&walk, req, false);
> -	while ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE) {
> -		n = AES_BLOCK_SIZE;

[ ... ]

> -		crypto_inc(walk.iv, AES_BLOCK_SIZE);
> -		ret = skcipher_walk_done(&walk, nbytes - n);
> -	}
> -	if (locked)
> +
> +	if (mutex_trylock(&ctrblk_lock)) {
> +		ret = __ctr_aes_crypt(sctx, &walk, true);
>  		mutex_unlock(&ctrblk_lock);
> +	} else {
> +		ret = __ctr_aes_crypt(sctx, &walk, false);
> +	}

[Severity: High]
Does this code accidentally drop the error code from skcipher_walk_virt()?

If skcipher_walk_virt() fails and returns a negative error code (e.g. -ENOMEM
or -EINVAL), ret is immediately overwritten by the return value of
__ctr_aes_crypt().

Because walk.nbytes is 0 on error, __ctr_aes_crypt() will skip its loop
and return 0, converting the error into a success:

arch/s390/crypto/aes_s390.c:__ctr_aes_crypt() {
	int ret = 0;
	while ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE) {
		...
	}
	return ret;
}

Could this result in callers silently treating unencrypted data as a
successful cryptographic operation?

> +
> +	nbytes = walk.nbytes;
>  	/*
>  	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
>  	 */

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