Re: [PATCH v3 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:
- [High] Missing explicit scrubbing of stack buffer `buf` containing sensitive cryptographic data in `ctr_paes_do_crypt()`.
--

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

s390/crypto: Fix missing scrub of temp buffers with AES ctr and gcm algorithm

This commit fixes missing scrubs of temporary stack buffers in the s390
AES CTR and GCM implementations. It adds memzero_explicit() calls to ensure
sensitive cryptographic data is not left on the stack after partial block
processing or error conditions.

Cc: [email protected] # 6.8+

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 366ce22d36237..10561aa687c78 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 should the same fix be applied
to ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c?

It appears that the protected key implementation handles the final block for
CTR mode using an identical approach, but the stack buffer is left unscrubbed:

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(...) == 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:
        ...
        return rc;
}

Could this lead to similar exposure of decrypted plaintext or encrypted
ciphertext on the kernel stack?

>  	}
>  
>  	return ret;

[ ... ]

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