Re: [PATCH v3 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390

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

Pre-existing issues:
- [High] Calling `mutex_trylock()` from softirq/interrupt context is unsafe and violates core kernel locking rules.
- [High] Stack buffer `buf` containing sensitive plaintext/ciphertext is not securely erased before returning from `ctr_aes_crypt`.
--

--- Patch [1]: [PATCH v3 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390 ---
commit 26c79c6f9fbbbd190ba8f14ce8cd5587ce6bbe31
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix skcipher_walk return code handling in aes_s390

The return codes from skcipher_walk_virt() were not properly checked
before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
If skcipher_walk_virt() fails, the walk structure may be in an undefined
state, and attempting to process data could lead to incorrect behavior
or accessing uninitialized memory.

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

[ ... ]

> @@ -577,7 +577,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  	locked = mutex_trylock(&ctrblk_lock);

[Severity: High]
This is a pre-existing issue, but calling mutex_trylock() from softirq or
interrupt context is unsafe and violates core kernel locking rules.

Because this crypto algorithm can be executed in softirq context (such as
when processing IPsec/ESP packets), the current task is arbitrary, which
will result in a lockdep splat.

Could this be fixed by checking if the execution context permits operating
on a sleepable lock, perhaps using in_task(), before attempting the trylock?

>  
>  	ret = skcipher_walk_virt(&walk, req, false);

[ ... ]

> @@ -596,7 +596,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  	/*
>  	 * final block may be < AES_BLOCK_SIZE, copy only nbytes
>  	 */
> -	if (nbytes) {
> +	if (!ret && nbytes) {
>  		memset(buf, 0, AES_BLOCK_SIZE);
>  		memcpy(buf, walk.src.virt.addr, nbytes);
>  		cpacf_kmctr(sctx->fc, sctx->key, buf, buf,

[Severity: High]
This is a pre-existing issue, but this function appears to return without
securely erasing the stack buffer buf.

When a partial block is processed, up to 15 bytes of sensitive cryptographic
material (plaintext or decrypted ciphertext) is left in this buffer. Could
this stack remnant be leaked to userspace by subsequent syscalls?

Other functions in this driver (e.g., cbc_aes_crypt) use memzero_explicit()
before returning. Should a similar cleanup be added here?

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