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

On 2026-07-30 16:29, [email protected] wrote:
> 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)

This sounds sane to me. However, will deep check this. But ...why the 
hell tells
me Bob that all my code is good and secure and does not find any 
misaligned memory
access possibilities !?!?

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