Re: [PATCH v9 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks

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

New issues:
- [High] Uninitialized memory read due to incorrect bounds check when copying subfunction code.
- [Low] Redundant and mathematically impossible integer overflow checks on 64-bit sum calculations.

Pre-existing issues:
- [High] Missing minimum size validation for EP11 CPRB messages allows reading uninitialized memory.
--

--- Patch [2]: [PATCH v9 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks ---
commit 93cf9dfadb28a27fc37f0252a7721661ccec467d
Author: Harald Freudenberger <[email protected]>
s390/zcrypt: Improve CCA CPRB length and overflow checks

This commit addresses integer overflow and missing bounds checks in the
xcrb_msg_to_type6cprb_msgx function during CCA CPRB processing. It introduces
64-bit size_t variables for length arithmetic and explicitly bounds the
calculated lengths to U32_MAX. Minimum control block sizes are also validated
prior to copying data from userspace.

> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c

[ ... ]

> @@ -342,49 +342,40 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
> +	req_sumlen = req_cblen + xcrb->request_data_length;
> +	if (req_sumlen > U32_MAX)
> +		return -EINVAL;
> +	if (req_cblen <= xcrb->request_data_length ?
>  	    req_sumlen < xcrb->request_data_length :
> -	    req_sumlen < CEIL4(xcrb->request_control_blk_length)) {
> +	    req_sumlen < req_cblen) {
>  		return -EINVAL;
>  	}

[Severity: Low]
Does this ternary check provide any value now that the lengths are using
64-bit size_t?

Since req_cblen and req_sumlen are bounded to U32_MAX, their sum cannot
overflow a 64-bit variable, making this condition impossible to reach.

[ ... ]

> @@ -404,8 +395,8 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
>  			     xcrb->request_control_blk_addr,
>  			     xcrb->request_control_blk_length))
>  		return -EFAULT;
> -	if (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
> -	    xcrb->request_control_blk_length)
> +	/* copy subfunction code into AP msg type 6 function code field */
> +	if (msg->cprbx.cprb_len > req_cblen - sizeof(msg->hdr.function_code))
>  		return -EINVAL;
>  	function_code = ((unsigned char *)&msg->cprbx) + msg->cprbx.cprb_len;
>  	memcpy(msg->hdr.function_code, function_code,

[Severity: High]
Can this modification lead to an uninitialized memory read?

If an attacker passes a CPRB request where xcrb->request_control_blk_length
is 13 bytes, the aligned req_cblen becomes 16. If they also set
cprbx.cprb_len to 14, z_copy_from_user() will copy only 13 bytes, leaving
the padding uninitialized.

The new bounds check allows cprb_len = 14 (since 14 is not greater than
16 - 2), and the code then copies 2 uninitialized bytes starting at offset 14
into msg->hdr.function_code. This could leak kernel heap memory to the
crypto device and influence routing decisions.

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar missing
minimum size validation in xcrb_msg_to_type6_ep11cprb_msgx().

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx() {
...
	/* Import CPRB data from the ioctl input parameter */
	if (z_copy_from_user(userspace, msg->userdata,
			     (char __force __user *)xcrb->req, xcrb->req_len)) {
		return -EFAULT;
	}

	if ((msg->pld_lenfmt & 0x80) == 0x80) {
...
}

Does this function also need a minimum size validation?

If a user issues the ZSENDEP11CPRB ioctl with an EP11 user request block
where req_len is extremely small (e.g., 0), z_copy_from_user() will succeed
by copying 0 bytes. The kernel then evaluates fields like msg->pld_lenfmt,
msg->cprbx.flags, and msg->cprbx.target_id from uninitialized kernel heap
memory, potentially allowing bypasses of authorization boundaries.

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