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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing bounds checking before reading `pld[offs++]` causes an integer underflow in `pld_len - offs`, bypassing bounds checks and enabling an Out-Of-Bounds (OOB) read and write.
--

--- Patch [4]: [PATCH v9 4/5] s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing ---
commit 750aceaeb85ad5fa9ff8e2614110f89a4c147522
Author: Harald Freudenberger <[email protected]>

s390/zcrypt: Improve EP11 CPRB domain handling with ASN.1 parsing

This commit replaces struct overlays with explicit ASN.1 parsing for EP11
CPRB domain handling. It introduces bounds checking at each parsing step
and adds a helper function to safely write integer values.

> 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

[ ... ]

> @@ -1235,51 +1258,90 @@ static long zcrypt_msgtype6_send_ep11_cprb(bool userspace, struct zcrypt_queue *
>  					   struct ap_message *ap_msg)
>  {

[ ... ]

> +		/* payload length field */
> +		i = asn1_length_decode(pld + offs, pld_len - offs, &u);
> +		if (i < 0)
> +			return -EINVAL;
> +		offs += i;
> +		if (u > pld_len - offs)
> +			return -EINVAL;
> +		/* function tag */
> +		if (pld[offs++] != 0x04)

[Severity: Critical]
If the ASN.1 payload tag and length encoding completely consume the payload
buffer, couldn't offs become equal to pld_len here?

If offs equals pld_len, does this array access read out of bounds and
increment offs past pld_len?

If offs exceeds pld_len, it appears the subsequent pld_len - offs
calculations will underflow to SIZE_MAX. Would this bypass the remaining
bounds checks, such as pld_len - offs >= 3, and potentially allow
asn1_int_encode() to write out of bounds?

Additionally, it looks like a similar pattern exists in
xcrb_msg_to_type6_ep11cprb_msgx() where pld[offs++] is checked for the
function tag without a prior bounds check.

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

[ ... ]

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