Re: [PATCH v1 1/1] s390/zcrypt: Validate length in reply before using it

On 8/20/26 16:20, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The patch fixes an out-of-bounds read in `_ip_cprb_helper()` but misses identical vulnerabilities in `cca_gencipherkey()` and `_ep11_genaeskey()`.
> - [High] Unvalidated length fields in the CPRBX response payload are used to advance a memory pointer, causing severe out-of-bounds reads in `cca_query_crypto_facility()`.
> --
> 
> --- Patch [1]: [PATCH v1 1/1] s390/zcrypt: Validate length in reply before using it ---
> commit 3fcaad86f04a2a9419691d5f1a54cc873f326675
> Author: Holger Dengler <[email protected]>
> 
> s390/zcrypt: Validate length in reply before using it
> 
> The length information in the reply is used to copy the key token to
> the target buffer. An invalid information in t->len of the reply may
> cause an over-read of the target buffer and also a over-write of the
> target buffer. To prevent that, check t->len before using it.
> 
>> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
>> index d4ce6352b5b2e..54b8edd1dfc74 100644
>> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
>> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
>> @@ -1158,8 +1158,21 @@ static int _ip_cprb_helper(u16 cardnr, u16 domain,
>>  
>>  	/* do not check the key here, it may be incomplete */
>>  
>> -	/* copy the vlsc key token back */
>> +	/*
>> +	 * Copy the vlsc key token back.
>> +	 * The available space in the destination (key_token) and the source
>> +	 * (t) buffer is always larger as the valid range of prepparm->kb.len.
>> +	 * Validate t->len ba comparing it with the length information in the
>> +	 * param block of the request (prepparm->kb.len)
>> +	 * The value range of prepparm->kb.len has been checked above.
>> +	 */
>>  	t = (struct cipherkeytoken *)prepparm->kb.tlv1.key_token;
>> +	if (t->len != prepparm->kb.len - 3 * sizeof(uint16_t)) {
>> +		ZCRYPT_DBF_ERR("%s reply with invalid key_token length %u\n",
>> +			       __func__, t->len);
>> +		rc = -EIO;
>> +		goto out;
>> +	}
>>  	memcpy(key_token, t, t->len);
>>  	*key_token_size = t->len;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since you are fixing the
> unvalidated length field from the device reply here, should similar validation
> be added to cca_gencipherkey() and _ep11_genaeskey()?
> 
> In drivers/s390/crypto/zcrypt_ccamisc.c:cca_gencipherkey(), the code reads
> the generated token length directly from the untrusted reply and calculates
> the copy length without checking bounds against the structure size:
> 
>     keybuflen = prepparm->kb.tlv1.len - 2 * sizeof(uint16_t);
>     ...
>     if (keybuf) {
>         if (*keybufsize >= keybuflen)
>             memcpy(keybuf, t, keybuflen);
> 
> Could a malicious or compromised device return an inflated length, causing
> memcpy() to read thousands of bytes out-of-bounds from the cprbmem
> allocation and potentially leak kernel memory to userspace?

The memcpy is only called, if keylen is less equal *keysize (which is
the available space in keybuf). As far as I can see, this prevents any
over-writes of the keybuf buffer, even with a malicious response from
the hardware.

This objection is not relevant.

> A similar unvalidated copy appears in
> drivers/s390/crypto/zcrypt_ep11misc.c:_ep11_genaeskey(), which relies entirely
> on the untrusted device data length:
> 
>     memcpy(keybuf, rep_pl->data, rep_pl->data_len);

In _ep11_genaeskey(), the rep_pl->data_len must be less equal
*keybufsize, otherwise the function returns with an error.

This objection is false.

> [Severity: High]
> This is a pre-existing issue, but does a similar vulnerability exist in
> drivers/s390/crypto/zcrypt_ccamisc.c:cca_query_crypto_facility() where the
> unvalidated length field is used to advance a memory pointer?
> 
> In that function, the code blindly advances ptr using an unchecked length
> provided by the device payload:
> 
>     len = *((u16 *)ptr);
>     if (len > sizeof(u16)) {
>         ...
>         ptr += len;
>     }
> 
> If the hardware returns a corrupt or malicious length, could ptr advance
> far beyond the cprbmem buffer bounds, leading to an out-of-bounds dereference
> on the next iteration or exposing arbitrary kernel memory during the
> subsequent memcpy()?

This objection is under investigation and will be fixed (if required) in
another series.

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler