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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer overflow in `xcrb_msg_to_type6_ep11cprb_msgx` allows bypassing message length checks, leading to an out-of-bounds `memset` in `copy_from_user` that panics the kernel.
- [High] Missing minimum size validation in `xcrb_msg_to_type6_ep11cprb_msgx` allows uninitialized memory access.
- [High] Kernel memory leak via uninitialized alignment padding bytes in `xcrb_msg_to_type6_ep11cprb_msgx`.
- [High] The driver trusts length and offset values (`offset1`, `count1`) from the hardware's type86_fmt2 message without fully checking them against the received message length, which can lead to an out-of-bounds kernel memory read.
--

--- Patch [1]: [PATCH v6 1/2] s390/zcrypt: Improve CCA CPRB length and overflow checks ---
commit 7784ed33ea5e9edca90b3daa3295185e1ce64459
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve CCA CPRB length and overflow checks

The xcrb_msg_to_type6cprb_msgx() function lacks proper input validation,
creating several security vulnerabilities including integer overflows, missing
minimum size validations, and a potential kernel memory leak. This patch fixes
these issues by using size_t for length calculations, adding boundary checks,
and zeroing trailing bytes.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..2e4aef330b680 100644
> --- 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,
[ ... ]
> -	if (CEIL4(xcrb->request_control_blk_length) <
> -			xcrb->request_control_blk_length)
> -		return -EINVAL; /* overflow after alignment*/
> -
> -	/* length checks */
> +	/* request length and overflow checks */
> +	if (xcrb->request_control_blk_length < sizeof(struct CPRBX))
> +		return -EINVAL;

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

Can a small req_len (e.g., 0) cause reads of uninitialized kernel heap memory
when extracting fields like pld_lenfmt or flags?

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx() {
    ...
    if ((msg->pld_lenfmt & 0x80) == 0x80) {
    ...
}

> +	req_cblen = CEIL4((size_t)xcrb->request_control_blk_length);
> +	if (req_cblen > U32_MAX)
> +		return -EINVAL;
>  	ap_msg->len = sizeof(struct type6_hdr) +
> -		CEIL4(xcrb->request_control_blk_length) +
> -		xcrb->request_data_length;
> +		req_cblen + xcrb->request_data_length;
>  	if (ap_msg->len > ap_msg->bufsize)
>  		return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but it looks like xcrb_msg_to_type6_ep11cprb_msgx()
has an integer overflow similar to the one fixed in this patch.

Can a massive req_len cause an integer overflow when computing ap_msg->len,
bypassing the ap_msg->bufsize bounds check?

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx() {
    ...
    ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
    ...
    if (z_copy_from_user(userspace, msg->userdata,
                         (char __force __user *)xcrb->req, xcrb->req_len)) {
    ...
}

If z_copy_from_user() is called with the large length, the copy_from_user()
fallback memset could write outside the allocated kernel buffer.

[ ... ]
> @@ -404,6 +395,9 @@ 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 (xcrb->request_control_blk_length < req_cblen)
> +		memset(msg->userdata + xcrb->request_control_blk_length,
> +		       0, req_cblen - xcrb->request_control_blk_length);

[Severity: High]
This is a pre-existing issue, but does xcrb_msg_to_type6_ep11cprb_msgx() leak
kernel memory via uninitialized alignment padding bytes in a similar way?

The buffer length sent to the hardware incorporates alignment, but only req_len
bytes are copied, leaving padding bytes uninitialized:

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx() {
    ...
    if (z_copy_from_user(userspace, msg->userdata,
                         (char __force __user *)xcrb->req, xcrb->req_len)) {
    ...
}

[ ... ]
> @@ -437,10 +431,13 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
>  	}
>  
>  	/* copy data block */
> -	if (xcrb->request_data_length &&
> -	    z_copy_from_user(userspace, req_data, xcrb->request_data_address,
> -			     xcrb->request_data_length))
> -		return -EFAULT;
> +	if (xcrb->request_data_length) {
> +		req_data = ap_msg->msg + sizeof(struct type6_hdr) + req_cblen;
> +		if (z_copy_from_user(userspace, req_data,
> +				     xcrb->request_data_address,
> +				     xcrb->request_data_length))
> +			return -EFAULT;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does convert_type86_xcrb()
trust length and offset values from the hardware without fully checking them?

zcrypt_msgtype6_receive() only validates offset2 and count2 if count2 is
non-zero, ignoring offset1 and count1 during the bounds check:

drivers/s390/crypto/zcrypt_msgtype6.c:zcrypt_msgtype6_receive() {
    ...
    if (t86r->fmt2.count2)
        len = t86r->fmt2.offset2 + t86r->fmt2.count2;
    ...
}

Later, convert_type86_xcrb() uses offset1 and count1 for a copy to user space
without verifying they are within the initialized message buffer:

drivers/s390/crypto/zcrypt_msgtype6.c:convert_type86_xcrb() {
    ...
    if (z_copy_to_user(userspace, xcrb->reply_control_blk_addr,
                       data + msg->fmt2.offset1, msg->fmt2.count1))
    ...
}

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