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

[PATCH v5 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.

The 3 parts 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. Also there was a condition
with additional data for an CCA reply where some of the field values
where unchecked used to invoke memcpy into user
space. zcrypt_msgtype6_receive() now checks all the relevant fields
before convert_type86_xcrb() uses them.

Improve zcrypt reply message verification checks

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.
v5 - Sashiko had a by-catch related to the very same fields. Under
     some circumstances the fields count1 and offset1 of the CPRB
     where not checked but used for a memcpy to userspace. So again a
     rework of the check of these x86 header fields in the receiving
     function before the CPRB is processed to be copied in parts to
     userspace. Removed all reviewed-by as I want to have another
     developer look onto this patch again.

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

 drivers/s390/crypto/zcrypt_msgtype6.c | 42 ++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 8 deletions(-)

--
2.43.0

[PATCH v5 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. Also there was a condition
with additional data for an CCA reply where some of the field values
where unchecked used to invoke memcpy into user
space. zcrypt_msgtype6_receive() now checks all the relevant fields
before convert_type86_xcrb() uses them.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 42 ++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..8252fd185663 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, len1, len2 = 0;
 
 	/* Copy the reply message to the request message buffer. */
 	if (!reply)
@@ -863,7 +870,8 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
 	    t86r->cprbx.cprb_ver_id == 0x02) {
 		switch (resp_type->type) {
 		case CEXXC_RESPONSE_TYPE_ICA:
-			len = sizeof(struct type86x_reply) + t86r->length;
+			len = (size_t)sizeof(struct type86x_reply) +
+				(size_t)t86r->length;
 			if (len > reply->bufsize || len > msg->bufsize ||
 			    len != reply->len) {
 				pr_debug("len mismatch => EMSGSIZE\n");
@@ -874,10 +882,27 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
 			msg->len = len;
 			break;
 		case CEXXC_RESPONSE_TYPE_XCRB:
-			if (t86r->fmt2.count2)
-				len = t86r->fmt2.offset2 + t86r->fmt2.count2;
-			else
-				len = t86r->fmt2.offset1 + t86r->fmt2.count1;
+			len1 = (size_t)t86r->fmt2.offset1 +
+				(size_t)t86r->fmt2.count1;
+			if (t86r->fmt2.offset1 > reply->len ||
+			    t86r->fmt2.count1 > reply->len ||
+			    len1 > reply->len) {
+				pr_debug("len mismatch => EMSGSIZE\n");
+				msg->rc = -EMSGSIZE;
+				goto out;
+			}
+			if (t86r->fmt2.count2) {
+				len2 = (size_t)t86r->fmt2.offset2 +
+					(size_t)t86r->fmt2.count2;
+				if (t86r->fmt2.offset2 > reply->len ||
+				    t86r->fmt2.count2 > reply->len ||
+				    len2 > reply->len) {
+					pr_debug("len mismatch => EMSGSIZE\n");
+					msg->rc = -EMSGSIZE;
+					goto out;
+				}
+			}
+			len = max_t(size_t, len1, len2);
 			if (len > reply->bufsize || len > msg->bufsize ||
 			    len != reply->len) {
 				pr_debug("len mismatch => EMSGSIZE\n");
@@ -917,7 +942,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)
@@ -927,7 +952,8 @@ static void zcrypt_msgtype6_receive_ep11(struct ap_queue *aq,
 	    t86r->cprbx.cprb_ver_id == 0x04) {
 		switch (resp_type->type) {
 		case CEXXC_RESPONSE_TYPE_EP11:
-			len = t86r->fmt2.offset1 + t86r->fmt2.count1;
+			len = (size_t)t86r->fmt2.offset1 +
+				(size_t)t86r->fmt2.count1;
 			if (len > reply->bufsize || len > msg->bufsize ||
 			    len != reply->len) {
 				pr_debug("len mismatch => EMSGSIZE\n");
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Uninitialized memory usage and buffer reuse leading to kernel heap information leak
--

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

s390/zcrypt: Improve zcrypt reply message verification checks

This commit improves reply message verification checks for CCA and EP11
messages in the zcrypt device driver. It updates length field checking to use
size_t and ensures bounds checking before using values to invoke memcpy.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..8252fd1856638 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -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, len1, len2 = 0;
>  
>  	/* Copy the reply message to the request message buffer. */
>  	if (!reply)
> @@ -863,7 +870,8 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
>  	    t86r->cprbx.cprb_ver_id == 0x02) {

[Severity: Medium]
This is a pre-existing issue, but is it possible for a short reply to cause
an out-of-bounds read here? The cprb_ver_id field is accessed before verifying
that reply->len is large enough to contain the CPRBX header.

Additionally, if the hardware returns a response that hits the else branch
in zcrypt_msgtype6_receive():

drivers/s390/crypto/zcrypt_msgtype6.c:zcrypt_msgtype6_receive() {
	...
	} else {
		memcpy(msg->msg, reply->msg, sizeof(error_reply));
		msg->len = sizeof(error_reply);
	}
	...
}

Only 8 bytes (sizeof(error_reply)) are copied, and the rest of msg->msg
remains user-controlled from the original request.

Could a malicious user craft a request via ZSECSENDCPRB embedding specific
values for cprb_ver_id and fmt2.offset2/count2, and if a short response
forces the else branch, bypass verification?

If convert_response_xcrb() reads the mixed buffer, sees TYPE86_RSP_CODE,
and validates cprb_ver_id == 0x02 from the user's data, it would call
convert_type86_xcrb(), which then copies arbitrary kernel heap memory to
user space using the user-controlled offsets:

drivers/s390/crypto/zcrypt_msgtype6.c:convert_type86_xcrb() {
	...
		if (z_copy_to_user(userspace, xcrb->reply_data_addr,
				   data + msg->fmt2.offset2, msg->fmt2.count2))
			return -EFAULT;
	...
}

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