Re: [PATCH v2 1/2] s390/sclp_ocf: Fix computation of length of GDS values

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Integer underflow when computing GDS subvector payload length leads to an 8-byte out-of-bounds read.

Pre-existing issues:
- [High] Parsing GDS vectors and subvectors without checking for zero length leads to an infinite loop.
- [High] Missing struct size and buffer bounds checking in GDS parsing loops allows out-of-bounds memory reads.
--

--- Patch [1]: [PATCH v2 1/2] s390/sclp_ocf: Fix computation of length of GDS values ---
commit 89f746e9734f7be9cdc7f0e26a7a861dc8653aae
Author: Alexander Egorenkov <[email protected]>

s390/sclp_ocf: Fix computation of length of GDS values
    
This commit attempts to fix a potential invalid read memory access while
extracting the HMC network and the CPC name from event buffers sent by OCF.
It subtracts the GDS header size from the total length to calculate the exact
size of the GDS value.

> diff --git a/drivers/s390/char/sclp_ocf.c b/drivers/s390/char/sclp_ocf.c
> index 35f3a4a08b123..cee4bfa4a48aa 100644
> --- a/drivers/s390/char/sclp_ocf.c
> +++ b/drivers/s390/char/sclp_ocf.c
> @@ -66,13 +66,13 @@ static void sclp_ocf_handler(struct evbuf_header *evbuf)
>  	/* Copy network name and cpc name. */
>  	spin_lock(&sclp_ocf_lock);
>  	if (netid) {
> -		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length);
> +		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length - sizeof(*netid));

[Severity: High]
Does this subtraction safely handle small length values?

If the hardware provides a malformed SCLP event buffer where netid->length is
0 or 1, subtracting sizeof(*netid) (which is 2) will underflow. Since the cast
to size_t makes the result unsigned, this underflows to a large positive value
like SIZE_MAX.

The min() function will then evaluate to OCF_LENGTH_HMC_NETWORK (8), leading
memcpy() to read out-of-bounds bytes from the event buffer.

>  		memcpy(hmc_network, netid + 1, size);
>  		EBCASC(hmc_network, size);
>  		hmc_network[size] = 0;
>  	}
>  	if (cpc) {
> -		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length);
> +		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length - sizeof(*cpc));

[Severity: High]
Could this code also underflow cpc->length?

Similar to the netid block above, if cpc->length is 0 or 1, subtracting
sizeof(*cpc) will underflow to a large unsigned value, bypassing the min()
clamp and causing memcpy() to read 8 bytes out-of-bounds.

>  		memset(cpc_name, 0, OCF_LENGTH_CPC_NAME);
>  		memcpy(cpc_name, cpc + 1, size);
>  	}

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the GDS parsing helpers
used earlier in this path:

drivers/s390/char/sclp.h:sclp_find_gds_subvector() {
    ...
	for (sv = start; (void *) sv < end; sv = (void *) sv + sv->length)
		if (sv->key == key)
			return sv;
    ...
}

Does this parsing loop prevent infinite loops on malformed buffers?

If the buffer contains a subvector where sv->length is 0, the pointer sv will
not advance, resulting in an infinite loop if the key doesn't match.

Additionally, does this condition ensure the full struct and its payload reside
within the buffer? The loop condition ((void *) sv < end) allows entry even if
there is only 1 byte left in the buffer. This means reading sv->length and
sv->key could result in an out-of-bounds memory read. It also doesn't verify
if sv + sv->length exceeds the end boundary.

The same concerns apply to sclp_find_gds_vector().

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