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

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?

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

[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()?

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