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