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

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

Pre-existing issues:
- [High] The `aes_s390.c` module contains the exact same locking bug in atomic context that this patch fixes in `paes_s390.c`. The `ctr_aes_crypt()` function uses `mutex_trylock(&ctrblk_lock)` and subsequently calls `mutex_unlock(&ctrblk_lock)`. As the patch's commit message notes, mutexes are not safe to use in atomic or interrupt context. Synchronous skcipher algorithms like `ctr-aes-s390` can be invoked directly from `softirq` context (e.g., by the IPsec stack). Calling `mutex_unlock()` in `softirq` is a strict violation of kernel locking rules.
--

--- Patch [3]: [PATCH v6 3/9] s390/crypto: Fix use of mutex in atomic context in PAES ---
commit 425e8f1d3532a176433992d286f917e0fd14ff9e
Author: Harald Freudenberger <[email protected]>

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

This commit fixes a locking issue in the PAES CTR implementation where a
mutex was used in atomic or interrupt context. It replaces the mutex with
a binary semaphore which is safe to use in such environments.

> 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
[ ... ]
> @@ -45,7 +45,7 @@ module_param_named(clrkey, pkey_clrkey_allowed, bool, 0444);
>  MODULE_PARM_DESC(clrkey, "Allow clear key material (default N)");
>  
>  static u8 *ctrblk;
> -static DEFINE_MUTEX(ctrblk_lock);
> +static DEFINE_SEMAPHORE(ctrblk_sem, 1);

[Severity: High]
This isn't a bug introduced by this patch, but arch/s390/crypto/aes_s390.c
appears to have the exact same bug that this patch is fixing here.

Looking at ctr_aes_crypt() in aes_s390.c:

	locked = mutex_trylock(&ctrblk_lock);

	ret = skcipher_walk_virt(&walk, req, false);
	while ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE) {
		...
	}
	if (locked)
		mutex_unlock(&ctrblk_lock);

Since synchronous skcipher algorithms like ctr-aes-s390 can be invoked from
softirq context (such as by the IPsec stack), would calling mutex_unlock()
here trigger a lockdep splat or crash, especially on PREEMPT_RT where
mutex_unlock() must be called from process context?

Does aes_s390.c also need to be converted to use a binary semaphore?

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