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

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 <[email protected]>

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/[email protected]?part=3