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@xxxxxxxxxxxxxxx> 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.
>