Re: [PATCH v7 2/5] s390/sclp: Introduce macro sclp_gds_for_each()

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 23.09.26 um 09:40 schrieb Alexander Egorenkov:
> sclp_find_gds_{sub}vector() does not deal well with malformed event buffers
> consisting of GDS {sub}vectors. This can result in an infinite loop or
> an out-of-bounds memory read. Therefore, abort with NULL if
> * the next GDS header would exceed the given end boundary
> * the length in a GDS header contains an invalid value.
>    A valid length value in a GDS header should be at least as large
>    as the size of the corresponding GDS header (2 or 4 bytes)
>    but also not lead to exceeding the given end boundary.
> 
> Use the new macro in sclp_find_gds_{sub}vector() to iterate over entries
> of a GDS {sub}vector in a safe manner bailing out on the first invalid
> entry.
> 
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
> Suggested-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Fixes: 30c2df51173e ("[S390] sclp: event buffer dissection")
> ---
>   drivers/s390/char/sclp.h | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
> index b31a680e0871..a006d7abfdf3 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) = (void*)(end) - (void*)(start), (v) = (typeof(v))(start);	\
> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\
> +	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)

void * instead of void* for checkpatch

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

add a blank line here and below? checkpatch complains. Same for the next patch.


> +	sclp_gds_for_each(v, n, start, end) {
>   		if (v->gds_id == id)
>   			return v;
> +	}
>   	return NULL;
>   }
>   
> -static inline struct gds_subvector *
> -sclp_find_gds_subvector(void *start, void *end, u8 key)
> +static inline struct gds_subvector *sclp_find_gds_subvector(void *start,
> +							    void *end, u8 key)
>   {
>   	struct gds_subvector *sv;
> -
> -	for (sv = start; (void *) sv < end; sv = (void *) sv + sv->length)
> +	int n;
> +	sclp_gds_for_each(sv, n, start, end) {
>   		if (sv->key == key)
>   			return sv;
> +	}
>   	return NULL;
>   }
>