[PATCH v4 0/2] Improve code in zcrypt msg type 6 handling

[PATCH v4 0/2] Improve code in zcrypt msg type 6 handling

[Harald Freudenberger]

Rework the limit and overflow checks in the both functions
xcrb_msg_to_type6_ep11cprb_msgx() and xcrb_msg_to_type6cprb_msgx().

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.
v3: - Split into two patches  - one for CCA one for EP11
    - rework upper limit/overflow check 
    - add lower limit check
v4: - tried to address all the remaining complains from sashiko.

Harald Freudenberger (2):
  s390/zcrypt: Improve CCA CPRB length and overflow checks
  s390/zcrypt: Improve EP11 CPRB length and overflow checks

 drivers/s390/crypto/zcrypt_msgtype6.c | 97 ++++++++++++++-------------
 1 file changed, 51 insertions(+), 46 deletions(-)

--
2.43.0

[PATCH v4 1/2] s390/zcrypt: Improve CCA CPRB length and overflow checks

[Harald Freudenberger]

The xcrb_msg_to_type6cprb_msgx() function lacks proper input
validation, creating security vulnerabilities:
1. Integer overflow after CEIL4 alignment: Signed int variables could
   overflow during 4-byte boundary alignment, causing undersized
   buffer allocations or incorrect bounds checking.
2. Missing minimum size validation: The CPRBX structure is copied from
   userspace without verifying sufficient buffer length. Undersized
   buffers cause uninitialized memory access when reading structure
   fields like cprbx.cprb_len and cprbx.domain.
3. Arithmetic overflow in sum calculations: Adding control block and
   data block sizes could overflow, bypassing size checks and enabling
   buffer overflows.
4. Potential kernel memory leak if copy_from_user() only copies
   xcrb->request_control_blk_length bytes but the processing of the
   message works with the rounded up to 4 byte boundary buffer size.

Fix by using size_t for length calculations, adding U32_MAX boundary
checks after alignment, validating minimum control block size before
copying from userspace, and detecting sum calculation overflows. Fix
the possible kernel memory leak by zeroing the trailing bytes.

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

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..2e4aef330b68 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -342,49 +342,40 @@ 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;
+	size_t req_cblen, rep_cblen, req_sumlen, rep_sumlen;
+	char *function_code, *req_data;
 
-	if (CEIL4(xcrb->request_control_blk_length) <
-			xcrb->request_control_blk_length)
-		return -EINVAL; /* overflow after alignment*/
-
-	/* length checks */
+	/* request length and overflow checks */
+	if (xcrb->request_control_blk_length < sizeof(struct CPRBX))
+		return -EINVAL;
+	req_cblen = CEIL4((size_t)xcrb->request_control_blk_length);
+	if (req_cblen > U32_MAX)
+		return -EINVAL;
 	ap_msg->len = sizeof(struct type6_hdr) +
-		CEIL4(xcrb->request_control_blk_length) +
-		xcrb->request_data_length;
+		req_cblen + xcrb->request_data_length;
 	if (ap_msg->len > ap_msg->bufsize)
 		return -EINVAL;
-
-	/*
-	 * 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 = req_cblen + xcrb->request_data_length;
+	if (req_sumlen > U32_MAX)
+		return -EINVAL;
+	if (req_cblen <= xcrb->request_data_length ?
 	    req_sumlen < xcrb->request_data_length :
-	    req_sumlen < CEIL4(xcrb->request_control_blk_length)) {
+	    req_sumlen < req_cblen) {
 		return -EINVAL;
 	}
 
-	if (CEIL4(xcrb->reply_control_blk_length) <
-			xcrb->reply_control_blk_length)
-		return -EINVAL; /* overflow after alignment*/
-
-	/*
-	 * 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)) {
+	/* reply length and overflow checks */
+	if (xcrb->reply_control_blk_length < sizeof(struct CPRBX))
+		return -EINVAL;
+	rep_cblen = CEIL4((size_t)xcrb->reply_control_blk_length);
+	if (rep_cblen > U32_MAX)
+		return -EINVAL;
+	rep_sumlen = rep_cblen + xcrb->reply_data_length;
+	if (rep_sumlen > U32_MAX)
+		return -EINVAL;
+	if (rep_cblen <= xcrb->reply_data_length ?
+	    rep_sumlen < xcrb->reply_data_length :
+	    rep_sumlen < rep_cblen) {
 		return -EINVAL;
 	}
 
@@ -393,7 +384,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;
@@ -404,6 +395,9 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
 			     xcrb->request_control_blk_addr,
 			     xcrb->request_control_blk_length))
 		return -EFAULT;
+	if (xcrb->request_control_blk_length < req_cblen)
+		memset(msg->userdata + xcrb->request_control_blk_length,
+		       0, req_cblen - xcrb->request_control_blk_length);
 	if (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
 	    xcrb->request_control_blk_length)
 		return -EINVAL;
@@ -437,10 +431,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;
 }
-- 
2.43.0

Re: [PATCH v4 1/2] s390/zcrypt: Improve CCA CPRB length and overflow checks

[sashiko-bot]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the patch completely misses identical logic flaws in the EP11 path (`xcrb_msg_to_type6_ep11cprb_msgx`). While the patch successfully fixes critical vulnerabilities in the CCA path, it leaves the kernel vulnerable to a catastrophic kernel panic (DoS), uninitialized kernel memory reads, and an uninitialized memory leak to hardware via the `ZSENDEP11CPRB` ioctl.
--

--- Patch [1]: [PATCH v4 1/2] s390/zcrypt: Improve CCA CPRB length and overflow checks ---
commit 0472132f230811c8d94963dccf05ca137d96d8ec
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve CCA CPRB length and overflow checks

This patch fixes integer overflows, minimum size validation, and potential
kernel memory leaks in xcrb_msg_to_type6cprb_msgx(). It adds U32_MAX
boundary checks, validates minimum control block sizes, and zeros trailing
bytes to prevent leaking uninitialized memory.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..2e4aef330b680 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -342,49 +342,40 @@ 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;
> +	size_t req_cblen, rep_cblen, req_sumlen, rep_sumlen;
> +	char *function_code, *req_data;
>  
> -	if (CEIL4(xcrb->request_control_blk_length) <
> -			xcrb->request_control_blk_length)
> -		return -EINVAL; /* overflow after alignment*/
> -
> -	/* length checks */
> +	/* request length and overflow checks */
> +	if (xcrb->request_control_blk_length < sizeof(struct CPRBX))
> +		return -EINVAL;
> +	req_cblen = CEIL4((size_t)xcrb->request_control_blk_length);

[Severity: Critical]
This is a pre-existing issue, but does xcrb_msg_to_type6_ep11cprb_msgx()
need these same fixes?

It looks like the EP11 path has the same logic flaws that this patch fixes
for the CCA path. For example, if a large req_len is provided, couldn't the
length calculation wrap around and bypass the buffer size check?

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx()
    ...
    ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
    ...

If the buffer size check is bypassed, could z_copy_from_user() fail
access_ok() and zero out the destination buffer with a massive length,
crashing the kernel?

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx()
    ...
    if (z_copy_from_user(userspace, msg->userdata,
                         (char __force __user *)xcrb->req, xcrb->req_len)) {
    ...

Additionally, if req_len is very small, could this lead to uninitialized
memory reads when accessing msg->pld_lenfmt, since there doesn't appear to
be a minimum control block size check?

drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx()
    ...
    if ((msg->pld_lenfmt & 0x80) == 0x80) { /*ext.len.fmt 2 or 3*/
    ...

Finally, if req_len requires padding, does the trailing padding get zeroed
out, or could it leak uninitialized kernel heap memory to the hardware?

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

[PATCH v4 2/2] s390/zcrypt: Improve EP11 CPRB length and overflow checks

[Harald Freudenberger]

The xcrb_msg_to_type6_ep11cprb_msgx() function lacks proper input
validation, creating security vulnerabilities:
1. Missing minimum size validation: The ep11_cprb structure and
   subsequent payload fields (pld_tag, pld_lenfmt) are copied from
   userspace without verifying sufficient buffer length.
2. Arithmetic overflow in length calculations: CEIL4 alignment could
   overflow, bypassing size checks and enabling buffer overflows.

Fix by using size_t for length calculations, adding U32_MAX boundary
checks after alignment, and validating minimum request size and
minimum reply size before copying from userspace.

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

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 2e4aef330b68..6f49165d7732 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -477,16 +477,24 @@ 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*/
+	size_t req_len, rep_len;
 
-	/* length checks */
-	ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
+	/* request length and overflow checks */
+	if (xcrb->req_len < sizeof(struct ep11_cprb) + 2 * sizeof(unsigned char))
+		return -EINVAL;
+	req_len = CEIL4(xcrb->req_len);
+	if (req_len < xcrb->req_len || req_len > U32_MAX)
+		return -EINVAL;
+	ap_msg->len = sizeof(struct type6_hdr) + req_len;
 	if (ap_msg->len > ap_msg->bufsize)
 		return -EINVAL;
 
-	if (CEIL4(xcrb->resp_len) < xcrb->resp_len)
-		return -EINVAL; /* overflow after alignment*/
+	/* reply length and overflow checks */
+	if (xcrb->resp_len < sizeof(struct ep11_cprb))
+		return -EINVAL;
+	rep_len = CEIL4(xcrb->resp_len);
+	if (rep_len < xcrb->resp_len || rep_len > U32_MAX)
+		return -EINVAL;
 
 	/* prepare type6 header */
 	msg->hdr = static_type6_ep11_hdr;
-- 
2.43.0

Re: [PATCH v4 2/2] s390/zcrypt: Improve EP11 CPRB length and overflow checks

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Incomplete minimum length check for EP11 CPRB requests leads to out-of-bounds reads and writes of uninitialized kernel heap memory.
- [Medium] Uninitialized padding bytes are leaked to the AP coprocessor hardware.
--

--- Patch [2]: [PATCH v4 2/2] s390/zcrypt: Improve EP11 CPRB length and overflow checks ---
commit 35795525c75a20e288cf1cb8e4a3621e47212c8d
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve EP11 CPRB length and overflow checks

The xcrb_msg_to_type6_ep11cprb_msgx() function lacks proper input
validation, creating security vulnerabilities:
1. Missing minimum size validation: The ep11_cprb structure and
   subsequent payload fields (pld_tag, pld_lenfmt) are copied from
   userspace without verifying sufficient buffer length.
2. Arithmetic overflow in length calculations: CEIL4 alignment could
   overflow, bypassing size checks and enabling buffer overflows.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 2e4aef330b680..6f49165d7732e 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -477,16 +477,24 @@ 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*/
> +	size_t req_len, rep_len;
>  
> -	/* length checks */
> -	ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
> +	/* request length and overflow checks */
> +	if (xcrb->req_len < sizeof(struct ep11_cprb) + 2 * sizeof(unsigned char))
> +		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this minimum length check ensure
enough data is copied for the payload header?

Later in xcrb_msg_to_type6_ep11cprb_msgx(), payload_hdr->func_val is accessed:

xcrb_msg_to_type6_ep11cprb_msgx() {
    ...
    payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
    *fcode = payload_hdr->func_val & 0xFFFF;
    ...
}

If xcrb->req_len is smaller than the required size for the payload header,
could this result in reading uninitialized kernel heap memory?

Additionally, could this lead to an out-of-bounds write in
zcrypt_msgtype6_send_ep11_cprb() when it assigns payload_hdr->dom_val?

> +	req_len = CEIL4(xcrb->req_len);
> +	if (req_len < xcrb->req_len || req_len > U32_MAX)
> +		return -EINVAL;
> +	ap_msg->len = sizeof(struct type6_hdr) + req_len;

[Severity: Medium]
This isn't a bug introduced by this patch, but do we need to zero out the
padding bytes added by CEIL4()?

If xcrb->req_len is not a multiple of 4, the length is rounded up, but the
padding bytes don't appear to be initialized. Could this leak up to 3 bytes
of uninitialized kernel memory to the AP hardware?

>  	if (ap_msg->len > ap_msg->bufsize)
>  		return -EINVAL;

[ ... ]

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