Re: [PATCH v3 3/6] s390/crypto: Fix use of mutex in atomic context in PAES

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

Pre-existing issues:
- [Critical] Temporary stack buffer `buf` containing plaintext and keystream is not zeroed before returning from `ctr_aes_crypt`.
- [High] The identical `mutex_trylock()` in atomic context bug remains unfixed in `arch/s390/crypto/aes_s390.c` for standard AES CTR mode.
--

--- Patch [3]: [PATCH v3 3/6] s390/crypto: Fix use of mutex in atomic context in PAES ---
commit a7d845af838256909450e5168e0a9dc3dda32e5a
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix use of mutex in atomic context in PAES

This commit replaces a mutex with a binary semaphore in the PAES CTR
implementation to allow fast CTR processing. This change avoids issues
when operating in atomic or interrupt contexts where mutexes are unsafe.

Cc: [email protected] # 6.16+

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index d075b0241f1f5..e8bbfc376ebb1 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[ ... ]

> @@ -971,7 +972,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  	if (rc)
>  		goto out;
>  
> -	locked = mutex_trylock(&ctrblk_lock);
> +	locked = down_trylock(&ctrblk_sem) == 0;

[Severity: High]
This is a pre-existing issue, but does arch/s390/crypto/aes_s390.c also
need this exact same fix?

It looks like ctr_aes_crypt() in arch/s390/crypto/aes_s390.c still uses
mutex_trylock() in atomic context:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
    locked = mutex_trylock(&ctrblk_lock);
    ...
}

Since ctr_aes_crypt() implements standard AES CTR as part of the skcipher
API, it is frequently reached via network processing from softirq context.
Could this trigger lockdep splats or fail on RT kernels if not updated to
use a binary semaphore?

[ ... ]

> @@ -1016,7 +1017,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  		}
>  	}
>  	if (locked)
> -		mutex_unlock(&ctrblk_lock);
> +		up(&ctrblk_sem);
>  
>  	/* final block may be < AES_BLOCK_SIZE, copy only nbytes */
>  	if (nbytes) {

[Severity: Critical]
This isn't a bug introduced by this patch, but while looking at the stack
buffer handling here, does arch/s390/crypto/aes_s390.c need to clear its
temporary buffer before returning?

In ctr_aes_crypt(), there is a temporary stack buffer buf that isn't
zeroed before the function returns:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
    if (nbytes) {
        ...
        ret = skcipher_walk_done(&walk, 0);
    }

    return ret;
}

During partial block processing, buf receives decrypted plaintext and the
remainder contains the raw AES CTR keystream. Since it isn't cleared explicitly,
could this leave cryptographic material on the kernel stack and expose
sensitive data to other components via uninitialized stack exposure vectors?

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