[PATCH 0/2] platform/x86: hp-bioscfg: fix OOB reads and buffer desynchronization in buffer parsers

[PATCH 0/2] platform/x86: hp-bioscfg: fix OOB reads and buffer desynchronization in buffer parsers

[Muhammad Bilal]

This series addresses out-of-bounds reads and buffer accounting issues
in hp-bioscfg's WMI binary buffer parsers (hp_get_integer_from_buffer()
and hp_get_string_from_buffer()):

1. Patch 1 fixes an out-of-bounds read and buffer size desynchronization
   in hp_get_integer_from_buffer() when reading integers from unaligned
   buffer addresses where PTR_ALIGN introduces padding.

2. Patch 2 fixes two heap out-of-bounds reads (passing byte count instead
   of wchar_t count to utf16s_to_utf8s(), and loop bound expansion in the
   escape-counting loop), a 2-byte under-allocation check, and buffer
   pointer/length desynchronization in hp_get_string_from_buffer().

Tested on HP hardware with CONFIG_KASAN=y.

Muhammad Bilal (2):
  platform/x86: hp-bioscfg: fix OOB read in hp_get_integer_from_buffer() on unaligned input
  platform/x86: hp-bioscfg: fix heap OOB read and buffer desync in hp_get_string_from_buffer()

 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 36 +++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.43.0

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

[Muhammad Bilal]

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@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 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);
 
 	return 0;
 }
-- 
2.55.0

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

[Muhammad Bilal]

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

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;
 
 	return size;
 }
-- 
2.55.0

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@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
>  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.

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

[Ilpo Järvinen]

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.

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

[Muhammad Bilal]

Confirmed, you're right about the redundancy.

utf16s_to_utf8s() takes src by value, so it can't advance the caller's
src pointer. The second loop then restarts from the same position and
overwrites everything the conversion just wrote, using dst[i] = *src,
a raw truncating cast with no UTF-8 encoding. So step 2 is currently
dead work, and the function is ASCII-only in practice: any character
above 0x7f gets truncated to garbage instead of being properly
encoded.

Two ways to fix that, and I'd like your preference before I send
anything more for it:

(a) Keep utf16s_to_utf8s() as the real conversion, and rewrite the
second loop to do escaping as a pass over its UTF-8 output instead of
re-deriving from UTF-16 src.

(b) Drop utf16s_to_utf8s() entirely if ASCII-only was always the
intent, and keep only the manual loop, fixing its bounds instead..


Thanks,
Muhammad

On Fri, Sep 18, 2026 at 7:18 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> 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.
>

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

[Ilpo Järvinen]

On Sat, 19 Sep 2026, Muhammad Bilal wrote:

> Confirmed, you're right about the redundancy.
> 
> utf16s_to_utf8s() takes src by value, so it can't advance the caller's
> src pointer. The second loop then restarts from the same position and
> overwrites everything the conversion just wrote, using dst[i] = *src,
> a raw truncating cast with no UTF-8 encoding. So step 2 is currently
> dead work, and the function is ASCII-only in practice: any character
> above 0x7f gets truncated to garbage instead of being properly
> encoded.
> 
> Two ways to fix that, and I'd like your preference before I send
> anything more for it:
> 
> (a) Keep utf16s_to_utf8s() as the real conversion, and rewrite the
> second loop to do escaping as a pass over its UTF-8 output instead of
> re-deriving from UTF-16 src.

Not exactly this, but somewhere there.

You should not try to build the escaping nor utf-8 parsing/length 
calculation within the driver but use generic code for that.

To give some directions...

There seems to be some escaping function in lib/string_helpers.c but since 
we're dealing with an UTF-8 string here, there might not be a readily 
available function for string inputs/outputs.

escape_space() seems to also cover escaping \v, which wasn't among the 
characters this driver escapes. You might need to check that particular 
character in driver before calling the library's escape funtion though 
I'm more thinking along the lines of not escaping it was an oversight from 
the original submitter (given the questionable quality of this driver to 
begin with).

utf8clen() seems to exists, but is currently in inconvinient place (and 
already duplicated so it should be placed into some header anyway).

> (b) Drop utf16s_to_utf8s() entirely if ASCII-only was always the
> intent, and keep only the manual loop, fixing its bounds instead..

I think ASCII only was not the intent, but it just happens to work in 
many cases which is why this has survived so far.

-- 
 i.

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

[Muhammad Bilal]

That makes sense, thanks for the pointers. Here's what I found and a
concrete proposal.

string_escape_mem() (lib/string_helpers.c) does exactly what the
manual loop is trying to do, safely: it takes a source byte buffer and
length, writes escaped output into dst up to osz without ever writing
past it, and returns the true escaped length so truncation is
detectable by comparing the return value to osz. I checked
escape_passthrough(), the fallback for anything not matched by a flag:
it copies the byte through unchanged. ESCAPE_SPACE covers \n \r \t \v
\f, ESCAPE_SPECIAL covers \\ \a \e and ". All of those are ASCII
values below 0x80, so none of them can collide with a UTF-8
continuation or lead byte (those are always >= 0x80). So running
string_escape_mem() over already-converted UTF-8 output is safe
without needing to be UTF-8-aware itself.

Proposed shape: convert with utf16s_to_utf8s() into a scratch buffer,
then string_escape_mem() that scratch buffer into dst.

char utf8_buf[MAX_BUFF_SIZE];
int utf8_len;
int escaped_len;

utf8_len = utf16s_to_utf8s(src, orig_size, UTF16_HOST_ENDIAN,
utf8_buf, sizeof(utf8_buf));

escaped_len = string_escape_mem(utf8_buf, utf8_len, dst, dst_size - 1,
ESCAPE_SPACE | ESCAPE_SPECIAL, NULL);
dst[escaped_len] = 0;

I want to flag the scratch buffer sizing specifically, since I got
this wrong in my first pass at this reply and want to be upfront about
it. src_size is attacker/firmware controlled (a u16 read straight from
the WMI buffer), so orig_size can be up to ~32767 u16 units, worst
case UTF-8 expansion of that is close to 100KB, nowhere near safe for
a fixed-size stack buffer, and the kernel doesn't allow sizing a stack
array off a runtime value. I checked all three call sites in this
file: dst_size is 512 (MAX_BUFF_SIZE) at every one of them, no
exceptions. So the above caps the conversion at MAX_BUFF_SIZE
regardless of what orig_size claims, same capping discipline the
current code already applies to conv_dst_size, just applied a step
earlier, before the escape-inflated count. Open to a different
constant or a kmalloc'd buffer instead if you'd rather not hardcode a
dependency on MAX_BUFF_SIZE inside this function.

Two more things I want your input on:

1. Quote handling changes. ESCAPE_SPECIAL turns " into \", the current
code turns it into ' instead (no backslash). Switching to the library
means losing that substitution, a visible output change I'd rather
confirm than assume is fine.

2. \v and \f get escaped now, where they weren't before. Matches what
you said about \v being an oversight, just flagging that \f comes
along with it from the same flag, there's no way to get one without
the other from ESCAPE_SPACE.

On utf8clen(): found it, static inline and duplicated in both
fs/unicode/mkutf8data.c and fs/unicode/utf8-norm.c, unusable from a
driver as is. I don't think this fix needs it though,
string_escape_mem() is already byte-transparent to UTF-8 as noted
above. The only thing utf8clen() would buy is guaranteeing a dst_size
truncation never lands mid-character. Given this is short BIOS
attribute strings, I'd lean toward leaving that as a known, minor
limitation for now rather than pulling in a separate cross-subsystem
cleanup patch to fs/unicode just for it. Let me know if you'd rather I
do that properly first.

Thanks,
Muhammad

On Mon, Sep 21, 2026 at 7:29 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Sat, 19 Sep 2026, Muhammad Bilal wrote:
>
> > Confirmed, you're right about the redundancy.
> >
> > utf16s_to_utf8s() takes src by value, so it can't advance the caller's
> > src pointer. The second loop then restarts from the same position and
> > overwrites everything the conversion just wrote, using dst[i] = *src,
> > a raw truncating cast with no UTF-8 encoding. So step 2 is currently
> > dead work, and the function is ASCII-only in practice: any character
> > above 0x7f gets truncated to garbage instead of being properly
> > encoded.
> >
> > Two ways to fix that, and I'd like your preference before I send
> > anything more for it:
> >
> > (a) Keep utf16s_to_utf8s() as the real conversion, and rewrite the
> > second loop to do escaping as a pass over its UTF-8 output instead of
> > re-deriving from UTF-16 src.
>
> Not exactly this, but somewhere there.
>
> You should not try to build the escaping nor utf-8 parsing/length
> calculation within the driver but use generic code for that.
>
> To give some directions...
>
> There seems to be some escaping function in lib/string_helpers.c but since
> we're dealing with an UTF-8 string here, there might not be a readily
> available function for string inputs/outputs.
>
> escape_space() seems to also cover escaping \v, which wasn't among the
> characters this driver escapes. You might need to check that particular
> character in driver before calling the library's escape funtion though
> I'm more thinking along the lines of not escaping it was an oversight from
> the original submitter (given the questionable quality of this driver to
> begin with).
>
> utf8clen() seems to exists, but is currently in inconvinient place (and
> already duplicated so it should be placed into some header anyway).
>
> > (b) Drop utf16s_to_utf8s() entirely if ASCII-only was always the
> > intent, and keep only the manual loop, fixing its bounds instead..
>
> I think ASCII only was not the intent, but it just happens to work in
> many cases which is why this has survived so far.
>
> --
>  i.
>

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

[Ilpo Järvinen]

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

On Tue, 22 Sep 2026, Muhammad Bilal wrote:

> That makes sense, thanks for the pointers. Here's what I found and a
> concrete proposal.
> 
> string_escape_mem() (lib/string_helpers.c) does exactly what the
> manual loop is trying to do, safely: it takes a source byte buffer and
> length, writes escaped output into dst up to osz without ever writing
> past it, and returns the true escaped length so truncation is
> detectable by comparing the return value to osz. I checked
> escape_passthrough(), the fallback for anything not matched by a flag:
> it copies the byte through unchanged. ESCAPE_SPACE covers \n \r \t \v
> \f, ESCAPE_SPECIAL covers \\ \a \e and ".

I think what we don't want is to add other characters beyond \\ from this 
ESCAPE_SPECIAL set, definitely not " I'd way. So one needs to perhaps add 
another flag to have it do only backslash escapes _without_ changing the 
meaning of ESCAPE_SPECIAL.

> All of those are ASCII
> values below 0x80, so none of them can collide with a UTF-8
> continuation or lead byte (those are always >= 0x80). So running
> string_escape_mem() over already-converted UTF-8 output is safe
> without needing to be UTF-8-aware itself.

Yes. I didn't realize this >= 0x80 property earlier.

> Proposed shape: convert with utf16s_to_utf8s() into a scratch buffer,
> then string_escape_mem() that scratch buffer into dst.
> 
> char utf8_buf[MAX_BUFF_SIZE];
> int utf8_len;
> int escaped_len;
> 
> utf8_len = utf16s_to_utf8s(src, orig_size, UTF16_HOST_ENDIAN,
> utf8_buf, sizeof(utf8_buf));
> 
> escaped_len = string_escape_mem(utf8_buf, utf8_len, dst, dst_size - 1,
> ESCAPE_SPACE | ESCAPE_SPECIAL, NULL);
> dst[escaped_len] = 0;
> 
> I want to flag the scratch buffer sizing specifically, since I got
> this wrong in my first pass at this reply and want to be upfront about
> it. src_size is attacker/firmware controlled (a u16 read straight from
> the WMI buffer), so orig_size can be up to ~32767 u16 units, worst
> case UTF-8 expansion of that is close to 100KB, nowhere near safe for
> a fixed-size stack buffer, and the kernel doesn't allow sizing a stack
> array off a runtime value. I checked all three call sites in this
> file: dst_size is 512 (MAX_BUFF_SIZE) at every one of them, no
> exceptions. So the above caps the conversion at MAX_BUFF_SIZE
> regardless of what orig_size claims, same capping discipline the
> current code already applies to conv_dst_size, just applied a step
> earlier, before the escape-inflated count. Open to a different
> constant or a kmalloc'd buffer instead if you'd rather not hardcode a
> dependency on MAX_BUFF_SIZE inside this function.
> 
> Two more things I want your input on:
> 
> 1. Quote handling changes. ESCAPE_SPECIAL turns " into \", the current
> code turns it into ' instead (no backslash). Switching to the library
> means losing that substitution, a visible output change I'd rather
> confirm than assume is fine.
> 
> 2. \v and \f get escaped now, where they weren't before. Matches what
> you said about \v being an oversight,

It might be AI just parroting what I said about it being an oversight, I 
don't know if it was an oversight or not but having these strings contain 
\v in unescaped form doesn't seem very useful, same goes for \f which I 
just didn't remember (I didn't check the code that deeply while writing 
the previous email).

> just flagging that \f comes
> along with it from the same flag, there's no way to get one without
> the other from ESCAPE_SPACE.

I'm arguing ESCAPE_SPACE might work for us, because both \f and \v relate 
to characters that might not have use in realistic characters this 
interface is going to have, BUT I'm definitely not sure of that. But it 
would seem worth a try.

...And not using ESCAPE_SPECIAL but add ESCAPE_BACKSLASH along side with 
ESCAPE_SPECIAL. It's easy to claim that \ always needs escapes when 
there's any escaping going on so the justification for adding it 
separately is there (both ESCAPE_SPACE and ESCAPE_NULL currently produce 
broken output without backslashes getting escaped).

> On utf8clen(): found it, static inline and duplicated in both
> fs/unicode/mkutf8data.c and fs/unicode/utf8-norm.c, unusable from a
> driver as is. I don't think this fix needs it though,
> string_escape_mem() is already byte-transparent to UTF-8 as noted
> above. The only thing utf8clen() would buy is guaranteeing a dst_size
> truncation never lands mid-character. Given this is short BIOS
> attribute strings, I'd lean toward leaving that as a known, minor
> limitation for now rather than pulling in a separate cross-subsystem
> cleanup patch to fs/unicode just for it. Let me know if you'd rather I
> do that properly first.

Yes, since string_escape_mem() likely works, utf8clen() doesn't matter.

-- 
 i.

> 
> Thanks,
> Muhammad
> 
> On Mon, Sep 21, 2026 at 7:29 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Sat, 19 Sep 2026, Muhammad Bilal wrote:
> >
> > > Confirmed, you're right about the redundancy.
> > >
> > > utf16s_to_utf8s() takes src by value, so it can't advance the caller's
> > > src pointer. The second loop then restarts from the same position and
> > > overwrites everything the conversion just wrote, using dst[i] = *src,
> > > a raw truncating cast with no UTF-8 encoding. So step 2 is currently
> > > dead work, and the function is ASCII-only in practice: any character
> > > above 0x7f gets truncated to garbage instead of being properly
> > > encoded.
> > >
> > > Two ways to fix that, and I'd like your preference before I send
> > > anything more for it:
> > >
> > > (a) Keep utf16s_to_utf8s() as the real conversion, and rewrite the
> > > second loop to do escaping as a pass over its UTF-8 output instead of
> > > re-deriving from UTF-16 src.
> >
> > Not exactly this, but somewhere there.
> >
> > You should not try to build the escaping nor utf-8 parsing/length
> > calculation within the driver but use generic code for that.
> >
> > To give some directions...
> >
> > There seems to be some escaping function in lib/string_helpers.c but since
> > we're dealing with an UTF-8 string here, there might not be a readily
> > available function for string inputs/outputs.
> >
> > escape_space() seems to also cover escaping \v, which wasn't among the
> > characters this driver escapes. You might need to check that particular
> > character in driver before calling the library's escape funtion though
> > I'm more thinking along the lines of not escaping it was an oversight from
> > the original submitter (given the questionable quality of this driver to
> > begin with).
> >
> > utf8clen() seems to exists, but is currently in inconvinient place (and
> > already duplicated so it should be placed into some header anyway).
> >
> > > (b) Drop utf16s_to_utf8s() entirely if ASCII-only was always the
> > > intent, and keep only the manual loop, fixing its bounds instead..
> >
> > I think ASCII only was not the intent, but it just happens to work in
> > many cases which is why this has survived so far.
> >
> > --
> >  i.
> >
>