On Sun, 27 Sep 2026, Muhammad Bilal wrote: > hp_get_string_from_buffer() has three problems. > > First, the loop that counts how many characters will need > backslash-escaping uses the same variable as both the accumulator and > the loop bound, so once an escape character is found near the end of > the valid range the loop keeps going past the buffer's true character > count, reading out of bounds. > > Second, the bounds check 'if (*buffer_size < src_size)' runs after > src has already stepped over the 2-byte length prefix, so if > *buffer_size equals src_size, reading src_size bytes reads 2 bytes > past the end of the input buffer. The pointer and remaining size are > then advanced using the escape-inflated count rather than the actual > number of bytes consumed, desyncing subsequent property parsers. > > Third, found during review of the fix for the first two: This kind of notes (how the patch came to be) are not content that should be put into changelog. Generally (there may be some exceptions), just explain what the problem is, not focusing on how it was found. The reporter can be credited with Reported-by or Suggested-by tag depending on which of them is more suitable for the particular case. > utf16s_to_utf8s() takes src by value, so the real UTF-16-to-UTF-8 > conversion it performs never reaches dst. While true from the end result point of view that nothing of the intermediate write remains, "never reaches dst" doesn't sound correct description of what happens here. > A second loop immediately > overwrites dst by re-walking the same src position and copying each > UTF-16 code unit with a truncating cast (dst[i] = *src), escaping > '\\', '\r', '\n', '\t' and turning '"' into '\''. Any character above > 0x7f is truncated to its low byte instead of being encoded, so the > function is ASCII-only in practice. For example, given the > two-character UTF-16 string 'eA' with the first character U+00E9 > (e-acute), Nit, it's actually possible to include non-ascii characters in the changelog. It might be still be useful to retain the textual explanation if something manages to mangle the character despite everything but normally it just works. (My surname has been mangled countless of times, but much less these days. In the early days when git tools were not as mature as today, one could even count how many times it got mangled while getting passed from person/tool to another :-D). > the function currently writes the two raw bytes 0xe9 0x41 > into dst, an invalid, truncated sequence, instead of the three bytes > 0xc3 0xa9 0x41, the correct UTF-8 encoding of U+00E9 followed by 'A'. > > Fix all three by keeping the true character count separate from the > escape-inflated one, checking *buffer_size against the full prefix > plus string length before reading, and replacing the redundant > conversion with a real one: convert into a fixed-size scratch buffer > with utf16s_to_utf8s(), then escape that UTF-8 into dst with > string_escape_mem(). The scratch buffer is sized to MAX_BUFF_SIZE > rather than to src_size, since src_size comes straight from the WMI > buffer and dst_size is MAX_BUFF_SIZE at every caller in this file. It's preferrable, if possible, to fix the problems one by one. If it's strictly impossible for some reason I cannot see, only then combine what has to be combined. This applies also to cases where something new comes up during a review of a patch, if the newly found problem is separate from what the commented patch was fixing, it should appear in own patch because it's logically a separate problem. Normally we aim to do logically minimal patches, and as many patches as needed in a series to fix all logically disjoint problems one by one. This allows more focused (and shorter) changelog per patch, and normally makes review much simpler than one bigger spaghetti change where it's hard for the reviewer to see what relates to which problem. > string_escape_mem() has no flag that escapes only a backslash without > also touching '"', so this depends on a separate patch that adds > ESCAPE_BACKSLASH to string_escape_mem(). Not true anymore because you fixed that in the first patch ;-) so don't write stale information into the subsequent changelogs. There's also no need to tell something like "feature x was added to y" (in case you're tempted to change the wording towards something along those lines). We work in very much present time when writing a changelog, so if the previous patch fixed a problem, we assume in the next patch's changelog the problem no longer exists so there's no need to even mention it. (There may be exceptions in case of very complex, interjoining problems but that's not relevant here.) > ESCAPE_SPACE together with > ESCAPE_BACKSLASH escapes '\\', '\f', '\n', '\r', '\t' and '\v'; '\v' > and '\f' were not escaped before, matching Ilpo's comment that this > was likely an oversight rather than intentional. The existing > substitution of '"' for '\'' is kept as an explicit strreplace() > afterward, since string_escape_mem() has no substitution concept and > switching '"' handling to backslash-escaping was ruled out in review. > > string_escape_mem()'s return value can exceed the destination size > when the input does not fit, so the NUL terminator position is > clamped to dst_size - 1 before use; writing dst[] = 0 > unconditionally would reintroduce a heap OOB write of exactly the > kind this patch fixes. > > Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg") > Cc: stable@vger.kernel.org > Suggested-by: Ilpo Järvinen > Signed-off-by: Muhammad Bilal > --- As this series is small, I suggest you send the next version of the entire series to all interest parties. Those looking at patch 1 would want to know about this change so to understand the use case, therefore make it easy to find for the reviewers by including them. I didn't look much into the diff itself as I believe it gets cleaner once you split the change logically into a series of patches. -- i. > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 78 ++++++++------------ > 1 file changed, 31 insertions(+), 47 deletions(-) > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > index 309634c..82c228c 100644 > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > #include "bioscfg.h" > #include "../../firmware_attributes_class.h" > @@ -55,72 +56,55 @@ int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_ > { > u16 *src = (u16 *)*buffer; > u16 src_size; > - > - u16 size; > - int i; > + u16 orig_size; > + char utf8_buf[MAX_BUFF_SIZE]; > + int utf8_len; > int conv_dst_size; > + int escaped_len; > > if (*buffer_size < sizeof(u16)) > return -EINVAL; > > - src_size = *(src++); > - /* size value in u16 chars */ > - size = src_size / sizeof(u16); > + src_size = *src; > > /* Ensure there is enough space remaining to read and convert > * the string > */ > - if (*buffer_size < src_size) > + if (*buffer_size < sizeof(u16) + src_size) > return -EINVAL; > > - for (i = 0; i < size; i++) > - if (src[i] == '\\' || > - src[i] == '\r' || > - src[i] == '\n' || > - src[i] == '\t') > - size++; > + src++; > + /* size value in u16 chars */ > + orig_size = src_size / sizeof(u16); > > /* > - * Conversion is limited to destination string max number of > - * bytes. > + * Convert from UTF-16 to UTF-8 into a scratch buffer first, then > + * escape the result into dst. utf16s_to_utf8s() is capped at > + * sizeof(utf8_buf) regardless of orig_size: orig_size comes > + * straight from the WMI buffer, and dst_size is MAX_BUFF_SIZE at > + * every caller in this file. > */ > - conv_dst_size = size; > - if (size >= dst_size) > - conv_dst_size = dst_size - 1; > + utf8_len = utf16s_to_utf8s(src, orig_size, UTF16_HOST_ENDIAN, > + utf8_buf, sizeof(utf8_buf)); > + > + conv_dst_size = dst_size - 1; > + escaped_len = string_escape_mem(utf8_buf, utf8_len, dst, conv_dst_size, > + ESCAPE_SPACE | ESCAPE_BACKSLASH, NULL); > + if (escaped_len > conv_dst_size) > + escaped_len = conv_dst_size; > + dst[escaped_len] = 0; > > /* > - * convert from UTF-16 unicode to ASCII > + * string_escape_mem() has no equivalent of the original quote > + * substitution; keep turning '"' into a plain single quote instead > + * of escaping it, to match existing behaviour. > */ > - utf16s_to_utf8s(src, src_size, UTF16_HOST_ENDIAN, dst, conv_dst_size); > - dst[conv_dst_size] = 0; > - > - for (i = 0; i < conv_dst_size; i++) { > - if (*src == '\\' || > - *src == '\r' || > - *src == '\n' || > - *src == '\t') { > - dst[i++] = '\\'; > - if (i == conv_dst_size) > - break; > - } > - > - if (*src == '\r') > - dst[i] = 'r'; > - else if (*src == '\n') > - dst[i] = 'n'; > - else if (*src == '\t') > - dst[i] = 't'; > - else if (*src == '"') > - dst[i] = '\''; > - else > - dst[i] = *src; > - src++; > - } > + strreplace(dst, '"', '\''); > > - *buffer = (u8 *)src; > - *buffer_size -= size * sizeof(u16); > + *buffer += sizeof(u16) + src_size; > + *buffer_size -= sizeof(u16) + src_size; > > - return size; > + return escaped_len; > } > > int hp_get_common_data_from_buffer(u8 **buffer_ptr, u32 *buffer_size, >