Re: [PATCH v1 3/3] 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:
- [High] The patch fixes a mutex used in atomic context for PAES, but leaves the exact same buggy pattern unfixed in the standard AES implementation.
- [High] Cryptographic keys, plaintext, and ciphertext leak onto the kernel stack due to missing zeroization on error paths and tail-block processing.
--

--- Patch [3]: [PATCH v1 3/3] s390/crypto: Fix use of mutex in atomic context in PAES ---
commit 80f788f92a13c083509e6350703c357c6460863b
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 for the PAES CTR
implementation. This resolves an issue where the mutex was unsafe to use in
atomic or interrupt contexts.

Cc: [email protected] # 6.16+

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 5a7dc46518854..4f65469d35fcd 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[Severity: High]
This is a pre-existing issue, but does the standard AES implementation need
the same fix?

The commit message notes that the mutex is not safe to use in atomic or
interrupt contexts. Looking at ctr_aes_crypt() in
arch/s390/crypto/aes_s390.c, it continues to use mutex_trylock(&ctrblk_lock)
in contexts where it may be called from softirq. Does that leave standard
AES CTR broken in atomic environments in the exact same way?

[ ... ]

> @@ -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) {
>  		memset(buf, 0, AES_BLOCK_SIZE);
>  		memcpy(buf, walk->src.virt.addr, nbytes);
>  		while (1) {
>  			if (cpacf_kmctr(ctx->fc, param, buf,
>  					buf, AES_BLOCK_SIZE,
>  					walk->iv) == AES_BLOCK_SIZE)
>  				break;
>  			if (!maysleep) {
>  				rc = -EKEYEXPIRED;
>  				goto out;
>  			}
>  			rc = paes_convert_key(ctx, tested);
>  			if (rc)
>  				goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path bypass
the scrubbing of sensitive stack buffers?

In ctr_paes_do_crypt(), if paes_convert_key() fails in the final block
processing loop, the code jumps to the out label, completely bypassing
memzero_explicit(buf, sizeof(buf)). Can this leave cryptographic material
exposed on the kernel stack?

There appear to be similar unscrubbed buffers in arch/s390/crypto/aes_s390.c,
such as ctr_aes_crypt() missing a memset on return entirely, and
gcm_aes_crypt() returning -ENOMEM directly when a scatterlist mapping fails,
which bypasses the zeroization of the master AES key.

>  			spin_lock_bh(&ctx->pk_lock);
>  			memcpy(param->key, ctx->pk.protkey, sizeof(param->key));
>  			spin_unlock_bh(&ctx->pk_lock);
>  		}
>  		memcpy(walk->dst.virt.addr, buf, nbytes);
>  		crypto_inc(walk->iv, AES_BLOCK_SIZE);
>  		rc = skcipher_walk_done(walk, 0);
>  		memzero_explicit(buf, sizeof(buf));
>  	}
> 
> out:
>  	pr_debug("rc=%d\n", rc);
>  	return rc;
>  }

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