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

On 2026-07-29 18:35, [email protected] wrote:
> 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.
> 

ok, will be covered with v10.

>> +		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;
>> +	}
>> +}
> 
> [ ... ]
> 

ok, will be covered with v10.

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

and patch #4 in this series addresses exactly this