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

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

Pre-existing issues:
- [High] The unprotected AES implementation (`aes_s390.c`) suffers from the same missing stack buffer scrub issue that this patch fixes in `paes_s390.c`, leading to potential leakage of plaintext, ciphertext, and AES-CTR keystream bytes.
--

--- Patch [2]: [PATCH v3 2/6] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm ---
commit 35a0f4315bfc23077f4581853ff227c37d38d817
Author: Harald Freudenberger <[email protected]>

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

In function ctr_paes_do_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. Rework the code to
explicitly scrub the buffer at the end of the function to avoid
exposure of maybe sensitive data.

In function __xts_2keys_prep_param() change the existing scrub to
clean the whole param block instead of just the key field.

Cc: [email protected] # 6.16+

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index ccc7da106c3a0..d075b0241f1f5 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -1044,6 +1044,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  	}
>  
>  out:
> +	memzero_explicit(buf, sizeof(buf));

[Severity: High]
This isn't a bug introduced by this patch, but does the unprotected aes
implementation in arch/s390/crypto/aes_s390.c suffer from the same missing
stack buffer scrub issue?

Looking at ctr_aes_crypt() and gcm_aes_crypt() in aes_s390.c, there are stack
buffers used for partial blocks that appear to be left unscrubbed:

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;
}

arch/s390/crypto/aes_s390.c:gcm_aes_crypt() {
    ...
	memzero_explicit(&param, sizeof(param));
	return ret;
}

The 16-byte stack buffer buf in ctr_aes_crypt() holds plaintext, ciphertext,
and aes keystream bytes from the zero-padding xored with the ctr keystream.
Similarly, the gw_in and gw_out stack structures in gcm_aes_crypt() contain
16-byte buf arrays that buffer partial plaintext and ciphertext blocks.

Since any userspace process (via af_alg) utilizing s390 hardware aes-ctr
for partial block lengths, or aes-gcm for unaligned data, could trigger
this, could this lead to a potential information leak of sensitive data?

Since this patch explicitly addresses this regression in the sibling
protected aes driver (paes_s390.c), should the unprotected aes driver be
patched as well?

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

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