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

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

New issues:
- [High] The `asn1_int_decode` function casts potentially unaligned byte pointers to `u16 *` and `u32 *`, invoking undefined behavior.

Pre-existing issues:
- [Critical] The patch updates `xcrb_msg_to_type6_ep11cprb_msgx()` to flexibly parse ASN.1 payloads but leaves the legacy fixed C-struct overlay (`struct pld_hdr`) in `zcrypt_msgtype6_send_ep11_cprb()` untouched, allowing domain isolation to be bypassed.
--

--- Patch [3]: [PATCH v9 3/5] s390/zcrypt: Improve EP11 CPRB length and overflow checks ---
commit ea4312469ba49e7cc43bf68e3a1d383e305d99b5
Author: Harald Freudenberger <[email protected]>

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.
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.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index fb37e28c82424..c4b5d9b85f7a9 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -439,12 +439,61 @@ 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)(*((u8 *)buf));
> +		return 1;
> +	case 2:
> +		*u = (u32)be16_to_cpup((u16 *)buf);

[Severity: High]
Does this code trigger undefined behavior?

The buf parameter points to an arbitrary offset within an ASN.1 encoded
payload in xcrb_msg_to_type6_ep11cprb_msgx(), which is not guaranteed to
be 2-byte aligned. Casting this unaligned u8 * to u16 * and passing it to
be16_to_cpup() violates C strict alignment rules.

While s390 hardware generally supports scalar unaligned accesses, the
compiler may assume u16 * is properly aligned and emit instructions that
strictly require alignment, potentially causing a kernel panic when parsing
untrusted payloads.

> +		return 2;
> +	case 4:
> +		*u = (u32)be32_to_cpup((u32 *)buf);

[Severity: High]
Does this code also trigger undefined behavior for 4-byte values?

Similar to the 2-byte case above, this casts a potentially unaligned byte
pointer to a u32 *, which could cause an Oops if the compiler emits
alignment-strict instructions.

> +		return 4;
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

> -	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;
> -

[Severity: Critical]
This is a pre-existing issue, but this patch updates
xcrb_msg_to_type6_ep11cprb_msgx() to parse ASN.1 payloads dynamically,
while leaving the legacy fixed C-struct overlay (struct pld_hdr) untouched
in zcrypt_msgtype6_send_ep11_cprb().

Does this allow the cryptographic domain isolation to be bypassed?

The kernel enforces crypto domain isolation by overwriting the dom_val field
in the user-provided EP11 payload with the queue's domain:

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

Because this still uses the hardcoded C-struct which assumes func_val is
exactly 4 bytes, what happens if an unprivileged user crafts an ASN.1
payload where the function ID is encoded in 1 or 2 bytes?

It appears the actual dom_val would be shifted earlier in the payload, and
the kernel would overwrite the wrong bytes at a fixed offset, leaving the
user-controlled domain ID unmodified. This seems to allow an attacker to
execute crypto commands against unauthorized domains.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3