Re: [PATCH v4 1/1] s390/zcrypt: Improve zcrypt reply message verification checks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] 32-bit integer overflow in reply length validation allows out-of-bounds kernel memory read.
- [High] Missing bounds validation for `offset1` and `count1` when `count2` is non-zero allows arbitrary kernel memory leak.
--

--- Patch [1]: [PATCH v4 1/1] s390/zcrypt: Improve zcrypt reply message verification checks ---
commit 6d9229604582e7a664ea0c26830d5edc37b47568
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve zcrypt reply message verification checks

This patch adds or improves checks related to buffer sizes and reply sizes
in the handling of CCA and EP11 replies from crypto cards. It changes reply
buffer sizes from int to size_t and adds a check for RNG replies to avoid
exceeding the hard-coded data buffer limit.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..ae175cab041b8 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -691,6 +691,13 @@ static int convert_type86_rng(struct zcrypt_queue *zq,
>  
>  	if (msg->cprbx.ccp_rtcode != 0 || msg->cprbx.ccp_rscode != 0)
>  		return -EINVAL;
> +	/*
> +	 * Note that offset2 and count2 have already been checked in
> +	 * zcrypt_msgtype6_receive(). So only check for not exceeding
> +	 * the hard coded rng buffer size.
> +	 */
> +	if (msg->fmt2.count2 > ZCRYPT_RNG_BUFFER_SIZE)
> +		return -EMSGSIZE;
>  	memcpy(buffer, data + msg->fmt2.offset2, msg->fmt2.count2);

[Severity: High]
This is a pre-existing issue, but does this bounds checking fully prevent
out-of-bounds reads if the offset2 and count2 values wrap around?

In zcrypt_msgtype6_receive(), len is calculated as:

    len = t86r->fmt2.offset2 + t86r->fmt2.count2;

If the firmware provides values that wrap modulo 2^32 (for instance,
offset2 = 0xFFFFFFE0 and count2 = 0x40), the sum is evaluated as an addition
of two 32-bit unsigned integers before being zero-extended to size_t.

The wrapped value could match the hardware's reply->len and bypass the
subsequent len > msg->bufsize checks.

This would allow the memcpy() here to read out-of-bounds kernel memory.

Could a malicious or faulty CEX card firmware trigger an out-of-bounds read
of kernel memory with this wrap around?

[ ... ]

> @@ -853,7 +860,7 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
>  	};
>  	struct ap_response_type *resp_type = &msg->response;
>  	struct type86x_reply *t86r;
> -	int len;
> +	size_t len;
>  
>  	/* Copy the reply message to the request message buffer. */
>  	if (!reply)
>  		goto out;	/* ap_msg->rc indicates the error */

[Severity: High]
This isn't a bug introduced by this patch, but does the bounds check in this
function correctly validate offset1 and count1 when count2 is non-zero?

When processing a CEXXC_RESPONSE_TYPE_XCRB response, the length validation
logic checks:

drivers/s390/crypto/zcrypt_msgtype6.c:zcrypt_msgtype6_receive() {
	...
		case CEXXC_RESPONSE_TYPE_XCRB:
			if (t86r->fmt2.count2)
				len = t86r->fmt2.offset2 + t86r->fmt2.count2;
			else
				len = t86r->fmt2.offset1 + t86r->fmt2.count1;
			if (len > reply->bufsize || len > msg->bufsize ||
	...
}

If t86r->fmt2.count2 is non-zero, only offset2 and count2 are validated.

However, in convert_type86_xcrb(), offset1 and count1 are unconditionally
used to copy kernel memory to userspace:

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))
		return -EFAULT;
	...
}

Could firmware provide a valid count2 and offset2 alongside maliciously
large offset1 and count1 values to leak kernel memory to userspace?

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