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

[PATCH v2 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
v2: Sashiko found a regression: The check
      if (ap_msg->len > ap_msg->bufsize)
    was missing in the rework. Added again this important check.

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

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

-- 
2.43.0

[PATCH v2 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 | 83 +++++++++++++++------------
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..d2ae58789aa3 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -342,19 +342,18 @@ 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 (check_add_overflow(sizeof(struct type6_hdr), req_cblen, &u))
+		return -EINVAL;
+	if (check_add_overflow(u, xcrb->request_data_length, &ap_msg->len))
+		return -EINVAL;
 	if (ap_msg->len > ap_msg->bufsize)
 		return -EINVAL;
 
@@ -362,30 +361,31 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
 	 * 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 +393,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 +437,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 +483,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 v2 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:

Pre-existing issues:
- [Critical] Missing minimum length checks on user-supplied CPRB blocks allow uninitialized kernel memory reads, leading to authorization bypass.
--

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

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

This commit replaces CEIL4 macro overflow checks with check_add_overflow()
to prevent undefined behavior and buffer overflow vulnerabilities in
xcrb_msg_to_type6cprb_msgx() and xcrb_msg_to_type6_ep11cprb_msgx(). Size
variables are also converted from signed to unsigned types.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..d2ae58789aa31 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -342,19 +342,18 @@ 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 (check_add_overflow(sizeof(struct type6_hdr), req_cblen, &u))
> +		return -EINVAL;
> +	if (check_add_overflow(u, xcrb->request_data_length, &ap_msg->len))
> +		return -EINVAL;
>  	if (ap_msg->len > ap_msg->bufsize)
>  		return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but is there a missing minimum length check
for xcrb->request_control_blk_length (and xcrb->req_len) here?

If an unprivileged user issues the ZSECSENDCPRB ioctl with an artificially
small request_control_blk_length, the message buffer allocated via kmalloc
is not zero-padded by z_copy_from_user.

Later in drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6cprb_msgx(),
the domain is read from potentially uninitialized memory:

	if (domain)
		*domain = msg->cprbx.domain;

The same applies to xcrb->req_len in xcrb_msg_to_type6_ep11cprb_msgx(),
where the target_id is also read from uninitialized memory:

	if (domain)
		*domain = msg->cprbx.target_id;

If this uninitialized memory happens to match AUTOSEL_DOM (0xFFFF), it
bypasses the authorization check in
drivers/s390/crypto/zcrypt_api.c:_zcrypt_send_cprb():

	if (perms != &ap_perms && domain < AP_DOMAINS) {
		if (ap_msg.flags & AP_MSG_FLAG_ADMIN) {
			if (!test_bit_inv(domain, perms->adm)) {
				rc = -ENODEV;
				goto out;
			}
		} else if ((ap_msg.flags & AP_MSG_FLAG_USAGE) == 0) {

Since 0xFFFF is not less than AP_DOMAINS, the check is bypassed entirely.

Could this allow unauthorized users to execute privileged administrative
commands or access secure cryptographic keys belonging to other domains?

Would it make sense to add a lower bound check for these lengths to prevent
reading uninitialized kernel memory?

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