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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing minimum size validation in `xcrb_msg_to_type6_ep11cprb_msgx` allows uninitialized memory access.
- [Medium] Uninitialized padding bytes from the kernel heap are leaked to the crypto device hardware.
--

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

s390/zcrypt: Improve CCA CPRB length and overflow checks

This commit improves CCA CPRB length and overflow checks in
xcrb_msg_to_type6cprb_msgx(). It switches to using size_t for length
calculations and adds U32_MAX boundary checks after alignment. It also
introduces minimum control block size validation before copying data from
userspace.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..4ac767661cb50 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,
>  		};
>  	} __packed * msg = ap_msg->msg;
>  
> -	int rcblen = CEIL4(xcrb->request_control_blk_length);
> -	int req_sumlen, resp_sumlen;
> -	char *req_data = ap_msg->msg + sizeof(struct type6_hdr) + rcblen;
> -	char *function_code;
> +	size_t req_cblen, rep_cblen, req_sumlen, rep_sumlen;
> +	char *function_code, *req_data;
>  
> -	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;
> +	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;

[Severity: Medium]
This is a pre-existing issue, but does this code leak uninitialized memory to
the hardware?

Because ap_msg->len incorporates the 4-byte aligned req_cblen, but the
subsequent z_copy_from_user() only copies xcrb->request_control_blk_length
bytes, the trailing padding bytes in ap_msg->msg could remain uninitialized.

When this message is sent over the bus, does this leak kernel heap data to the
crypto device hardware?

>  	if (ap_msg->len > ap_msg->bufsize)
>  		return -EINVAL;

[ ... ]

> @@ -437,10 +428,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 while you are fixing the length
checks in xcrb_msg_to_type6cprb_msgx(), does xcrb_msg_to_type6_ep11cprb_msgx()
need similar minimum size validation?

In xcrb_msg_to_type6_ep11cprb_msgx(), there does not appear to be a check for a
minimum size before reading xcrb->req_len bytes:

	/* 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) {

If an unprivileged user space application issues a ZSENDEP11CPRB ioctl with
req_len smaller than the required control block size, could this result in
subsequent code parsing uninitialized trailing bytes from msg->userdata?

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