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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Muhammad Bilal <meatuni001@xxxxxxxxx>
> > ---
> >  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.
>