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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fails to scrub the temporary stack buffers `gw_in` and `gw_out` in `gcm_aes_crypt()`, leaving plaintext or ciphertext fragments on the kernel stack.
- [High] The `ctr_aes_crypt` function acquires a mutex (`ctrblk_lock`) using `mutex_trylock()`, which is illegal in atomic or softirq context.
--

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

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

This commit fixes missing scrubbing of temporary buffers in AES CTR and GCM
modes. It scrubs the buf array in ctr_aes_crypt() and uses an explicit error
path in gcm_aes_crypt() to ensure param is scrubbed before returning.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 366ce22d36237..976f6f7257d51 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)

[Severity: High]
This is a pre-existing issue, but is it safe to use mutex_trylock() and
mutex_unlock() in this function?

arch/s390/crypto/aes_s390.c:ctr_aes_crypt() {
    ...
	if (mutex_trylock(&ctrblk_lock)) {
		ret = __ctr_aes_crypt(sctx, &walk, true);
		mutex_unlock(&ctrblk_lock);
	} else {
    ...
}

The ctr(aes) skcipher algorithm can be invoked from softirq context, such as
via the IPsec network stack in NET_RX_SOFTIRQ.

Calling mutex_trylock() and mutex_unlock() from softirq context is generally
prohibited as it causes lock context violations. On PREEMPT_RT kernels, it
triggers WARN_ON_ONCE(!in_task()), and mutex_unlock() may acquire the
wait_lock raw spinlock without disabling softirqs, potentially creating
a deadlock.

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

[ ... ]

> @@ -895,10 +896,14 @@ static int gcm_aes_crypt(struct aead_request *req, unsigned int flags)
>  			  gw_in.ptr, aad_bytes);
>  
>  		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;
> +		if (gcm_in_walk_done(&gw_in, n) != n) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		if (gcm_out_walk_done(&gw_out, n) != n) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  		aadlen -= aad_bytes;
>  		pclen -= pc_bytes;
>  	} while (aadlen + pclen > 0);
> @@ -910,6 +915,7 @@ static int gcm_aes_crypt(struct aead_request *req, unsigned int flags)
>  	} else
>  		scatterwalk_map_and_copy(param.t, req->dst, len, taglen, 1);
>  
> +out:
>  	memzero_explicit(&param, sizeof(param));

[Severity: High]
This isn't a bug introduced by this patch, but since the goal is to prevent
exposure of sensitive data on the stack by scrubbing temporary buffers,
shouldn't gw_in and gw_out also be scrubbed?

The gcm_sg_walk structures contain a u8 buf[AES_BLOCK_SIZE] that is allocated
on the stack:

arch/s390/crypto/aes_s390.c:gcm_aes_crypt() {
    ...
    struct gcm_sg_walk gw_in, gw_out;
    ...
}

Any AEAD encryption/decryption with fragmented input scatterlists will use
gw_in.buf and gw_out.buf to bounce-buffer data, which will be left on the
kernel stack upon return.

>  	return ret;
>  }

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