[PATCH v2 0/1] Fix and improve zcrypt reply message checks

[PATCH v2 0/1] Fix and improve zcrypt reply message checks

[Harald Freudenberger]

Fix and improve zcrypt reply messages checks of type 6 - CCA and EP11
- and type 50 - ICA.

Changelog:
v1: Initial version
v2: Sashiko complained about not checking offset1/count1 at all if
    count2 for an CCA type6 reply is given. And a check like this was
    in place before the rework. So now offset1/count1 is always
    checked and only if count2 is non zero offset2/count2 is
    additionally checked.

Harald Freudenberger (1):
  s390/zcrypt: Fix and improve zcrypt reply message verification checks

 drivers/s390/crypto/zcrypt_msgtype50.c |  40 +++++--
 drivers/s390/crypto/zcrypt_msgtype6.c  | 160 +++++++++++++++++--------
 2 files changed, 136 insertions(+), 64 deletions(-)

--
2.43.0

[PATCH v2 1/1] s390/zcrypt: Fix and 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, EP11 (AP message
type 6) and ICA (AP type 50) 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.

Add length validation before accessing reply message structures in
zcrypt_msgtype6_receive(), zcrypt_msgtype6_receive_ep11(), and
zcrypt_msgtype50_receive() to prevent out-of-bounds reads and
potential kernel memory disclosure.

Also rework these three functions to validate reply lengths more
carefully before copying data back into the request buffer. Use size_t
for length calculations, reject inconsistent reply sizes, and add
defensive handling for short invalid replies. For XCRB replies,
validate both reply segments and derive the effective message length
from the covered range instead of trusting only the second segment.

Fixes: 3b6245fd303f ("s390/zcrypt: Separate msgtype implementation from card modules.")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org
---
 drivers/s390/crypto/zcrypt_msgtype50.c |  40 +++++--
 drivers/s390/crypto/zcrypt_msgtype6.c  | 160 +++++++++++++++++--------
 2 files changed, 136 insertions(+), 64 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype50.c b/drivers/s390/crypto/zcrypt_msgtype50.c
index d6fc2d8e7fad..80c447ea2d83 100644
--- a/drivers/s390/crypto/zcrypt_msgtype50.c
+++ b/drivers/s390/crypto/zcrypt_msgtype50.c
@@ -416,26 +416,40 @@ static void zcrypt_msgtype50_receive(struct ap_queue *aq,
 		.reply_code = REP82_ERROR_MACHINE_FAILURE,
 	};
 	struct type80_hdr *t80h;
-	int len;
+	size_t len;
 
 	/* Copy the reply message to the request message buffer. */
 	if (!reply)
 		goto out;	/* ap_msg->rc indicates the error */
+
 	t80h = reply->msg;
-	if (t80h->type == TYPE80_RSP_CODE) {
-		len = t80h->len;
-		if (len > reply->bufsize || len > msg->bufsize ||
-		    len != reply->len) {
-			pr_debug("len mismatch => EMSGSIZE\n");
-			msg->rc = -EMSGSIZE;
-			goto out;
-		}
-		memcpy(msg->msg, reply->msg, len);
-		msg->len = len;
-	} else {
-		memcpy(msg->msg, reply->msg, sizeof(error_reply));
+
+	if (reply->len < sizeof(*t80h) ||
+	    t80h->type != TYPE80_RSP_CODE) {
+		if (reply->len < sizeof(error_reply))
+			memcpy(msg->msg, &error_reply, sizeof(error_reply));
+		else
+			memcpy(msg->msg, reply->msg, sizeof(error_reply));
 		msg->len = sizeof(error_reply);
+		goto out;
+	}
+
+	len = t80h->len;
+	if (len != reply->len) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu rpl.len %zu mismatch, msg.rc=%d\n",
+			 len, reply->len, msg->rc);
+		goto out;
 	}
+	if (len > reply->bufsize || len > msg->bufsize) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu exceeds buf %zu/%zu, msg.rc=%d\n",
+			 len, reply->bufsize, msg->bufsize, msg->rc);
+		goto out;
+	}
+	memcpy(msg->msg, reply->msg, len);
+	msg->len = len;
+
 out:
 	complete(&msg->response.work);
 }
diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 3df1d676de5d..b53801b0a971 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -766,6 +766,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;
 }
@@ -928,48 +935,80 @@ 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 minlen, len;
 
 	/* Copy the reply message to the request message buffer. */
 	if (!reply)
 		goto out;	/* ap_msg->rc indicates the error */
+
 	t86r = reply->msg;
-	if (t86r->hdr.type == TYPE86_RSP_CODE &&
-	    t86r->cprbx.cprb_ver_id == 0x02) {
-		switch (resp_type->type) {
-		case CEXXC_RESPONSE_TYPE_ICA:
-			len = sizeof(struct type86x_reply) + t86r->length;
-			if (len > reply->bufsize || len > msg->bufsize ||
-			    len != reply->len) {
-				pr_debug("len mismatch => EMSGSIZE\n");
-				msg->rc = -EMSGSIZE;
-				goto out;
-			}
-			memcpy(msg->msg, reply->msg, len);
-			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;
-			if (len > reply->bufsize || len > msg->bufsize ||
-			    len != reply->len) {
-				pr_debug("len mismatch => EMSGSIZE\n");
+	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
+		offsetof(struct CPRBX, cprb_ver_id) +
+		sizeof(t86r->cprbx.cprb_ver_id);
+
+	if (reply->len < minlen ||
+	    t86r->hdr.type != TYPE86_RSP_CODE ||
+	    t86r->cprbx.cprb_ver_id != 0x02) {
+		if (reply->len < sizeof(error_reply))
+			memcpy(msg->msg, &error_reply, sizeof(error_reply));
+		else
+			memcpy(msg->msg, reply->msg, sizeof(error_reply));
+		msg->len = sizeof(error_reply);
+		goto out;
+	}
+
+	switch (resp_type->type) {
+	case CEXXC_RESPONSE_TYPE_ICA:
+		if (reply->len < sizeof(struct type86x_reply)) {
+			msg->rc = -EMSGSIZE;
+			pr_debug("rpl.len %zu < struct type86_reply, msg.rc=%d\n",
+				 reply->len, msg->rc);
+			goto out;
+		}
+		len = sizeof(struct type86x_reply) + (size_t)t86r->length;
+		break;
+	case CEXXC_RESPONSE_TYPE_XCRB:
+		len = (size_t)t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
+		if (len > reply->len) {
+			msg->rc = -EMSGSIZE;
+			pr_debug("offset1 %u count1 %u rpl.len %zu mismatch, msg.rc=%d\n",
+				 t86r->fmt2.offset1, t86r->fmt2.count1,
+				 reply->len, msg->rc);
+			goto out;
+		}
+		if (t86r->fmt2.count2) {
+			len = (size_t)t86r->fmt2.offset2 +
+				(size_t)t86r->fmt2.count2;
+			if (len > reply->len) {
 				msg->rc = -EMSGSIZE;
+				pr_debug("offset2 %u count2 %u rpl.len %zu mismatch, msg.rc=%d\n",
+					 t86r->fmt2.offset2, t86r->fmt2.count2,
+					 reply->len, msg->rc);
 				goto out;
 			}
-			memcpy(msg->msg, reply->msg, len);
-			msg->len = len;
-			break;
-		default:
-			memcpy(msg->msg, &error_reply, sizeof(error_reply));
-			msg->len = sizeof(error_reply);
 		}
-	} else {
-		memcpy(msg->msg, reply->msg, sizeof(error_reply));
+		break;
+	default:
+		memcpy(msg->msg, &error_reply, sizeof(error_reply));
 		msg->len = sizeof(error_reply);
+		goto out;
 	}
+
+	if (len != reply->len) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu rpl.len %zu mismatch, msg.rc=%d\n",
+			 len, reply->len, msg->rc);
+		goto out;
+	}
+	if (len > reply->bufsize || len > msg->bufsize) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu exceeds buf %zu/%zu, msg.rc=%d\n",
+			 len, reply->bufsize, msg->bufsize, msg->rc);
+		goto out;
+	}
+	memcpy(msg->msg, reply->msg, len);
+	msg->len = len;
+
 out:
 	complete(&resp_type->work);
 }
@@ -992,34 +1031,53 @@ 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 minlen, len;
 
 	/* Copy the reply message to the request message buffer. */
 	if (!reply)
 		goto out;	/* ap_msg->rc indicates the error */
+
 	t86r = reply->msg;
-	if (t86r->hdr.type == TYPE86_RSP_CODE &&
-	    t86r->cprbx.cprb_ver_id == 0x04) {
-		switch (resp_type->type) {
-		case CEXXC_RESPONSE_TYPE_EP11:
-			len = t86r->fmt2.offset1 + t86r->fmt2.count1;
-			if (len > reply->bufsize || len > msg->bufsize ||
-			    len != reply->len) {
-				pr_debug("len mismatch => EMSGSIZE\n");
-				msg->rc = -EMSGSIZE;
-				goto out;
-			}
-			memcpy(msg->msg, reply->msg, len);
-			msg->len = len;
-			break;
-		default:
+	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
+		offsetof(struct ep11_cprb, cprb_ver_id) +
+		sizeof(t86r->cprbx.cprb_ver_id);
+
+	if (reply->len < minlen ||
+	    t86r->hdr.type != TYPE86_RSP_CODE ||
+	    t86r->cprbx.cprb_ver_id != 0x04) {
+		if (reply->len < sizeof(error_reply))
 			memcpy(msg->msg, &error_reply, sizeof(error_reply));
-			msg->len = sizeof(error_reply);
-		}
-	} else {
-		memcpy(msg->msg, reply->msg, sizeof(error_reply));
+		else
+			memcpy(msg->msg, reply->msg, sizeof(error_reply));
 		msg->len = sizeof(error_reply);
+		goto out;
 	}
+
+	switch (resp_type->type) {
+	case CEXXC_RESPONSE_TYPE_EP11:
+		len = (size_t)t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
+		break;
+	default:
+		memcpy(msg->msg, &error_reply, sizeof(error_reply));
+		msg->len = sizeof(error_reply);
+		goto out;
+	}
+
+	if (len != reply->len) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu rpl.len %zu mismatch, msg.rc=%d\n",
+			 len, reply->len, msg->rc);
+		goto out;
+	}
+	if (len > reply->bufsize || len > msg->bufsize) {
+		msg->rc = -EMSGSIZE;
+		pr_debug("len %zu exceeds buf %zu/%zu, msg.rc=%d\n",
+			 len, reply->bufsize, msg->bufsize, msg->rc);
+		goto out;
+	}
+	memcpy(msg->msg, reply->msg, len);
+	msg->len = len;
+
 out:
 	complete(&resp_type->work);
 }
-- 
2.43.0

Re: [PATCH v2 1/1] s390/zcrypt: Fix and improve zcrypt reply message verification checks

[sashiko-bot]

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

New issues:
- [High] Missing error code assignment (`msg->rc = -EMSGSIZE`) on short replies leads to kernel memory disclosure in multiple message receive functions.

Pre-existing issues:
- [Medium] Incomplete length validation for CEXXC_RESPONSE_TYPE_XCRB allows an out-of-bounds read of CPRBX fields in convert_type86_rng().
--

commit 4aedb5baa8548df5ca01ec9ef8a55fbf780a7ee8
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Fix and 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, EP11 (AP message
type 6) and ICA (AP type 50) 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.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype50.c b/drivers/s390/crypto/zcrypt_msgtype50.c
> index d6fc2d8e7fad9..80c447ea2d839 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype50.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype50.c
> @@ -416,26 +416,40 @@ static void zcrypt_msgtype50_receive(struct ap_queue *aq,
>  		.reply_code = REP82_ERROR_MACHINE_FAILURE,
>  	};
>  	struct type80_hdr *t80h;
> -	int len;
> +	size_t len;
>  
>  	/* Copy the reply message to the request message buffer. */
>  	if (!reply)
>  		goto out;	/* ap_msg->rc indicates the error */
> +
>  	t80h = reply->msg;
> -	if (t80h->type == TYPE80_RSP_CODE) {
> -		len = t80h->len;
> -		if (len > reply->bufsize || len > msg->bufsize ||
> -		    len != reply->len) {
> -			pr_debug("len mismatch => EMSGSIZE\n");
> -			msg->rc = -EMSGSIZE;
> -			goto out;
> -		}
> -		memcpy(msg->msg, reply->msg, len);
> -		msg->len = len;
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +
> +	if (reply->len < sizeof(*t80h) ||
> +	    t80h->type != TYPE80_RSP_CODE) {
> +		if (reply->len < sizeof(error_reply))
> +			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
>  		msg->len = sizeof(error_reply);
> +		goto out;

[Severity: High]
Does this missing error code assignment on short replies inadvertently
validate them?

If the hardware sends a reply that is shorter than sizeof(*t80h) but has a
valid success header, this early bailout catches it but fails to set msg->rc
to an error code like -EMSGSIZE. Because the caller sees rc == 0, it may
parse the success header and read uninitialized/leftover memory from the
request buffer, potentially leaking kernel memory to userspace.

> +	}
> +
> +	len = t80h->len;

[ ... ]

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 3df1d676de5d8..b53801b0a9710 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -766,6 +766,13 @@ static int convert_type86_rng(struct zcrypt_queue *zq,
>  
>  	if (msg->cprbx.ccp_rtcode != 0 || msg->cprbx.ccp_rscode != 0)
>  		return -EINVAL;

[Severity: Medium]
This isn't a bug introduced by this patch, but could this blindly access
ccp_rtcode and ccp_rscode if the hardware sent a truncated reply?

If a hardware or emulator responds to a CEXXC_RESPONSE_TYPE_XCRB request
with a truncated message that satisfies the 51-byte minlen checked in
zcrypt_msgtype6_receive(), but doesn't include the full CPRBX structure,
these fields may contain leftover data from the request buffer.

> +	/*
> +	 * 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;
>  }

[ ... ]

> @@ -928,48 +935,80 @@ 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 minlen, len;
>  
>  	/* Copy the reply message to the request message buffer. */
>  	if (!reply)
>  		goto out;	/* ap_msg->rc indicates the error */
> +
>  	t86r = reply->msg;
> -	if (t86r->hdr.type == TYPE86_RSP_CODE &&
> -	    t86r->cprbx.cprb_ver_id == 0x02) {
> -		switch (resp_type->type) {
> +	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
> +		offsetof(struct CPRBX, cprb_ver_id) +
> +		sizeof(t86r->cprbx.cprb_ver_id);

[Severity: Medium]
This isn't a bug introduced by this patch, but does this minlen check
guarantee the rest of the CPRBX structure is present?

This check ensures the message has a valid cprb_ver_id (51 bytes), but it
might not guarantee the rest of the CPRBX structure is present before allowing
the CEXXC_RESPONSE_TYPE_XCRB branch to succeed, potentially leading to the
issue in convert_type86_rng() mentioned above.

> +
> +	if (reply->len < minlen ||
> +	    t86r->hdr.type != TYPE86_RSP_CODE ||
> +	    t86r->cprbx.cprb_ver_id != 0x02) {
> +		if (reply->len < sizeof(error_reply))
> +			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +		msg->len = sizeof(error_reply);
> +		goto out;

[Severity: High]
Does this missing error code assignment (msg->rc = -EMSGSIZE) similarly lead
to unintended memory disclosures as in zcrypt_msgtype50_receive()?

For ICA requests, convert_type86_ica() could use the leftover count1 to copy
up to PAGE_SIZE bytes of the buffer back to userspace if the rc remains 0.

> +	}
> +
> +	switch (resp_type->type) {

[ ... ]

> @@ -992,34 +1031,53 @@ 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 minlen, len;
>  
>  	/* Copy the reply message to the request message buffer. */
>  	if (!reply)
>  		goto out;	/* ap_msg->rc indicates the error */
> +
>  	t86r = reply->msg;
> -	if (t86r->hdr.type == TYPE86_RSP_CODE &&
> -	    t86r->cprbx.cprb_ver_id == 0x04) {
> -		switch (resp_type->type) {
> +	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
> +		offsetof(struct ep11_cprb, cprb_ver_id) +
> +		sizeof(t86r->cprbx.cprb_ver_id);
> +
> +	if (reply->len < minlen ||
> +	    t86r->hdr.type != TYPE86_RSP_CODE ||
> +	    t86r->cprbx.cprb_ver_id != 0x04) {
> +		if (reply->len < sizeof(error_reply))
>  			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> -			msg->len = sizeof(error_reply);
> -		}
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
>  		msg->len = sizeof(error_reply);
> +		goto out;

[Severity: High]
Could this logic omit setting the error code for short EP11 messages as well?

Bypassing length validation here may similarly allow uninitialized buffer
reads.

>  	}
> +
> +	switch (resp_type->type) {

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