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

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

New issues:
- [Medium] The ASN.1 parser in `xcrb_msg_to_type6_ep11cprb_msgx()` can read one byte past the initialized user payload and branch on uninitialized memory if a maliciously crafted payload causes `offs` to exactly equal `pld_len`.

Pre-existing issues:
- [Critical] The patch replaces the vulnerable C struct overlay parsing in `xcrb_msg_to_type6_ep11cprb_msgx()` with a dynamic ASN.1 parser, but completely misses `zcrypt_msgtype6_send_ep11_cprb()`. The send function still uses the flawed, hardcoded `struct pld_hdr` overlay to dynamically patch the domain ID in the ASN.1 payload, allowing domain isolation bypass and data corruption.
- [Medium] Up to 3 bytes of uninitialized kernel heap memory can be transmitted to the hardware coprocessor due to unzeroed alignment padding.
--

--- Patch [3]: [PATCH v10 3/5] s390/zcrypt: Improve EP11 CPRB length and overflow checks ---
commit 92cbe0a3862e6fcbf60a93a6091ac8e0146860be
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: missing minimum size
validation, arithmetic overflow in length calculations, and simple
c struct overlay for asn1 payload parsing. Fix by using size_t for
length calculations, adding boundary checks, and doing a very simple
asn1 parsing of the payload up to the function value field.

Cc: [email protected] # 7.1+

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c

[ ... ]

> @@ -492,26 +541,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;
> -	}

[Severity: Medium]
This isn't a bug introduced by this patch, but does this code leave the
CEIL4 alignment padding uninitialized?

When xcrb->req_len is not a multiple of 4, the calculated ap_msg->len
includes the CEIL4 padding, but z_copy_from_user() only copies the exact
requested bytes without zeroing the trailing bytes. Could this leak up to
3 bytes of uninitialized kernel heap memory to the hardware coprocessor?

[ ... ]

> +	/* 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;

[Severity: Medium]
Does this bounds check allow an out-of-bounds read?

If a maliciously crafted payload has tag and length fields that consume
exactly pld_len bytes, the check offs > pld_len evaluates to false (for
example, when both are 6).

The parser then proceeds to the next line:

> +	/* function tag */
> +	if (pld[offs++] != 0x04)

This would read past the end of the user-provided payload buffer. Should
this check ensure there is at least 1 byte remaining before reading the
function tag?

> +		return -EINVAL;
> +	/* function length */
> +	if (offs > pld_len)
> +		return -EINVAL;

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does zcrypt_msgtype6_send_ep11_cprb()
also need to be updated to use the new ASN.1 parser instead of the C struct
overlay?

Looking at zcrypt_msgtype6_send_ep11_cprb() in this file, which was not
modified by the patch:

    payload_hdr = (struct pld_hdr *)((&msg->pld_lenfmt) + lfmt);
    payload_hdr->dom_val = AP_QID_QUEUE(zq->queue->qid);

If a user supplies a valid ASN.1 payload with a 1-byte or 2-byte function
value length, the kernel will patch the domain ID at a fixed offset since
it expects a 4-byte function value.

Could this overwrite the wrong bytes and leave the user-supplied domain ID
intact, potentially bypassing domain isolation when the hardware coprocessor
processes the request?

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