Re: [PATCH v1 2/3] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

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

Pre-existing issues:
- [High] The patch adds a stack buffer scrub for `buf` in `ctr_paes_do_crypt()`, but misses the error paths, leaving sensitive cryptographic data exposed on the kernel stack.
- [High] Identical temporary stack buffer vulnerabilities exist in unprotected `aes_s390.c` functions (`ctr_aes_crypt` and `gcm_aes_crypt`), including raw AES key leaks on error paths.
--

--- Patch [2]: [PATCH v1 2/3] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm ---
commit d69919771b30f447650b0115c573c98b74a7ef91
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

This commit explicitly zeros out the buffer used for remaining bytes in
ctr_paes_do_crypt() to avoid exposing sensitive data. It also changes the
existing scrub in __xts_2keys_prep_param() to clean the entire parameter
block.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index ccc7da106c3a0..5a7dc46518854 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -1041,6 +1041,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  		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:

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this fix still leave
sensitive cryptographic data exposed on the kernel stack during error paths?

In ctr_paes_do_crypt(), trailing partial bytes are copied into the stack
allocated buf array. Inside the subsequent while loop, if cpacf_kmctr() fails
and !maysleep is true, or if paes_convert_key() fails, the code executes a
goto out:

		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;

Since the newly added memzero_explicit(buf, sizeof(buf)) is placed at the end
of the if (nbytes) block, the goto out jump bypasses the scrub entirely.
Would it be better to move the memzero_explicit() after the out label to
ensure the stack buffer is scrubbed even when returning an error?

[Severity: High]
This is a pre-existing issue, but there are similar temporary stack buffer
vulnerabilities in the unprotected aes_s390.c functions.

In ctr_aes_crypt(), a local buf array is used to process trailing partial
bytes but is never scrubbed before the function returns:

	if (nbytes) {
		memset(buf, 0, AES_BLOCK_SIZE);
		...
		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;

Additionally, in gcm_aes_crypt(), a param struct containing the raw AES key
(param.k) is constructed on the stack. During the scatterlist walk loop, if
either gcm_in_walk_done() or gcm_out_walk_done() fail, the function
immediately executes return -ENOMEM:

	if (gcm_in_walk_done(&gw_in, n) != n)
		return -ENOMEM;
	if (gcm_out_walk_done(&gw_out, n) != n)
		return -ENOMEM;

This early return bypasses the memzero_explicit(&param, sizeof(param))
cleanup at the end of the function, leaking the AES key on the stack. 
Could these similar cases in aes_s390.c be addressed as well?

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