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