Re: [PATCH v2 2/2] s390/crypto: Explicit scrub temp buffer in AES ctr mode algorithm

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

Pre-existing issues:
- [Critical] Missing zeroization of the highly sensitive AES master key (`param` structure) on early error paths in `gcm_aes_crypt()`.
- [High] The `ctr_paes_do_crypt()` function in Protected AES (PAES) CTR mode suffers from the exact same stack buffer leak fixed in `ctr_aes_crypt()`, exposing plaintext or ciphertext on the kernel stack.
- [High] The `gcm_aes_crypt()` function leaves temporary scatter/gather walk buffers (`gw_in` and `gw_out`) and authentication tags unscrubbed on the kernel stack, exposing sensitive plaintext and ciphertext data.
- [High] The PowerPC `p10_aes_gcm_crypt()` glue code uses an unscrubbed stack buffer to process partial AES-GCM blocks.
- [High] Invalid use of `mutex_trylock()` and `mutex_unlock()` in potentially atomic/softirq context in `ctr_aes_crypt()` and `ctr_paes_do_crypt()`.
--

--- Patch [2]: [PATCH v2 2/2] s390/crypto: Explicit scrub temp buffer in AES ctr mode algorithm ---
commit e6f2a17e1141661b93e968958338ce9c79ff6466
Author: Harald Freudenberger <[email protected]>

    s390/crypto: Explicit scrub temp buffer in AES ctr mode algorithm
    
    In function ctr_aes_crypt() there is a buffer used to process
    remaining bytes < AES_BLOCK_SIZE. This buffer was not scrubbed and
    thus could lead to expose of unwanted data. Explicitly scrub it at
    the end of the code block.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 366ce22d36237..f52c612ae9cb3 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -604,6 +604,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
>  		memcpy(walk.dst.virt.addr, buf, nbytes);
>  		crypto_inc(walk.iv, AES_BLOCK_SIZE);
>  		ret = skcipher_walk_done(&walk, 0);
> +		memzero_explicit(buf, sizeof(buf));

[Severity: High]
This isn't a bug introduced by this patch, but since ctr_aes_crypt() is a
synchronous skcipher that can be called from softirq context, is it safe to
use mutex_trylock() and mutex_unlock() here?

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

Holding or releasing a sleepable lock like a mutex from a software interrupt
context typically results in lockdep splats. The identical pattern also
exists in arch/s390/crypto/paes_s390.c in ctr_paes_do_crypt().

[Severity: High]
This is a pre-existing issue, but arch/s390/crypto/paes_s390.c seems to have
the exact same stack buffer leak for the protected aes ctr mode. Could this
also expose unwanted data?

arch/s390/crypto/paes_s390.c:ctr_paes_do_crypt() {
    ...
	if (nbytes) {
		memset(buf, 0, AES_BLOCK_SIZE);
		memcpy(buf, walk->src.virt.addr, nbytes);
        ...
		memcpy(walk->dst.virt.addr, buf, nbytes);
		crypto_inc(walk->iv, AES_BLOCK_SIZE);
		rc = skcipher_walk_done(walk, 0);
        // missing memzero_explicit(buf, sizeof(buf));
	}
    ...
}

This temporary stack buffer buf[] appears to be populated with cryptographic
data but never explicitly zeroized before returning.

[Severity: High]
This is a pre-existing issue, but could gcm_aes_crypt() in the same file
also be exposing sensitive plaintext and ciphertext data on the kernel stack?

arch/s390/crypto/aes_s390.c:gcm_aes_crypt() {
	struct gcm_sg_walk gw_in, gw_out;
	u8 tag[GHASH_DIGEST_SIZE];
    ...
}

The gw_in and gw_out structures contain u8 buf[AES_BLOCK_SIZE] bounce
buffers used during scatterlist walks for unaligned chunks. These walk
buffers, along with the authentication tag, are omitted from the
memzero_explicit() block at the end of the function.

[Severity: Critical]
This is a pre-existing issue, but does an early return in gcm_aes_crypt()
bypass the explicit zeroization of the param structure containing the
master key?

arch/s390/crypto/aes_s390.c:gcm_aes_crypt() {
    ...
		n = aad_bytes + pc_bytes;
		if (gcm_in_walk_done(&gw_in, n) != n)
			return -ENOMEM;
		if (gcm_out_walk_done(&gw_out, n) != n)
			return -ENOMEM;
    ...
	memzero_explicit(&param, sizeof(param));
	return ret;
}

If gcm_in_walk_done() or gcm_out_walk_done() fail, the function early
returns -ENOMEM and skips zeroizing the param structure on the stack.

[Severity: High]
This is a pre-existing issue, but I am wondering if the powerpc aes-gcm
stitched implementation has a similar partial block leak?

arch/powerpc/crypto/aes-gcm-p10-glue.c:p10_aes_gcm_crypt() {
    ...
	while ((nbytes = walk.nbytes) > 0 && ret == 0) {
		const u8 *src = walk.src.virt.addr;
		u8 *dst = walk.dst.virt.addr;
		u8 buf[AES_BLOCK_SIZE];

		if (unlikely(nbytes > 0 && nbytes < AES_BLOCK_SIZE))
			src = dst = memcpy(buf, src, nbytes);
    ...
}

The local buf array holds partial blocks but is never zeroized
before the loop continues or the function returns, leaving data on the stack.

>  	}
>  
>  	return ret;

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