Re: [PATCH v4 2/3] s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm

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

Pre-existing issues:
- [Medium] Missing scrub of sensitive temporary buffer in `ctr_paes_do_crypt()` when handling remaining bytes < AES_BLOCK_SIZE.
--

--- Patch [2]: [PATCH v4 2/3] s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm ---
commit 3d2b9adbb3df3417589c4d42aa1add6ac98dbf33
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm 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. When the buffer is used
explicitly scrub it at the end of the code block to avoid exposure of
maybe sensitive data.

In a similar way the function gcm_aes_crypt() hat an error path where
the CPACF param block was not scrubbed. Instead of return early now
these error paths go to end of function where explicit scrubbing is
done. Similar with the buffers which are part of the gcm_sg_walk
structs from the variables gw_in and gw_out.

Cc: [email protected] # 6.8+

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

[ ... ]

> @@ -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,
> @@ -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));
>  	}
>  
>  	return ret;

[Severity: Medium]
This is a pre-existing issue, but while you are patching ctr_aes_crypt(),
could the same missing scrub vulnerability be present in the PAES equivalent?

Looking at ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c, when the
remaining bytes are less than AES_BLOCK_SIZE, it uses a local buf array in an
almost identical manner but never explicitly scrubs it before returning:

arch/s390/crypto/paes_s390.c:ctr_paes_do_crypt() {
...
	/* 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;
...
		}
		memcpy(walk->dst.virt.addr, buf, nbytes);
		crypto_inc(walk->iv, AES_BLOCK_SIZE);
		rc = skcipher_walk_done(walk, 0);
	}

out:
	pr_debug("rc=%d\n", rc);
	return rc;
}

Can this leak plaintext or ciphertext to the kernel stack where it might be
subsequently exposed to user space via uninitialized stack padding?

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