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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3535 bytes --]

On Sat, 19 Sep 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:

I don't see the explanation telling why things go wrong in the aligned 
case.

> 
>   *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.
> 
> Also switch ptr from "int *" to "u32 *", matching the type of the
> output parameter it's really standing in for, and size everything off
> sizeof(*ptr)/sizeof(*integer) instead of the bare "int" type name.
> 
> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>

This also lacks information whether this problem is actually seen to occur 
on some real device.

-- 
 i.


> ---
> Changes in v2:
>   - Use sizeof(*ptr)/sizeof(*integer) instead of bare sizeof(int), and
>     change ptr from "int *" to "u32 *" to match *integer's type, per
>     Ilpo Järvinen's review.
> 
> Link: https://lore.kernel.org/r/20260824225610.18471-2-meatuni001@gmail.com [v1]
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 309634c..f59957e 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -38,15 +38,19 @@ 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 *ptr = PTR_ALIGN((u32 *)*buffer, sizeof(*ptr));
> +	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(*ptr))
>  		return -EINVAL;
>  
>  	*integer = *(ptr++);
>  	*buffer = (u8 *)ptr;
> -	*buffer_size -= sizeof(int);
> +	*buffer_size -= pad + sizeof(*integer);
>  
>  	return 0;
>  }
>