[PATCH v2 0/2] Fix skcipher_walk return code handling in aes_s390

[PATCH v2 0/2] Fix skcipher_walk return code handling in aes_s390

[Harald Freudenberger]

The return codes from skcipher_walk_virt() were not properly checked
before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
If skcipher_walk_virt() fails, the walk structure may be in an undefined
state, and attempting to process data could lead to incorrect behavior
or accessing uninitialized memory.

Add proper return code checking to ensure correct handling of the walk
initialization and walk advance and eventually return to the caller
with that return code.

Patch #2 deals with a not scrubbed temp buffer.

Changelog:

v1: initial version
v2: Fix missing scrub on a temp buffer used to process left over bytes
    in CTR mode.

Harald Freudenberger (2):
  s390/crypto: Fix skcipher_walk return code handling in aes_s390
  s390/crypto: Explicit scrub temp buffer in AES ctr mode algorithm

 arch/s390/crypto/aes_s390.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

--
2.43.0

[PATCH v2 1/2] s390/crypto: Fix skcipher_walk return code handling in aes_s390

[Harald Freudenberger]

The return codes from skcipher_walk_virt() were not properly checked
before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
If skcipher_walk_virt() fails, the walk structure may be in an undefined
state, and attempting to process data could lead to incorrect behavior
or accessing uninitialized memory.

Add proper return code checking to ensure correct handling of the walk
initialization and walk advance and eventually return to the caller
with that return code.

Fixes: 7988fb2c03c8 ("crypto: s390/aes - convert to skcipher API")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 5.5+
---
 arch/s390/crypto/aes_s390.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 62edc66d5478..366ce22d3623 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -129,7 +129,7 @@ static int ecb_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 		return fallback_skcipher_crypt(sctx, req, modifier);
 
 	ret = skcipher_walk_virt(&walk, req, false);
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(sctx->fc | modifier, sctx->key,
@@ -233,7 +233,7 @@ static int cbc_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 		return ret;
 	memcpy(param.iv, walk.iv, AES_BLOCK_SIZE);
 	memcpy(param.key, sctx->key, sctx->key_len);
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_kmc(sctx->fc | modifier, &param,
@@ -359,7 +359,7 @@ static int xts_aes_crypt(struct skcipher_request *req, unsigned long modifier)
 	memcpy(xts_param.key + offset, xts_ctx->key, xts_ctx->key_len);
 	memcpy(xts_param.init, pcc_param.xts, 16);
 
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(xts_ctx->fc | modifier, xts_param.key + offset,
@@ -487,7 +487,7 @@ static int fullxts_aes_crypt(struct skcipher_request *req,  unsigned long modifi
 	memcpy(fxts_param.tweak, req->iv, AES_BLOCK_SIZE);
 	fxts_param.nap[0] = 0x01; /* initial alpha power (1, little-endian) */
 
-	while ((nbytes = walk.nbytes) != 0) {
+	while (!ret && ((nbytes = walk.nbytes) != 0)) {
 		/* only use complete blocks */
 		n = nbytes & ~(AES_BLOCK_SIZE - 1);
 		cpacf_km(xts_ctx->fc | modifier, fxts_param.key + offset,
@@ -577,7 +577,7 @@ static int ctr_aes_crypt(struct skcipher_request *req)
 	locked = mutex_trylock(&ctrblk_lock);
 
 	ret = skcipher_walk_virt(&walk, req, false);
-	while ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE) {
+	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
 		n = AES_BLOCK_SIZE;
 
 		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
@@ -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,
-- 
2.43.0

Re: [PATCH v2 1/2] s390/crypto: Fix skcipher_walk return code handling in aes_s390

[sashiko-bot]

> The return codes from skcipher_walk_virt() were not properly checked
> before entering the processing loops in ecb_aes_crypt() and ctr_aes_crypt().
> If skcipher_walk_virt() fails, the walk structure may be in an undefined
> state, and attempting to process data could lead to incorrect behavior
> or accessing uninitialized memory.
> 
> Add proper return code checking to ensure correct handling of the walk
> initialization and walk advance and eventually return to the caller
> with that return code.
> 
> Fixes: 7988fb2c03c8 ("crypto: s390/aes - convert to skcipher API")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806084950.17679-1-freude@linux.ibm.com?part=1

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

[Harald Freudenberger]

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.

Fixes: d07f951903fa ("crypto: s390/aes - Fix buffer overread in CTR mode")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.8+
---
 arch/s390/crypto/aes_s390.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 366ce22d3623..f52c612ae9cb 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));
 	}
 
 	return ret;
-- 
2.43.0

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

[sashiko-bot]

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 <freude@linux.ibm.com>

    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/20260806084950.17679-1-freude@linux.ibm.com?part=2