[PATCH v1 0/1] Improve code in zcrypt msg type 6 handling

[PATCH v1 0/1] Improve code in zcrypt msg type 6 handling

[Harald Freudenberger]

Changelog:

v1: initial version, one patch dealing with overflow checks around
    the use of the CEIL4 macro in zcrypt_msgtype6.c

Harald Freudenberger (1):
  s390/zcrypt: Improve length check and overflow detection at zcrypt msg
    type 6

 drivers/s390/crypto/zcrypt_msgtype6.c | 84 ++++++++++++++-------------
 1 file changed, 45 insertions(+), 39 deletions(-)

--
2.43.0

[PATCH v1 1/1] s390/zcrypt: Improve length check and overflow detection at zcrypt msg type 6

[Harald Freudenberger]

The existing overflow checks in zcrypt_msgtype6.c in function
xcrb_msg_to_type6cprb_msgx() and xcrb_msg_to_type6_ep11cprb_msgx() had
several security weaknesses:

1. The CEIL4() macro used for overflow detection has itself an
   overflow problem.
2. Post-calculation overflow detection could allow undefined behavior
   to occur before the check was performed.
2. Missing overflow checks on message length calculations created
   potential buffer overflow vulnerabilities.
3. Use of signed integer types for size calculations risked signed
   integer overflow undefined behavior.

The patch improves this by:

- Implementing overflow detection before the CEIL4() macro is used via
  kernel function check_add_overflow().
- Adding explicit overflow checks for all message length calculations
  to eliminate buffer overflow risks.
- Converting size variables from signed to unsigned types and thus
  prevents from signed overflow.
- Improving code readability by replacing complex ternary operators
  with clear if-else structures.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 84 ++++++++++++++-------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..8bf735a64383 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -342,50 +342,49 @@ 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;
+	u32 u, req_cblen, rep_cblen, req_sumlen, rep_sumlen;
+	char *req_data, *function_code;
 
-	if (CEIL4(xcrb->request_control_blk_length) <
-			xcrb->request_control_blk_length)
-		return -EINVAL; /* overflow after alignment*/
+	if (check_add_overflow(xcrb->request_control_blk_length, 3, &u))
+		return -EINVAL; /* would overflow at CEIL4 */
+	req_cblen = CEIL4(xcrb->request_control_blk_length);
 
 	/* length checks */
-	ap_msg->len = sizeof(struct type6_hdr) +
-		CEIL4(xcrb->request_control_blk_length) +
-		xcrb->request_data_length;
-	if (ap_msg->len > ap_msg->bufsize)
+	if (check_add_overflow(sizeof(struct type6_hdr), req_cblen, &u))
 		return -EINVAL;
+	if (check_add_overflow(u, xcrb->request_data_length, &u))
+		return -EINVAL;
+	ap_msg->len = u;
 
 	/*
 	 * Overflow check
 	 * sum must be greater (or equal) than the largest operand
 	 */
-	req_sumlen = CEIL4(xcrb->request_control_blk_length) +
-			xcrb->request_data_length;
-	if ((CEIL4(xcrb->request_control_blk_length) <=
-	     xcrb->request_data_length) ?
-	    req_sumlen < xcrb->request_data_length :
-	    req_sumlen < CEIL4(xcrb->request_control_blk_length)) {
-		return -EINVAL;
+	req_sumlen = req_cblen + xcrb->request_data_length;
+	if (req_cblen <= xcrb->request_data_length) {
+		if (req_sumlen < xcrb->request_data_length)
+			return -EINVAL;
+	} else {
+		if (req_sumlen < req_cblen)
+			return -EINVAL;
+
 	}
 
-	if (CEIL4(xcrb->reply_control_blk_length) <
-			xcrb->reply_control_blk_length)
-		return -EINVAL; /* overflow after alignment*/
+	if (check_add_overflow(xcrb->reply_control_blk_length, 3, &u))
+		return -EINVAL; /* would overflow at CEIL4 */
+	rep_cblen = CEIL4(xcrb->reply_control_blk_length);
 
 	/*
 	 * Overflow check
 	 * sum must be greater (or equal) than the largest operand
 	 */
-	resp_sumlen = CEIL4(xcrb->reply_control_blk_length) +
-			xcrb->reply_data_length;
-	if ((CEIL4(xcrb->reply_control_blk_length) <=
-	     xcrb->reply_data_length) ?
-	    resp_sumlen < xcrb->reply_data_length :
-	    resp_sumlen < CEIL4(xcrb->reply_control_blk_length)) {
-		return -EINVAL;
+	rep_sumlen = rep_cblen + xcrb->reply_data_length;
+	if (rep_cblen <= xcrb->reply_data_length) {
+		if (rep_sumlen < xcrb->reply_data_length)
+			return -EINVAL;
+	} else {
+		if (rep_sumlen < rep_cblen)
+			return -EINVAL;
 	}
 
 	/* prepare type6 header */
@@ -393,7 +392,7 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
 	memcpy(msg->hdr.agent_id, &xcrb->agent_ID, sizeof(xcrb->agent_ID));
 	msg->hdr.tocardlen1 = xcrb->request_control_blk_length;
 	if (xcrb->request_data_length) {
-		msg->hdr.offset2 = msg->hdr.offset1 + rcblen;
+		msg->hdr.offset2 = msg->hdr.offset1 + req_cblen;
 		msg->hdr.tocardlen2 = xcrb->request_data_length;
 	}
 	msg->hdr.fromcardlen1 = xcrb->reply_control_blk_length;
@@ -437,10 +436,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;
 }
@@ -480,16 +482,20 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap
 		unsigned int	dom_val;	/* domain id	   */
 	} __packed * payload_hdr = NULL;
 
-	if (CEIL4(xcrb->req_len) < xcrb->req_len)
-		return -EINVAL; /* overflow after alignment*/
+	u64 u;
+
+	if (check_add_overflow(xcrb->req_len, 3, &u))
+		return -EINVAL; /* would overflow when used with CEIL4 */
 
 	/* length checks */
-	ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
-	if (ap_msg->len > ap_msg->bufsize)
+	if (check_add_overflow(sizeof(struct type6_hdr),
+			       CEIL4(xcrb->req_len), &u) ||
+	    u > ap_msg->bufsize)
 		return -EINVAL;
+	ap_msg->len = u;
 
-	if (CEIL4(xcrb->resp_len) < xcrb->resp_len)
-		return -EINVAL; /* overflow after alignment*/
+	if (check_add_overflow(xcrb->resp_len, 3, &u))
+		return -EINVAL; /* would overflow when used with CEIL4 */
 
 	/* prepare type6 header */
 	msg->hdr = static_type6_ep11_hdr;
-- 
2.43.0

Re: [PATCH v1 1/1] s390/zcrypt: Improve length check and overflow detection at zcrypt msg type 6

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Heap buffer overflow in xcrb_msg_to_type6cprb_msgx() due to missing ap_msg->bufsize bound check.
--

--- Patch [1]: [PATCH v1 1/1] s390/zcrypt: Improve length check and overflow detection at zcrypt msg type 6 ---
commit 817ae0ed2183d762b91c6888629d3fa414135f83
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve length check and overflow detection at zcrypt msg type 6

This commit replaces manual overflow checks with check_add_overflow() and
changes some variable types to unsigned. It also aims to prevent buffer
overflow risks by improving message length calculations.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..8bf735a643835 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -342,50 +342,49 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,

[ ... ]

>  	/* length checks */
> -	ap_msg->len = sizeof(struct type6_hdr) +
> -		CEIL4(xcrb->request_control_blk_length) +
> -		xcrb->request_data_length;
> -	if (ap_msg->len > ap_msg->bufsize)
> +	if (check_add_overflow(sizeof(struct type6_hdr), req_cblen, &u))
>  		return -EINVAL;
> +	if (check_add_overflow(u, xcrb->request_data_length, &u))
> +		return -EINVAL;
> +	ap_msg->len = u;

[Severity: Critical]
Does removing the check against ap_msg->bufsize allow an arbitrarily large
length to bypass validation?

If u is successfully calculated without integer overflow but is larger than
ap_msg->bufsize, it appears the subsequent calls to z_copy_from_user() could
write out of bounds into the ap_msg->msg heap buffer:

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6cprb_msgx() {
	...
	/* prepare CPRB */
	if (z_copy_from_user(userspace, msg->userdata,
			     xcrb->request_control_blk_addr,
			     xcrb->request_control_blk_length))
		return -EFAULT;
	...
	/* copy data block */
	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;
	}
}

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