Re: [PATCH v10 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:
- [High] An off-by-one error in bounds checking allows a 1-byte out-of-bounds read when parsing the ASN.1 function tag.
--

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

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

This commit replaces struct overlays in EP11 CPRB domain handling with
explicit ASN.1 parsing to improve security and maintainability. It adds
bounds checking and a new helper function for safe integer encoding.

> 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

[ ... ]

> @@ -1239,51 +1261,94 @@ static long zcrypt_msgtype6_send_ep11_cprb(bool userspace, struct zcrypt_queue *

[ ... ]

> +		/* payload tag */
> +		if (pld[offs++] != 0x30)
> +			return -EINVAL;
> +		/* 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: High]
Does this allow a 1-byte out-of-bounds read of the pld buffer if the ASN.1
payload length field uses a multi-byte encoding that exactly consumes the
remaining pld_len bytes?

If offs exactly equals pld_len, the bounds check offs > pld_len will evaluate
to false. If u is 0, u > pld_len - offs becomes 0 > 0, which is also false.

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

The code would then proceed to read pld[offs++], reading one byte past the
end of the pld buffer.

Should this bounds check use >= instead?

if (offs >= pld_len || u > pld_len - offs)

Additionally, looking at xcrb_msg_to_type6_ep11cprb_msgx() in the same file:

	/* 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;
	/* function tag */
	if (pld[offs++] != 0x04)

This is a pre-existing issue, but does it suffer from the identical bounds
checking flaw?

> +			return -EINVAL;

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