[PATCH v4 0/1] Improve zcrypt reply message verification checks

[PATCH v4 0/1] Improve zcrypt reply message verification checks

[Harald Freudenberger]

Add or improve checks related to buffer sizes and reply sizes to the
handling of replies from the crypto cards for CCA and EP11 (AP message
type 6) messages. The verification code related to reply field length
was not designed well and thus firmware deficiencies could lead to
unexpected behavior in the zcrypt device driver. Thus improve the code
to more closely inspect especially length fields at message replies.

Changelog:
v1 - initial patch
v2 - fixed typo in header check_for_overflow -> check_add_overflow.
v3 - rephrased and smoothed subject and text of the patch. It is now
     "s390/zcrypt: Improve zcrypt reply message verification checks"
     and the text does not talk about malicious cards any more.
     Updated Reviewed-by tags
v4 - As sashiko clearly states the addition of two 32 bit values can
     mathematically never overflow a 64 bit value and thus the
     check_add_overflow() was total overkill - removed.

Harald Freudenberger (1):
  s390/zcrypt: Improve zcrypt reply message verification checks

 drivers/s390/crypto/zcrypt_msgtype6.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--
2.43.0

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

[Harald Freudenberger]

Add or improve checks related to buffer sizes and reply sizes to the
handling of replies from the crypto cards for CCA and EP11 (AP message
type 6) messages. The verification code related to reply field length
was not designed well and thus firmware deficiencies could lead to
unexpected behavior in the zcrypt device driver. Thus improve the code
to more closely inspect especially length fields at message replies.

The 3 hunks of this patch deal with CCA, EP11 and (CCA) RNG replies
and improve the checking for reply buffer size by using size_t instead
of int. RNG replies an additional check makes sure the hard coded
limit of the data buffer is not exceeded.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
Cc: stable@vger.kernel.org
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..ae175cab041b 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);
 	return msg->fmt2.count2;
 }
@@ -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)
@@ -917,7 +924,7 @@ static void zcrypt_msgtype6_receive_ep11(struct ap_queue *aq,
 	};
 	struct ap_response_type *resp_type = &msg->response;
 	struct type86_ep11_reply *t86r;
-	int len;
+	size_t len;
 
 	/* Copy the reply message to the request message buffer. */
 	if (!reply)
-- 
2.43.0

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

[sashiko-bot]

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