[PATCH v11 0/5] Improve code in zcrypt msg type 6 handling

[PATCH v11 0/5] 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().
And more ... all related to the length checks done in these both
functions.

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.
v5: - Yea, sashiko still found out that the min req size needs
      to take the minimal payload header into account. Fixed.
v6: - And finally add padding zeros when there is a gap between
      user space provided message size and rounded message size
      in kernel.
v7: - As requested by Holger, the padding code is now an own patch.
    - As the length checks are now so strict the allocation in the
      both misc files needed adaption to correctly allocate 4 byte
      rounded up message buffers.
    - The EP11 length check has been found to be a mess. The payload
      is asn1 encoded and now the length parses parts of this payload
      to extract the needed fields (function value).
    - Furthermore another function zcrypt_msgtype6_send_ep11_cprb()
      also needed to be adjusted to parse the asn1 payload to patch
      the domain value into the payload.
v8: - Sashiko found out that the cprb free with memory scrub also
      needs adaption when the allocation rounds up to a 4 byte
      boundary. So fixed just this in the first patch of the series. 
v9: - hunk for cprb free with 4 byte boundary for EP11 was missing.
    - use of put_unaligned_be16/32 instead of direct cast and store.
v10: - make the asn1 parsing bullet proof. Sashiko found that under
       some circumstances there could have been an access behind the
       actual payload.
     - Added Cc and Fixes tags.
v11: - Added some Reviewed-by from Holger.
     - Fixed the wrong limit checks with offs >= pld_len mentioned
       by Sashiko.

Harald Freudenberger (5):
  s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code
  s390/zcrypt: Improve CCA CPRB length and overflow checks
  s390/zcrypt: Improve EP11 CPRB length and overflow checks
  s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing
  s390/zcrypt: Pad trailing CCA or EP11 message with zeros

 drivers/s390/crypto/zcrypt_ccamisc.c  |  20 +-
 drivers/s390/crypto/zcrypt_ep11misc.c |  16 +-
 drivers/s390/crypto/zcrypt_msgtype6.c | 366 ++++++++++++++++++--------
 3 files changed, 278 insertions(+), 124 deletions(-)

--
2.43.0

[PATCH v11 1/5] s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code

[Harald Freudenberger]

Both CPRB alloc functions in zcrypt_ccamisc.c and zcrypt_ep11misc.c
did not round up the memory allocation to a multiple of 4 bytes as it
is needed by the zcrypt layer to process the CPRBs.

Now the alloc_and_prep_cprbmem() and alloc_cprbmem() functions
guarantee that the base CPRB struct and a possible parameter block are
aligned to a 4-byte boundary and the backing memory allocation is
rounded up to the next multiple of 4 byte. Also the free_cprbmem() is
updated and scrubs the rounded up amount of memory.

Fixes: 9bdb5f7e8369 ("s390/zcrypt: Introduce cprb mempool for cca misc functions")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
Cc: stable@vger.kernel.org # 6.16+
---
 drivers/s390/crypto/zcrypt_ccamisc.c  | 20 ++++++++++++++------
 drivers/s390/crypto/zcrypt_ep11misc.c | 16 +++++++++++-----
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
index 86d2ee78c9f4..d4ce6352b5b2 100644
--- a/drivers/s390/crypto/zcrypt_ccamisc.c
+++ b/drivers/s390/crypto/zcrypt_ccamisc.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/random.h>
+#include <linux/align.h>
 #include <asm/zcrypt.h>
 #include <asm/pkey.h>
 
@@ -267,6 +268,10 @@ EXPORT_SYMBOL(cca_check_sececckeytoken);
  * block, reply CPRB and reply param block and fill in values
  * for the common fields. Returns 0 on success or errno value
  * on failure.
+ * It is guaranteed that request and a possible param block
+ * are aligned to a 4 byte boundary. Furthermore if a param
+ * block is used, the memory allocated for this is rounded up to
+ * the next multiple of 4 bytes.
  */
 static int alloc_and_prep_cprbmem(size_t paramblen,
 				  u8 **p_cprb_mem,
@@ -275,7 +280,8 @@ static int alloc_and_prep_cprbmem(size_t paramblen,
 				  u32 xflags)
 {
 	u8 *cprbmem = NULL;
-	size_t cprbplusparamblen = sizeof(struct CPRBX) + paramblen;
+	size_t cprbplusparamblen =
+		ALIGN(sizeof(struct CPRBX), 4) + ALIGN(paramblen, 4);
 	size_t len = 2 * cprbplusparamblen;
 	struct CPRBX *preqcblk, *prepcblk;
 
@@ -302,10 +308,10 @@ static int alloc_and_prep_cprbmem(size_t paramblen,
 	memcpy(preqcblk->func_id, "T2", 2);
 	preqcblk->rpl_msgbl = cprbplusparamblen;
 	if (paramblen) {
-		preqcblk->req_parmb =
-			((u8 __user *)preqcblk) + sizeof(struct CPRBX);
-		preqcblk->rpl_parmb =
-			((u8 __user *)prepcblk) + sizeof(struct CPRBX);
+		preqcblk->req_parmb = ((u8 __user *)preqcblk) +
+			ALIGN(sizeof(struct CPRBX), 4);
+		preqcblk->rpl_parmb = ((u8 __user *)prepcblk) +
+			ALIGN(sizeof(struct CPRBX), 4);
 	}
 
 	*p_cprb_mem = cprbmem;
@@ -323,8 +329,10 @@ static int alloc_and_prep_cprbmem(size_t paramblen,
  */
 static void free_cprbmem(void *mem, size_t paramblen, bool scrub, u32 xflags)
 {
+	size_t cprblen = ALIGN(sizeof(struct CPRBX), 4) + ALIGN(paramblen, 4);
+
 	if (mem && scrub)
-		memzero_explicit(mem, 2 * (sizeof(struct CPRBX) + paramblen));
+		memzero_explicit(mem, 2 * cprblen);
 
 	if (xflags & ZCRYPT_XFLAG_NOMEMALLOC)
 		mempool_free(mem, cprb_mempool);
diff --git a/drivers/s390/crypto/zcrypt_ep11misc.c b/drivers/s390/crypto/zcrypt_ep11misc.c
index 3dda9589f2b9..2d900ffc5068 100644
--- a/drivers/s390/crypto/zcrypt_ep11misc.c
+++ b/drivers/s390/crypto/zcrypt_ep11misc.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/random.h>
 #include <linux/slab.h>
+#include <linux/align.h>
 #include <asm/zcrypt.h>
 #include <asm/pkey.h>
 #include <crypto/aes.h>
@@ -355,21 +356,24 @@ EXPORT_SYMBOL(ep11_check_aes_key);
 
 /*
  * Allocate and prepare ep11 cprb plus additional payload.
+ * It is guaranteed that the memory is aligned to a 4 byte boundary.
+ * Furthermore the memory allocation is rounded up to the next
+ * multiple of 4 bytes (with taking the payload_len into account).
  */
 static void *alloc_cprbmem(size_t payload_len, u32 xflags)
 {
-	size_t len = sizeof(struct ep11_cprb) + payload_len;
+	size_t memlen = ALIGN(sizeof(struct ep11_cprb) + payload_len, 4);
 	struct ep11_cprb *cprb = NULL;
 
 	if (xflags & ZCRYPT_XFLAG_NOMEMALLOC) {
-		if (len <= CPRB_MEMPOOL_ITEM_SIZE)
+		if (memlen <= CPRB_MEMPOOL_ITEM_SIZE)
 			cprb = mempool_alloc_preallocated(cprb_mempool);
 	} else {
-		cprb = kmalloc(len, GFP_KERNEL);
+		cprb = kmalloc(memlen, GFP_KERNEL);
 	}
 	if (!cprb)
 		return NULL;
-	memset(cprb, 0, len);
+	memset(cprb, 0, memlen);
 
 	cprb->cprb_len = sizeof(struct ep11_cprb);
 	cprb->cprb_ver_id = 0x04;
@@ -385,8 +389,10 @@ static void *alloc_cprbmem(size_t payload_len, u32 xflags)
  */
 static void free_cprbmem(void *mem, size_t payload_len, bool scrub, u32 xflags)
 {
+	size_t memlen = ALIGN(sizeof(struct ep11_cprb) + payload_len, 4);
+
 	if (mem && scrub)
-		memzero_explicit(mem, sizeof(struct ep11_cprb) + payload_len);
+		memzero_explicit(mem, memlen);
 
 	if (xflags & ZCRYPT_XFLAG_NOMEMALLOC)
 		mempool_free(mem, cprb_mempool);
-- 
2.43.0

[PATCH v11 2/5] 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.

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.

Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.1+
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 79 ++++++++++++---------------
 1 file changed, 36 insertions(+), 43 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 40f72cdf284d..3e19e79d747c 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -342,49 +342,39 @@ 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 */
-	ap_msg->len = sizeof(struct type6_hdr) +
-		CEIL4(xcrb->request_control_blk_length) +
-		xcrb->request_data_length;
+	/* 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;
+	req_sumlen = req_cblen + xcrb->request_data_length;
+	if (req_sumlen > U32_MAX)
+		return -EINVAL;
+	ap_msg->len = sizeof(struct type6_hdr) + req_sumlen;
 	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) ?
+	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 +383,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,8 +394,8 @@ 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 (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
-	    xcrb->request_control_blk_length)
+	/* copy subfunction code into AP msg type 6 function code field */
+	if (msg->cprbx.cprb_len > req_cblen - sizeof(msg->hdr.function_code))
 		return -EINVAL;
 	function_code = ((unsigned char *)&msg->cprbx) + msg->cprbx.cprb_len;
 	memcpy(msg->hdr.function_code, function_code,
@@ -437,10 +427,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

[PATCH v11 3/5] 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.
3. The payload is asn1 encoded but the function just uses a simple c
   struct overlay to access some fields of the payload.

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. Do a very simple
asn1 parsing of the payload up to the function value field.

Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.1+
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 152 +++++++++++++++++++-------
 1 file changed, 113 insertions(+), 39 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 3e19e79d747c..7e1f76c935ee 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
+#include <linux/unaligned.h>
 
 #include "ap_bus.h"
 #include "zcrypt_api.h"
@@ -34,6 +35,9 @@
 #define CEXXC_RESPONSE_TYPE_XCRB 1
 #define CEXXC_RESPONSE_TYPE_EP11 2
 
+/* smallest possible EP11 payload size */
+#define MIN_EP11_PAYLOAD_SIZE 5
+
 MODULE_AUTHOR("IBM Corporation");
 MODULE_DESCRIPTION("Cryptographic Coprocessor (message type 6), " \
 		   "Copyright IBM Corp. 2001, 2023");
@@ -438,12 +442,59 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
 	return 0;
 }
 
+/*
+ * Simple asn1 int reader/decoder helper function
+ * Returns number of bytes processed or < 0 on failure
+ * Only accepts int length values of 1, 2 or 4.
+ */
+static inline int asn1_int_decode(const u8 *buf, size_t intlen, u32 *u)
+{
+	switch (intlen) {
+	case 1:
+		*u = (u32)(*buf);
+		return 1;
+	case 2:
+		*u = (u32)get_unaligned_be16(buf);
+		return 2;
+	case 4:
+		*u = (u32)get_unaligned_be32(buf);
+		return 4;
+	default:
+		return -EINVAL;
+	}
+}
+
+/*
+ * Simple asn1 length parse helper function
+ * Returns number of bytes processed or < 0 on failure
+ * Only accepts length encoded within the length octet
+ * or for long form 1, 2 or 4 octet length bytes.
+ */
+static inline int asn1_length_decode(const u8 *buf, size_t buflen, u32 *u)
+{
+	int i;
+
+	if (buflen < 1)
+		return -EINVAL;
+
+	if (*buf < 128) {
+		*u = (u32)(*buf & 0x7F);
+		return 1;
+	}
+
+	i = *buf & 0x7F;
+	if (--buflen < i)
+		return -EINVAL;
+	i = asn1_int_decode(++buf, i, u);
+
+	return i < 0 ? i : i + 1;
+}
+
 static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap_msg,
 					   struct ep11_urb *xcrb,
 					   unsigned int *fcode,
 					   unsigned int *domain)
 {
-	unsigned int lfmt;
 	static struct type6_hdr static_type6_ep11_hdr = {
 		.type		=  0x06,
 		.rqid		= {0x00, 0x01},
@@ -455,34 +506,32 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap
 	struct {
 		struct type6_hdr hdr;
 		union {
-			struct {
-				struct ep11_cprb cprbx;
-				unsigned char pld_tag;    /* fixed value 0x30 */
-				unsigned char pld_lenfmt; /* length format */
-			} __packed;
+			struct ep11_cprb cprbx;
 			DECLARE_FLEX_ARRAY(u8, userdata);
 		};
 	} __packed * msg = ap_msg->msg;
 
-	struct pld_hdr {
-		unsigned char	func_tag;	/* fixed value 0x4 */
-		unsigned char	func_len;	/* fixed value 0x4 */
-		unsigned int	func_val;	/* function ID	   */
-		unsigned char	dom_tag;	/* fixed value 0x4 */
-		unsigned char	dom_len;	/* fixed value 0x4 */
-		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, pld_len;
+	unsigned char *pld;
+	int offs = 0, i;
+	unsigned int u;
 
-	/* 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) + MIN_EP11_PAYLOAD_SIZE)
+		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;
@@ -491,26 +540,51 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap
 
 	/* Import CPRB data from the ioctl input parameter */
 	if (z_copy_from_user(userspace, msg->userdata,
-			     (char __force __user *)xcrb->req, xcrb->req_len)) {
+			     (char __force __user *)xcrb->req, xcrb->req_len))
 		return -EFAULT;
-	}
 
-	if ((msg->pld_lenfmt & 0x80) == 0x80) { /*ext.len.fmt 2 or 3*/
-		switch (msg->pld_lenfmt & 0x03) {
-		case 1:
-			lfmt = 2;
-			break;
-		case 2:
-			lfmt = 3;
-			break;
-		default:
-			return -EINVAL;
-		}
-	} else {
-		lfmt = 1; /* length format #1 */
-	}
-	payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
-	*fcode = payload_hdr->func_val & 0xFFFF;
+	pld = msg->userdata + sizeof(struct ep11_cprb);
+	pld_len = msg->cprbx.payload_len;
+	if (pld_len != xcrb->req_len - sizeof(struct ep11_cprb))
+		return -EINVAL;
+	/*
+	 * At this point pld_len is always >= MIN_EP11_PAYLOAD_SIZE
+	 * and the smallest supported asn1 payload is:
+	 *   payload tag (1 octet)
+	 *   payload length (1-5 octets)
+	 *   function tag (1 octet)
+	 *   function length (1-5 octets)
+	 *   function value (1-4 octets)
+	 */
+
+	/* payload tag */
+	if (pld[offs++] != 0x30)
+		return -EINVAL;
+	/* payload length field */
+	i = asn1_length_decode(pld + offs, pld_len - offs, &u);
+	if (i < 0)
+		return -EINVAL;
+	offs += i;
+	if (offs >= pld_len || u > pld_len - offs)
+		return -EINVAL;
+	/* function tag */
+	if (pld[offs++] != 0x04)
+		return -EINVAL;
+	/* function length */
+	if (offs >= pld_len)
+		return -EINVAL;
+	i = asn1_length_decode(pld + offs, pld_len - offs, &u);
+	if (i < 0)
+		return -EINVAL;
+	offs += i;
+	if (offs >= pld_len || u > pld_len - offs)
+		return -EINVAL;
+	/* function value */
+	i = asn1_int_decode(pld + offs, u, &u);
+	if (i < 0)
+		return -EINVAL;
+	offs += i;
+	*fcode = 0xFFFF & u;
 
 	/* enable special processing based on the cprbs flags special bit */
 	if (msg->cprbx.flags & 0x20)
-- 
2.43.0

[PATCH v11 4/5] s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing

[Harald Freudenberger]

The zcrypt_msgtype6_send_ep11_cprb() function uses fragile struct
overlays to access and modify the domain field in the EP11 CPRB
payload, creating maintainability and security concerns:
1. Struct overlay approach (pld_hdr) assumes fixed payload structure
   and doesn't validate the actual ASN.1 encoding.
2. Complex length format detection logic is error-prone and doesn't
   properly validate bounds at each parsing step.
3. Direct struct member access bypasses proper ASN.1 validation.

Fix by replacing struct overlays with explicit ASN.1 parsing that
validates each field (payload tag/length, function tag/length/value,
optional domain tag/length/value) with proper bounds checking at every
step. Add asn1_int_encode() helper function to safely write integer
values with correct endianness conversion. This makes the code
consistent with the validation pattern introduced with the rework of
the xcrb_msg_to_type6_ep11cprb_msgx() function.

Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.1+
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 127 +++++++++++++++++++-------
 1 file changed, 96 insertions(+), 31 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 7e1f76c935ee..e79e68b4dd41 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -1226,6 +1226,28 @@ int prep_ep11_ap_msg(bool userspace, struct ep11_urb *xcrb,
 					       func_code, domain);
 }
 
+/*
+ * Simple asn1 int writer/encoder helper function
+ * Returns number of bytes processed or < 0 on failure
+ * Only accepts int length values of 1, 2 or 4.
+ */
+static inline int asn1_int_encode(u8 *buf, size_t intlen, u32 u)
+{
+	switch (intlen) {
+	case 1:
+		*buf = (u8)u;
+		return 1;
+	case 2:
+		put_unaligned_be16((u16)u, buf);
+		return 2;
+	case 4:
+		put_unaligned_be32((u32)u, buf);
+		return 4;
+	default:
+		return -EINVAL;
+	}
+}
+
 /*
  * The request distributor calls this function if it picked the CEX4P
  * device to handle a send_ep11_cprb request.
@@ -1238,51 +1260,94 @@ static long zcrypt_msgtype6_send_ep11_cprb(bool userspace, struct zcrypt_queue *
 					   struct ap_message *ap_msg)
 {
 	int rc;
-	unsigned int lfmt;
 	struct ap_response_type *resp_type = &ap_msg->response;
 	struct {
 		struct type6_hdr hdr;
 		struct ep11_cprb cprbx;
-		unsigned char	pld_tag;	/* fixed value 0x30 */
-		unsigned char	pld_lenfmt;	/* payload length format */
 	} __packed * msg = ap_msg->msg;
-	struct pld_hdr {
-		unsigned char	func_tag;	/* fixed value 0x4 */
-		unsigned char	func_len;	/* fixed value 0x4 */
-		unsigned int	func_val;	/* function ID	   */
-		unsigned char	dom_tag;	/* fixed value 0x4 */
-		unsigned char	dom_len;	/* fixed value 0x4 */
-		unsigned int	dom_val;	/* domain id	   */
-	} __packed * payload_hdr = NULL;
 
 	/*
 	 * The target domain field within the cprb body/payload block will be
 	 * replaced by the usage domain for non-management commands only.
 	 * Therefore we check the first bit of the 'flags' parameter for
 	 * management command indication.
-	 *   0 - non management command
-	 *   1 - management command
 	 */
-	if (!((msg->cprbx.flags & 0x80) == 0x80)) {
-		msg->cprbx.target_id = (unsigned int)
-					AP_QID_QUEUE(zq->queue->qid);
-
-		if ((msg->pld_lenfmt & 0x80) == 0x80) { /*ext.len.fmt 2 or 3*/
-			switch (msg->pld_lenfmt & 0x03) {
-			case 1:
-				lfmt = 2;
-				break;
-			case 2:
-				lfmt = 3;
-				break;
-			default:
+	if (!(msg->cprbx.flags & 0x80)) {
+		int i, offs = 0;
+		size_t pld_len;
+		u8 *pld;
+		u32 u;
+
+		/* update target field in ep11_cprb */
+		msg->cprbx.target_id = (u32)AP_QID_QUEUE(zq->queue->qid);
+
+		/* ptr and length to payload */
+		pld = ap_msg->msg +
+			sizeof(struct type6_hdr) + sizeof(struct ep11_cprb);
+		pld_len = msg->cprbx.payload_len;
+		if (pld_len < MIN_EP11_PAYLOAD_SIZE)
+			return -EINVAL;
+
+		/*
+		 * Parse the asn1 payload, at least we have
+		 *   pld tag (1 octet)
+		 *   payload length (1-5 octets)
+		 *   function tag (1 octet)
+		 *   function length (1-5 octets)
+		 *   function value (1-4 octets)
+		 *   ----- optional fields -----
+		 *   domain tag (1 octet)
+		 *   domain length (1-5 octets)
+		 *   domain value (1-4 octets)
+		 *   ... maybe much more data ...
+		 */
+
+		/* payload tag */
+		if (pld[offs++] != 0x30)
+			return -EINVAL;
+		/* payload length field */
+		i = asn1_length_decode(pld + offs, pld_len - offs, &u);
+		if (i < 0)
+			return -EINVAL;
+		offs += i;
+		if (offs >= pld_len || u > pld_len - offs)
+			return -EINVAL;
+		/* function tag */
+		if (pld[offs++] != 0x04)
+			return -EINVAL;
+		/* function length */
+		if (offs >= pld_len)
+			return -EINVAL;
+		i = asn1_length_decode(pld + offs, pld_len - offs, &u);
+		if (i < 0)
+			return -EINVAL;
+		offs += i;
+		if (u > pld_len - offs)
+			return -EINVAL;
+		/* skip over the function value */
+		offs += u;
+		/* is there some payload left which could hold a domain value ? */
+		if (offs < pld_len && pld_len - offs >= 3) {
+			/* domain tag */
+			if (pld[offs++] != 0x04)
 				return -EINVAL;
-			}
-		} else {
-			lfmt = 1; /* length format #1 */
+			/* domain length */
+			i = asn1_length_decode(pld + offs, pld_len - offs, &u);
+			if (i < 0)
+				return -EINVAL;
+			offs += i;
+			if (offs >= pld_len || u > pld_len - offs)
+				return -EINVAL;
+			/*
+			 * pld[offs] is now at the start of the domain value
+			 * with the value sprawled in u octets.
+			 */
+			i = asn1_int_encode(pld + offs, u,
+					    AP_QID_QUEUE(zq->queue->qid));
+			if (i < 0)
+				return -EINVAL;
+			offs += i;
 		}
-		payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
-		payload_hdr->dom_val = AP_QID_QUEUE(zq->queue->qid);
 	}
 
 	/*
-- 
2.43.0

[PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros

[Harald Freudenberger]

The both functions xcrb_msg_to_type6cprb_msgx() and
xcrb_msg_to_type6_ep11cprb_msgx() copy the user space message into a
kernel buffer based on the message length. But on further processing
the message is supposed to be 4 byte length adjusted. Thus up to 3
bytes of uninitialized kernel memory are forwarded to further
processing steps and may unwanted expose kernel memory to the crypto
card firmware.

This patch contains code to pad the gap between user space copied
message and message buffer length sent down to further processing of
the CCA or EP11 message to zeros.

Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.1+
---
 drivers/s390/crypto/zcrypt_msgtype6.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index e79e68b4dd41..3df1d676de5d 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -398,6 +398,10 @@ 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;
+	/* pad tail with 0 up to req_cblen */
+	if (xcrb->request_control_blk_length < req_cblen)
+		memset(msg->userdata + xcrb->request_control_blk_length,
+		       0, req_cblen - xcrb->request_control_blk_length);
 	/* copy subfunction code into AP msg type 6 function code field */
 	if (msg->cprbx.cprb_len > req_cblen - sizeof(msg->hdr.function_code))
 		return -EINVAL;
@@ -542,6 +546,10 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap
 	if (z_copy_from_user(userspace, msg->userdata,
 			     (char __force __user *)xcrb->req, xcrb->req_len))
 		return -EFAULT;
+	/* pad tail with 0 up to req_len */
+	if (xcrb->req_len < req_len)
+		memset(msg->userdata + xcrb->req_len, 0,
+		       req_len - xcrb->req_len);
 
 	pld = msg->userdata + sizeof(struct ep11_cprb);
 	pld_len = msg->cprbx.payload_len;
-- 
2.43.0

Re: [PATCH v11 4/5] s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing

[sashiko-bot]

> The zcrypt_msgtype6_send_ep11_cprb() function uses fragile struct
> overlays to access and modify the domain field in the EP11 CPRB
> payload, creating maintainability and security concerns:
> 1. Struct overlay approach (pld_hdr) assumes fixed payload structure
>    and doesn't validate the actual ASN.1 encoding.
> 2. Complex length format detection logic is error-prone and doesn't
>    properly validate bounds at each parsing step.
> 3. Direct struct member access bypasses proper ASN.1 validation.
> 
> Fix by replacing struct overlays with explicit ASN.1 parsing that
> validates each field (payload tag/length, function tag/length/value,
> optional domain tag/length/value) with proper bounds checking at every
> step. Add asn1_int_encode() helper function to safely write integer
> values with correct endianness conversion. This makes the code
> consistent with the validation pattern introduced with the rework of
> the xcrb_msg_to_type6_ep11cprb_msgx() function.
> 
> Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v11 1/5] s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code

[sashiko-bot]

> Both CPRB alloc functions in zcrypt_ccamisc.c and zcrypt_ep11misc.c
> did not round up the memory allocation to a multiple of 4 bytes as it
> is needed by the zcrypt layer to process the CPRBs.
> 
> Now the alloc_and_prep_cprbmem() and alloc_cprbmem() functions
> guarantee that the base CPRB struct and a possible parameter block are
> aligned to a 4-byte boundary and the backing memory allocation is
> rounded up to the next multiple of 4 byte. Also the free_cprbmem() is
> updated and scrubs the rounded up amount of memory.
> 
> Fixes: 9bdb5f7e8369 ("s390/zcrypt: Introduce cprb mempool for cca misc functions")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros

[sashiko-bot]

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

Pre-existing issues:
- [Medium] The function `zcrypt_type6_crt_key()` leaves up to 7 bytes of padding uninitialized before sending the key token to the crypto card, leaking kernel memory.
--

--- Patch [5]: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros ---
commit a6bb8fce2d346e386b1306d838ac36f1853d13f8
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Pad trailing CCA or EP11 message with zeros

This commit pads the gap between the message copied from userspace and
the message buffer sent to the CCA/EP11 hardware. This prevents
uninitialized kernel memory from being forwarded to the crypto card.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index e79e68b4dd41d..3df1d676de5d8 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -398,6 +398,10 @@ 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;
> +	/* pad tail with 0 up to req_cblen */
> +	if (xcrb->request_control_blk_length < req_cblen)
> +		memset(msg->userdata + xcrb->request_control_blk_length,
> +		       0, req_cblen - xcrb->request_control_blk_length);

[Severity: Medium]
This is a pre-existing issue, but while reviewing this fix for uninitialized
memory leaks, I noticed a similar issue in zcrypt_type6_crt_key() where
pad_len bytes appear to be left uninitialized:

drivers/s390/crypto/zcrypt_cca_key.h:zcrypt_type6_crt_key() {
    ...
    memset(key, 0, sizeof(*key));
    ...
    if (copy_from_user(key->key_parts, crt->np_prime, long_len) ||
        copy_from_user(key->key_parts + long_len,
                       crt->nq_prime, short_len) ||
        copy_from_user(key->key_parts + long_len + short_len,
                       crt->bp_key, long_len) ||
        copy_from_user(key->key_parts + 2 * long_len + short_len,
                       crt->bq_key, short_len) ||
        copy_from_user(key->key_parts + 2 * long_len + 2 * short_len,
                       crt->u_mult_inv, long_len))
        return -EFAULT;
    memset(key->key_parts + 3 * long_len + 2 * short_len + pad_len,
           0xff, crt->inputdatalength);
    ...
}

Because sizeof(*key) doesn't include the flexible array member key_parts[],
the memory inside key_parts is uninitialized before the copies.

Does the final memset() skip over pad_len bytes by adding it to the offset,
leaving those padding bytes uninitialized before being sent to the firmware?

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

Re: [PATCH v11 3/5] s390/zcrypt: Improve EP11 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] Incomplete Fix / Domain Isolation Bypass. The patch introduces proper ASN.1 parsing in `xcrb_msg_to_type6_ep11cprb_msgx()` to extract the function code, allowing shorter encodings of `func_val` (e.g., 1 byte instead of 4). However, it fails to update the subsequent send function `zcrypt_msgtype6_send_ep11_cprb()`, which still uses a fixed-offset C struct (`struct pld_hdr`) to overwrite the domain ID (`dom_val`) in the payload. Because the EP11 hardware natively parses the ASN.1 payload, an attacker can supply a shorter `func_val` to shift the actual `dom_val` field. The driver will validate the request but overwrite the wrong offset, leaving the attacker-supplied domain ID intact. The hardware will then process the payload using the attacker-controlled domain ID, bypassing cryptographic domain isolation.
--

--- Patch [3]: [PATCH v11 3/5] s390/zcrypt: Improve EP11 CPRB length and overflow checks ---
commit ded2aaf4bb041722a4604d826ab3b2f1e7bf5794
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve EP11 CPRB length and overflow checks

This commit fixes missing minimum size validation and arithmetic overflows
in xcrb_msg_to_type6_ep11cprb_msgx(). It introduces proper size checks and a
simple ASN.1 parsing mechanism for the payload up to the function value
field.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 3e19e79d747cf..7e1f76c935ee1 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -491,26 +540,51 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap

[ ... ]

> +	/* function length */
> +	if (offs >= pld_len)
> +		return -EINVAL;
> +	i = asn1_length_decode(pld + offs, pld_len - offs, &u);
> +	if (i < 0)
> +		return -EINVAL;
> +	offs += i;
> +	if (offs >= pld_len || u > pld_len - offs)
> +		return -EINVAL;
> +	/* function value */
> +	i = asn1_int_decode(pld + offs, u, &u);
> +	if (i < 0)
> +		return -EINVAL;
> +	offs += i;
> +	*fcode = 0xFFFF & u;

[Severity: Critical]
This is a pre-existing issue, but this updated parsing exposes a bypass of
the cryptographic domain isolation.

The new ASN.1 logic correctly parses a variable-length payload, allowing a
shorter encoding of func_val (e.g., 1 byte instead of 4). However, how does
the subsequent send function handle this?

Looking at zcrypt_msgtype6_send_ep11_cprb(), it appears to still use a
fixed C-struct offset to overwrite the domain ID:

drivers/s390/crypto/zcrypt_msgtype6.c:zcrypt_msgtype6_send_ep11_cprb() {
        ...
        payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
        payload_hdr->dom_val = AP_QID_QUEUE(zq->queue->qid);
        ...
}

If an unprivileged user issues an EP11 CPRB ioctl with a valid ASN.1 payload
containing a 1-byte function ID, wouldn't the fixed offset in
zcrypt_msgtype6_send_ep11_cprb() miss the actual dom_val field?

It seems this would result in the driver validating the request but
overwriting the wrong offset. This could leave an attacker-supplied domain
ID intact, allowing the EP11 hardware to process the payload using the
wrong domain.

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

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

[sashiko-bot]

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

New issues:
- [High] Out-of-bounds read into uninitialized padding due to relaxed bounds check.

Pre-existing issues:
- [High] Missing minimum size validation for EP11 requests allows uninitialized memory access.
--

--- Patch [2]: [PATCH v11 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks ---
commit 91475dced80fa0f4ff3795e01d4bba627b9d0bfe
Author: Harald Freudenberger <freude@linux.ibm.com>
s390/zcrypt: Improve CCA CPRB length and overflow checks

This commit improves length and overflow validations for CCA CPRB structures.
It updates calculations to use size_t, adds U32_MAX boundary checks, and
enforces minimum control block size requirements.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..3e19e79d747cf 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c

[ ... ]

> @@ -404,8 +394,8 @@ 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 (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
> -	    xcrb->request_control_blk_length)
> +	/* copy subfunction code into AP msg type 6 function code field */
> +	if (msg->cprbx.cprb_len > req_cblen - sizeof(msg->hdr.function_code))
>  		return -EINVAL;
>  	function_code = ((unsigned char *)&msg->cprbx) + msg->cprbx.cprb_len;
>  	memcpy(msg->hdr.function_code, function_code,

[Severity: High]
Does this change allow an out-of-bounds read into uninitialized padding?

The bounds check was relaxed from checking against
request_control_blk_length to checking against the 4-byte aligned req_cblen.

If a user supplies an unaligned xcrb->request_control_blk_length (e.g., 37)
and sets msg->cprbx.cprb_len to point into the uninitialized padding
(e.g., 38), it appears up to 2 bytes of uninitialized kernel heap memory can
be read into msg->hdr.function_code and subsequently sent to the
cryptographic hardware.

[Severity: High]
This is a pre-existing issue, but could we apply similar minimum size
validation to xcrb_msg_to_type6_ep11cprb_msgx() for EP11 requests?

When a user calls the zcrypt ioctl for an EP11 request with an undersized
xcrb->req_len (e.g., 0):

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)) {
    ...
}

User-supplied data is copied using the user-controlled length without
verifying it meets the minimum required size for the struct fields that
are subsequently accessed.

This allows uninitialized heap memory in ap_msg->msg to dictate control flow
and be sent to the cryptographic hardware when reading fields like
msg->pld_lenfmt or msg->cprbx.flags.

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

Re: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros

[Harald Freudenberger]

On 2026-08-03 10:49, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [Medium] The function `zcrypt_type6_crt_key()` leaves up to 7 bytes
> of padding uninitialized before sending the key token to the crypto
> card, leaking kernel memory.
> --
> 
> --- Patch [5]: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11
> message with zeros ---
> commit a6bb8fce2d346e386b1306d838ac36f1853d13f8
> Author: Harald Freudenberger <freude@linux.ibm.com>
> 
> s390/zcrypt: Pad trailing CCA or EP11 message with zeros
> 
> This commit pads the gap between the message copied from userspace and
> the message buffer sent to the CCA/EP11 hardware. This prevents
> uninitialized kernel memory from being forwarded to the crypto card.
> 
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index e79e68b4dd41d..3df1d676de5d8 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
>> @@ -398,6 +398,10 @@ 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;
>> +	/* pad tail with 0 up to req_cblen */
>> +	if (xcrb->request_control_blk_length < req_cblen)
>> +		memset(msg->userdata + xcrb->request_control_blk_length,
>> +		       0, req_cblen - xcrb->request_control_blk_length);
> 
> [Severity: Medium]
> This is a pre-existing issue, but while reviewing this fix for 
> uninitialized
> memory leaks, I noticed a similar issue in zcrypt_type6_crt_key() where
> pad_len bytes appear to be left uninitialized:
> 
> drivers/s390/crypto/zcrypt_cca_key.h:zcrypt_type6_crt_key() {
>     ...
>     memset(key, 0, sizeof(*key));
>     ...
>     if (copy_from_user(key->key_parts, crt->np_prime, long_len) ||
>         copy_from_user(key->key_parts + long_len,
>                        crt->nq_prime, short_len) ||
>         copy_from_user(key->key_parts + long_len + short_len,
>                        crt->bp_key, long_len) ||
>         copy_from_user(key->key_parts + 2 * long_len + short_len,
>                        crt->bq_key, short_len) ||
>         copy_from_user(key->key_parts + 2 * long_len + 2 * short_len,
>                        crt->u_mult_inv, long_len))
>         return -EFAULT;
>     memset(key->key_parts + 3 * long_len + 2 * short_len + pad_len,
>            0xff, crt->inputdatalength);
>     ...
> }
> 
> Because sizeof(*key) doesn't include the flexible array member 
> key_parts[],
> the memory inside key_parts is uninitialized before the copies.
> 
> Does the final memset() skip over pad_len bytes by adding it to the 
> offset,
> leaving those padding bytes uninitialized before being sent to the 
> firmware?

Different issue - will be addressed with another patch.

Re: [PATCH v11 3/5] s390/zcrypt: Improve EP11 CPRB length and overflow checks

[Harald Freudenberger]

On 2026-08-03 10:50, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Incomplete Fix / Domain Isolation Bypass. The patch
> introduces proper ASN.1 parsing in `xcrb_msg_to_type6_ep11cprb_msgx()`
> to extract the function code, allowing shorter encodings of `func_val`
> (e.g., 1 byte instead of 4). However, it fails to update the
> subsequent send function `zcrypt_msgtype6_send_ep11_cprb()`, which
> still uses a fixed-offset C struct (`struct pld_hdr`) to overwrite the
> domain ID (`dom_val`) in the payload. Because the EP11 hardware
> natively parses the ASN.1 payload, an attacker can supply a shorter
> `func_val` to shift the actual `dom_val` field. The driver will
> validate the request but overwrite the wrong offset, leaving the
> attacker-supplied domain ID intact. The hardware will then process the
> payload using the attacker-controlled domain ID, bypassing
> cryptographic domain isolation.
> --
> 
> --- Patch [3]: [PATCH v11 3/5] s390/zcrypt: Improve EP11 CPRB length
> and overflow checks ---
> commit ded2aaf4bb041722a4604d826ab3b2f1e7bf5794
> Author: Harald Freudenberger <freude@linux.ibm.com>
> 
> s390/zcrypt: Improve EP11 CPRB length and overflow checks
> 
> This commit fixes missing minimum size validation and arithmetic 
> overflows
> in xcrb_msg_to_type6_ep11cprb_msgx(). It introduces proper size checks 
> and a
> simple ASN.1 parsing mechanism for the payload up to the function value
> field.
> 
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index 3e19e79d747cf..7e1f76c935ee1 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
>> @@ -491,26 +540,51 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool 
>> userspace, struct ap_message *ap
> 
> [ ... ]
> 
>> +	/* function length */
>> +	if (offs >= pld_len)
>> +		return -EINVAL;
>> +	i = asn1_length_decode(pld + offs, pld_len - offs, &u);
>> +	if (i < 0)
>> +		return -EINVAL;
>> +	offs += i;
>> +	if (offs >= pld_len || u > pld_len - offs)
>> +		return -EINVAL;
>> +	/* function value */
>> +	i = asn1_int_decode(pld + offs, u, &u);
>> +	if (i < 0)
>> +		return -EINVAL;
>> +	offs += i;
>> +	*fcode = 0xFFFF & u;
> 
> [Severity: Critical]
> This is a pre-existing issue, but this updated parsing exposes a bypass 
> of
> the cryptographic domain isolation.
> 
> The new ASN.1 logic correctly parses a variable-length payload, 
> allowing a
> shorter encoding of func_val (e.g., 1 byte instead of 4). However, how 
> does
> the subsequent send function handle this?
> 
> Looking at zcrypt_msgtype6_send_ep11_cprb(), it appears to still use a
> fixed C-struct offset to overwrite the domain ID:
> 
> drivers/s390/crypto/zcrypt_msgtype6.c:zcrypt_msgtype6_send_ep11_cprb() 
> {
>         ...
>         payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
>         payload_hdr->dom_val = AP_QID_QUEUE(zq->queue->qid);
>         ...
> }
> 
> If an unprivileged user issues an EP11 CPRB ioctl with a valid ASN.1 
> payload
> containing a 1-byte function ID, wouldn't the fixed offset in
> zcrypt_msgtype6_send_ep11_cprb() miss the actual dom_val field?
> 
> It seems this would result in the driver validating the request but
> overwriting the wrong offset. This could leave an attacker-supplied 
> domain
> ID intact, allowing the EP11 hardware to process the payload using the
> wrong domain.

And exactly this is addressed with #4 of this patch series.

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

[Harald Freudenberger]

On 2026-08-03 11:04, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> New issues:
> - [High] Out-of-bounds read into uninitialized padding due to relaxed
> bounds check.
> 
> Pre-existing issues:
> - [High] Missing minimum size validation for EP11 requests allows
> uninitialized memory access.
> --
> 
> --- Patch [2]: [PATCH v11 2/5] s390/zcrypt: Improve CCA CPRB length
> and overflow checks ---
> commit 91475dced80fa0f4ff3795e01d4bba627b9d0bfe
> Author: Harald Freudenberger <freude@linux.ibm.com>
> s390/zcrypt: Improve CCA CPRB length and overflow checks
> 
> This commit improves length and overflow validations for CCA CPRB 
> structures.
> It updates calculations to use size_t, adds U32_MAX boundary checks, 
> and
> enforces minimum control block size requirements.
> 
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index 40f72cdf284d3..3e19e79d747cf 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> 
> [ ... ]
> 
>> @@ -404,8 +394,8 @@ 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 (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
>> -	    xcrb->request_control_blk_length)
>> +	/* copy subfunction code into AP msg type 6 function code field */
>> +	if (msg->cprbx.cprb_len > req_cblen - 
>> sizeof(msg->hdr.function_code))
>>  		return -EINVAL;
>>  	function_code = ((unsigned char *)&msg->cprbx) + 
>> msg->cprbx.cprb_len;
>>  	memcpy(msg->hdr.function_code, function_code,
> 
> [Severity: High]
> Does this change allow an out-of-bounds read into uninitialized 
> padding?
> 
> The bounds check was relaxed from checking against
> request_control_blk_length to checking against the 4-byte aligned 
> req_cblen.
> 
> If a user supplies an unaligned xcrb->request_control_blk_length (e.g., 
> 37)
> and sets msg->cprbx.cprb_len to point into the uninitialized padding
> (e.g., 38), it appears up to 2 bytes of uninitialized kernel heap 
> memory can
> be read into msg->hdr.function_code and subsequently sent to the
> cryptographic hardware.
> 
> [Severity: High]
> This is a pre-existing issue, but could we apply similar minimum size
> validation to xcrb_msg_to_type6_ep11cprb_msgx() for EP11 requests?
> 
> When a user calls the zcrypt ioctl for an EP11 request with an 
> undersized
> xcrb->req_len (e.g., 0):
> 
> 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)) {
>     ...
> }
> 
> User-supplied data is copied using the user-controlled length without
> verifying it meets the minimum required size for the struct fields that
> are subsequently accessed.
> 
> This allows uninitialized heap memory in ap_msg->msg to dictate control 
> flow
> and be sent to the cryptographic hardware when reading fields like
> msg->pld_lenfmt or msg->cprbx.flags.

see #5 of this patch series

Re: [PATCH v11 3/5] s390/zcrypt: Improve EP11 CPRB length and overflow checks

[Holger Dengler]

On 8/3/26 10:33, Harald Freudenberger wrote:
> 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.
> 3. The payload is asn1 encoded but the function just uses a simple c
>    struct overlay to access some fields of the payload.
> 
> 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. Do a very simple
> asn1 parsing of the payload up to the function value field.
> 
> Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Cc: stable@vger.kernel.org # 7.1+

Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler

Re: [PATCH v11 4/5] s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing

[Holger Dengler]

On 8/3/26 10:33, Harald Freudenberger wrote:
> The zcrypt_msgtype6_send_ep11_cprb() function uses fragile struct
> overlays to access and modify the domain field in the EP11 CPRB
> payload, creating maintainability and security concerns:
> 1. Struct overlay approach (pld_hdr) assumes fixed payload structure
>    and doesn't validate the actual ASN.1 encoding.
> 2. Complex length format detection logic is error-prone and doesn't
>    properly validate bounds at each parsing step.
> 3. Direct struct member access bypasses proper ASN.1 validation.
> 
> Fix by replacing struct overlays with explicit ASN.1 parsing that
> validates each field (payload tag/length, function tag/length/value,
> optional domain tag/length/value) with proper bounds checking at every
> step. Add asn1_int_encode() helper function to safely write integer
> values with correct endianness conversion. This makes the code
> consistent with the validation pattern introduced with the rework of
> the xcrb_msg_to_type6_ep11cprb_msgx() function.
> 
> Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within zcrypt device driver")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Cc: stable@vger.kernel.org # 7.1+

Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler