Re: [PATCH 2/2] platform/x86: hp-bioscfg: fix heap OOB read and buffer desync in hp_get_string_from_buffer()

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

On Tue, 25 Aug 2026, Muhammad Bilal wrote:

> hp_get_string_from_buffer() has several buffer boundary and memory
> safety bugs when parsing UTF-16 strings from WMI BIOS buffers:

Can these be fixed separately? It would help review.

> First, the loop that counts how many characters will need backslash-
> escaping uses the same variable as both the accumulator and the loop
> bound:
> 
>   size = src_size / sizeof(u16);
>   ...
>   for (i = 0; i < size; i++)
>           if (src[i] == '\\' || src[i] == '\r' ||
>               src[i] == '\n' || src[i] == '\t')
>                   size++;
> 
> Each escape character found extends size, which is also what i is
> compared against, so the loop keeps going past the buffer's true
> character count once any escape character is seen at or near the end
> of the valid range. Every escape character found causes one additional
> out-of-bounds src[i] read.
> 
> Second, once conv_dst_size is computed, the conversion call passes the
> byte length instead of the character count:
> 
>   utf16s_to_utf8s(src, src_size, UTF16_HOST_ENDIAN, dst, conv_dst_size);
> 
> utf16s_to_utf8s()'s inlen parameter is a count of u16 units: its main
> loop decrements inlen once and advances the source pointer by one
> wchar_t per character consumed. src_size here is a byte count (the
> code's own preceding comment, "size value in u16 chars", computes the
> true character count separately as src_size / sizeof(u16)), so passing
> it directly makes the conversion loop walk up to twice as many u16
> units as the source buffer actually holds whenever maxout does not
> run out first.
> 
> Third, the bounds check 'if (*buffer_size < src_size)' is checked after
> src++ has already stepped over the 2-byte prefix. If *buffer_size equals
> src_size, only src_size - 2 bytes remain, so reading src_size bytes
> reads 2 bytes past the end of the input buffer.
> 
> Finally, at the end of the function, the pointer and remaining buffer
> size are adjusted using the escape-inflated size rather than the actual
> number of input bytes consumed from the WMI buffer (sizeof(u16) +
> src_size), causing the buffer pointer and remaining length to drift out
> of sync for subsequent property parsers.
> 
> Fix these by:
> - Keeping the true, unmodified character count in a separate orig_size
>   variable.
> - Checking *buffer_size against sizeof(u16) + src_size before reading.
> - Accurately advancing *buffer and *buffer_size by sizeof(u16) + src_size.
>
> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 29 +++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 32b99a862082..dd453a9b962f 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -60,6 +60,7 @@ int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_
>  	u16 *src = (u16 *)*buffer;
>  	u16 src_size;
>  
> +	u16 orig_size;
>  	u16 size;
>  	int i;
>  	int conv_dst_size;
> @@ -67,17 +68,16 @@ int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_
>  	if (*buffer_size < sizeof(u16))
>  		return -EINVAL;
>  
> -	src_size = *(src++);
> -	/* size value in u16 chars */
> -	size = src_size / sizeof(u16);
> -
> -	/* Ensure there is enough space remaining to read and convert
> -	 * the string
> -	 */
> -	if (*buffer_size < src_size)
> +	src_size = *src;
> +	if (*buffer_size < sizeof(u16) + src_size)
>  		return -EINVAL;
>  
> -	for (i = 0; i < size; i++)
> +	src++;
> +	/* size value in u16 chars */
> +	orig_size = src_size / sizeof(u16);
> +	size = orig_size;
> +
> +	for (i = 0; i < orig_size; i++)
>  		if (src[i] == '\\' ||
>  		    src[i] == '\r' ||
>  		    src[i] == '\n' ||
> @@ -93,9 +93,12 @@ int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_
>  		conv_dst_size = dst_size - 1;
>  
>  	/*
> -	 * convert from UTF-16 unicode to ASCII
> +	 * Convert from UTF-16 unicode to ASCII. utf16s_to_utf8s() counts
> +	 * its length argument in u16 units, not bytes, so pass the
> +	 * original character count rather than src_size (bytes) or the
> +	 * escape-inflated size.
>  	 */
> -	utf16s_to_utf8s(src, src_size, UTF16_HOST_ENDIAN, dst, conv_dst_size);
> +	utf16s_to_utf8s(src, orig_size, UTF16_HOST_ENDIAN, dst, conv_dst_size);
>  	dst[conv_dst_size] = 0;
>  
>  	for (i = 0; i < conv_dst_size; i++) {
> @@ -121,8 +124,8 @@ int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_
>  		src++;
>  	}
>  
> -	*buffer = (u8 *)src;
> -	*buffer_size -= size * sizeof(u16);
> +	*buffer += sizeof(u16) + src_size;
> +	*buffer_size -= sizeof(u16) + src_size;

I really don't even understand how this function is even supposed to 
work... So lets try to agree on its functionalit first and if that makes 
any sense...

1. Function calculates some lengths

2. Calls utf16s_to_utf8s() to do src -> dst conversion

3. It overwrites dst in a loop by copying from src or escaping the src 
   char.

What is the purpose of step 2 if step 3 overwrites dst? Does this happen 
to work just because ASCII chars in src are <= 0x7f so the copy in step 3 
won't mess _most_ strings up??


-- 
 i.