Re: [PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sclp: out-of-bounds read in sclp_gds_for_each() due to integer promotion
--

commit f292c3c2b28abf9088d99e5a8af81f738a00c9ee
Author: Alexander Egorenkov <egorenar@linux.ibm.com>

s390/sclp: Introduce macro sclp_gds_for_each()

This commit introduces a macro to iterate safely over GDS {sub}vectors. It
adds bounds checks to avoid infinite loops and out-of-bounds memory accesses
when parsing malformed event buffers.

> diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
> index b31a680e08714..d22003b769f14 100644
> --- a/drivers/s390/char/sclp.h
> +++ b/drivers/s390/char/sclp.h
> @@ -360,25 +360,33 @@ sclp_ascebc_str(char *str, int nr)
>  	(machine_is_vm()) ? ASCEBC(str, nr) : ASCEBC_500(str, nr);
>  }
>  
> -static inline struct gds_vector *
> -sclp_find_gds_vector(void *start, void *end, u16 id)
> +/* Loop over all GDS {sub}vectors in a safe manner. */
> +#define sclp_gds_for_each(v, n, start, end)	\
> +	for ((n) = (end) - (start), (v) = (start);	\
> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\

[Severity: High]
Does this code introduce an out-of-bounds read due to integer promotion?

If a malformed SCLP event buffer is provided by the hardware with an
evbuf->length smaller than the header size, start will be greater than end.
This happens in callers like sclp_ocf_handler() and sclp_tty_receiver():

drivers/s390/char/sclp_ocf.c:sclp_ocf_handler() {
    ...
    v = sclp_find_gds_vector(evbuf + 1, (void *) evbuf + evbuf->length,
                             0x9f00);
    ...
}

In sclp_gds_for_each(), the loop limit n is computed as (end) - (start).
Because callers define n as an int, if start is greater than end, n will be a
negative integer.

The bounds condition (n) >= sizeof(*(v)) compares the signed int n with the
unsigned size_t returned by sizeof. Due to C integer promotion rules, n is
implicitly promoted to an unsigned type, wrapping the negative value to a very
large positive number.

Consequently, the bounds check evaluates to true, and the loop mistakenly
executes (v)->length >= sizeof(*(v)), reading from the out-of-bounds start
pointer.

The original logic ((void *) v < end) handled start > end safely by
immediately skipping the loop.

> +	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)
> +
> +static inline struct gds_vector *sclp_find_gds_vector(void *start,
> +						      void *end, u16 id)
>  {
>  	struct gds_vector *v;
> -
> -	for (v = start; (void *) v < end; v = (void *) v + v->length)
> +	int n;
> +	sclp_gds_for_each(v, n, start, end) {
>  		if (v->gds_id == id)
>  			return v;
> +	}
>  	return NULL;
>  }

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