Re: [PATCH v2 2/2] s390/zcrypt: Fix out-of-bounds ptr advance in cca_query_crypto_facility()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] crypto: zcrypt: OOB read and underflow in cca_query_crypto_facility()
--

commit 5de45449fe732b12f3310f3ce76a2abecc04ed97
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Fix out-of-bounds ptr advance in cca_query_crypto_facility()

The FQ reply parser code blindly advanced the walk pointer by a length
value read directly from the hardware reply payload without checking
that the advance stayed within the allocated reply buffer.

> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
> index 19909bf43dc91..862947431164f 100644
> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
[ ... ]
> @@ -1673,27 +1674,44 @@ int cca_query_crypto_facility(u16 cardnr, u16 domain,
>  	prepcblk->rpl_parmb = (u8 __user *)ptr;
>  	prepparm = (struct fqrepparm *)ptr;
>  	ptr = prepparm->lvdata;
> +	datalen = parmbsize - 2 * sizeof(u8);
>  
>  	/* check and possibly copy reply rule array */
>  	len = *((u16 *)ptr);
> +	if (len > datalen) {
> +		ZCRYPT_DBF_ERR("%s reply rule array len %u exceeds datalen %zu\n",
> +			       __func__, len, datalen);
> +		rc = -EIO;
> +		goto out;
> +	}
> +	datalen -= sizeof(u16);
> +	ptr += sizeof(u16);
>  	if (len > sizeof(u16)) {
> -		ptr += sizeof(u16);
>  		len -= sizeof(u16);
>  		if (rarray && rarraylen && *rarraylen > 0) {
>  			*rarraylen = (len > *rarraylen ? *rarraylen : len);
>  			memcpy(rarray, ptr, *rarraylen);
>  		}
> +		datalen -= len;
>  		ptr += len;
>  	}
>  	/* check and possible copy reply var array */
>  	len = *((u16 *)ptr);

[Severity: High]
Does this code introduce an out-of-bounds read and subsequent integer
underflow?

If a corrupt or malicious device response sets the first array length (len) to
exactly or almost exactly the remaining reply buffer space (datalen), the
first array parsing will advance ptr to the end of the buffer and reduce
datalen to 0 or 1.

When we reach the second array parsing here, a 16-bit length field is
unconditionally read from ptr before checking if there are at least 2 bytes
remaining in the buffer (datalen >= sizeof(u16)). This could read 2 bytes past
the end of the allocated buffer.

> +	if (len > datalen) {
> +		ZCRYPT_DBF_ERR("%s reply var array len %u exceeds datalen %zu\n",
> +			       __func__, len, datalen);
> +		rc = -EIO;
> +		goto out;
> +	}
> +	datalen -= sizeof(u16);

[Severity: High]
If the out-of-bounds read above results in a value smaller than or equal to
datalen (e.g., 0), the check (len > datalen) is bypassed, and the subtraction
here will blindly consume 2 bytes. This would cause an integer underflow if
datalen was 0 or 1, wrapping the tracking variable to a huge value.

> +	ptr += sizeof(u16);
>  	if (len > sizeof(u16)) {
> -		ptr += sizeof(u16);
>  		len -= sizeof(u16);
>  		if (varray && varraylen && *varraylen > 0) {
>  			*varraylen = (len > *varraylen ? *varraylen : len);
>  			memcpy(varray, ptr, *varraylen);
>  		}
> +		datalen -= len;
>  		ptr += len;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006141245.7558-1-freude@linux.ibm.com?part=2