Fixes and rework for aes_s390

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

Pre-existing issues:
- [High] Using a mutex (ctrblk_lock) in ctr_aes_crypt, which can be executed in interrupt/softirq context.
--

--- Patch [1]: [PATCH v4 1/3] s390/crypto: Fix skcipher_walk return code handling in aes_s390 ---
commit 17298a26fe3f9b4cf4ce956d4ad5d1b5f20ca7ef
Author: Harald Freudenberger <[email protected]> s390/crypto: Fix skcipher_walk return code handling in aes_s390

This patch adds proper return code checking from skcipher_walk_virt() in the processing loops of several symmetric cipher modes to prevent operating on an uninitialized walk structure if the initialization fails.

Quoted 4 lines

arch/s390/crypto/aes_s390.c +0 -0

index 62edc66d54788..366ce22d36237 100644

[ ... ]

arch/s390/crypto/aes_s390.c static int ctr_aes_crypt(struct skcipher_request · line 578

Quoted 3 lines

arch/s390/crypto/aes_s390.c +0 -0

@@ -577,7 +577,7 @@ static int ctr_aes_crypt(struct skcipher_request 
 *req)
 	locked = mutex_trylock(&ctrblk_lock);

[Severity: High]
This isn't a bug introduced by this patch, but can using a mutex here corrupt the interrupted task's lock state?

Since ctr_aes_crypt() can be executed in softirq context (such as during IPsec ESP receive processing via NET_RX_SOFTIRQ), calling mutex_trylock() and later mutex_unlock() assigns ownership to the arbitrarily interrupted task. Mutexes enforce strict task ownership and are explicitly forbidden in interrupt context, even when using trylock.

Could this trigger lockdep splats, incorrect optimistic spinning behavior, or hard panics on PREEMPT_RT kernels where mutexes are strictly task-bound?

Quoted 1 line

arch/s390/crypto/aes_s390.c static int ctr_aes_crypt(struct skcipher_request *req) · line 583

Quoted 6 lines

arch/s390/crypto/aes_s390.c +1 -1

 	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)

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 <[email protected]>

Cc [email protected] # 5.5+

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+

Quoted 4 lines

arch/s390/crypto/aes_s390.c +0 -0

index 62edc66d54788..10561aa687c78 100644

[ ... ]

Quoted 18 lines

arch/s390/crypto/aes_s390.c +1 -1

@@ -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);

arch/s390/crypto/aes_s390.c +1 -0

 		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));
 	}
 

Quoted 1 line

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?

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.

Fixes d07f951903fa ("crypto: s390/aes - Fix buffer overread in CTR mode")

Signed-off-by Harald Freudenberger <[email protected]>

Cc [email protected] # 6.8+

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

Pre-existing issues:
- [High] The patch completely misses the exact same preexisting invalid mutex usage in atomic context in the Protected AES (PAES) CTR implementation (paes_s390.c).
--

--- Patch [3]: [PATCH v4 3/3] s390/crypto: Fix use of mutex in atomic context ---
commit 709721b3784d3a0026753ef3b28100620270373d
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix use of mutex in atomic context

This commit replaces a mutex with a binary semaphore in the s390 AES CTR implementation. This prevents unsafe locking when the crypto routines are called from atomic or interrupt contexts.

Quoted 4 lines

arch/s390/crypto/aes_s390.c +0 -0

index 10561aa687c78..bd9815988c813 100644

[ ... ]

Quoted 4 lines

arch/s390/crypto/aes_s390.c +0 -0

@@ -569,12 +569,13 @@ static int ctr_aes_crypt(struct skcipher_request 
 *req)
 	u8 buf[AES_BLOCK_SIZE], *ctrptr;
 	struct skcipher_walk walk;

arch/s390/crypto/aes_s390.c static int ctr_aes_crypt(struct skcipher_request · line 579

Quoted 10 lines

arch/s390/crypto/aes_s390.c +3 -2

 	unsigned int n, nbytes;
-	int ret, locked;
+	bool locked;
+	int ret;
 
 	if (unlikely(!sctx->fc))
 		return fallback_skcipher_crypt(sctx, req, 0);
 
-	locked = mutex_trylock(&ctrblk_lock);
+	locked = down_trylock(&ctrblk_sem) == 0;

[Severity: High]
This is a pre-existing issue, but it appears the exact same pattern exists in the Protected AES (PAES) CTR implementation which wasn't updated here.

Looking at ctr_paes_do_crypt() in arch/s390/crypto/paes_s390.c:

	locked = mutex_trylock(&ctrblk_lock);

Since the skcipher API can be invoked from atomic or interrupt contexts (like network softirqs), does the PAES implementation also risk an atomic context locking violation? Should arch/s390/crypto/paes_s390.c receive a similar conversion to a binary semaphore?

The AES CTR implementation used a mutex to lock one page of exclusive memory for fast CTR processing. Unfortunately a mutex is not save to use in atomic or interrupt context. So use a binary semaphore instead which is save to use in such environments.

Fixes 7988fb2c03c8 ("crypto: s390/aes - convert to skcipher API")

Suggested-by Heiko Carstens <[email protected]>

Signed-off-by Harald Freudenberger <[email protected]>

Cc [email protected] # 5.5+