Re: [PATCH 1/2] platform/x86: hp-bioscfg: fix OOB read in hp_get_integer_from_buffer() on unaligned input

[Ilpo Järvinen]

On Tue, 25 Aug 2026, Muhammad Bilal wrote:

> hp_get_integer_from_buffer() aligns the read pointer before dereferencing
> it:
> 
>   int *ptr = PTR_ALIGN((int *)*buffer, sizeof(int));
> 
> When *buffer is not 4-byte aligned, PTR_ALIGN() advances ptr forward by
> 1-3 bytes to reach the next aligned address. The bounds check that
> follows does not account for that advance:
> 
>   if (*buffer_size < sizeof(int))
>           return -EINVAL;
> 
> This only confirms 4 bytes remain from the original *buffer, not from
> the aligned ptr. If *buffer is unaligned and *buffer_size is between 4
> and (pad + 3) bytes, *(ptr++) reads up to 3 bytes past the end of the
> buffer.
> 
> *buffer_size is also under-decremented on every call, aligned or not:
> 
>   *buffer_size -= sizeof(int);
> 
> *buffer is advanced to the aligned, post-read position, but
> *buffer_size only accounts for the 4 bytes of the integer itself, not
> the alignment padding skipped to reach it. Each unaligned read leaves
> *buffer_size overstating the true remaining space by the pad amount,
> an error that compounds across repeated calls against the same buffer
> (hp_get_common_data_from_buffer() calls this in a sequence), making
> later bounds checks against *buffer_size progressively less reliable.
> 
> Compute the padding explicitly, check for it, and account for it when
> advancing *buffer_size, so the pointer and the remaining-length count
> stay consistent with each other.
> 
> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Muhammad Bilal <meatuni001@xxxxxxxxx>
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 0edc6e7cfa9a..32b99a862082 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -39,14 +39,18 @@ struct kobj_attribute common_display_langcode =
>  int hp_get_integer_from_buffer(u8 **buffer, u32 *buffer_size, u32 *integer)
>  {
>  	int *ptr = PTR_ALIGN((int *)*buffer, sizeof(int));
> +	u32 pad = (u8 *)ptr - *buffer;
>  
> -	/* Ensure there is enough space remaining to read the integer */
> -	if (*buffer_size < sizeof(int))
> +	/*
> +	 * Ensure there is enough space remaining to read the integer,
> +	 * including any padding PTR_ALIGN() introduced to reach it.
> +	 */
> +	if (*buffer_size < pad + sizeof(int))
>  		return -EINVAL;
>  
>  	*integer = *(ptr++);
>  	*buffer = (u8 *)ptr;
> -	*buffer_size -= sizeof(int);
> +	*buffer_size -= pad + sizeof(int);

Shouldn't all thse sizeof()s be based on sizeof(*integer) or sizeof(*ptr) 
instead of unbound "int"?

And is type of ptr correct? If it is, this lacks an underflow check?

This driver is such a nightmare... :-( Thanks for helping to clean it up.


-- 
 i.