[PATCH 0/2] platform/x86: hp-bioscfg: fix OOB reads and buffer desynchronization in buffer parsers

[PATCH 0/2] platform/x86: hp-bioscfg: fix OOB reads and buffer desynchronization in buffer parsers

[Muhammad Bilal]

This series addresses out-of-bounds reads and buffer accounting issues
in hp-bioscfg's WMI binary buffer parsers (hp_get_integer_from_buffer()
and hp_get_string_from_buffer()):

1. Patch 1 fixes an out-of-bounds read and buffer size desynchronization
   in hp_get_integer_from_buffer() when reading integers from unaligned
   buffer addresses where PTR_ALIGN introduces padding.

2. Patch 2 fixes two heap out-of-bounds reads (passing byte count instead
   of wchar_t count to utf16s_to_utf8s(), and loop bound expansion in the
   escape-counting loop), a 2-byte under-allocation check, and buffer
   pointer/length desynchronization in hp_get_string_from_buffer().

Tested on HP hardware with CONFIG_KASAN=y.

Muhammad Bilal (2):
  platform/x86: hp-bioscfg: fix OOB read in hp_get_integer_from_buffer() on unaligned input
  platform/x86: hp-bioscfg: fix heap OOB read and buffer desync in hp_get_string_from_buffer()

 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 36 +++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH 1/2] platform/x86: hp-bioscfg: fix OOB read in hp_get_integer_from_buffer() on unaligned input

[Muhammad Bilal]

hp_get_integer_from_buffer() aligns the read pointer before dereferencing
it:

  int *ptr = PTR_ALIGN((int *)*buffer, sizeof(int));

When *buffer is not 4-byte aligned, PTR_ALIGN() advances ptr forward by
1-3 bytes to reach the next aligned address. The bounds check that
follows does not account for that advance:

  if (*buffer_size < sizeof(int))
          return -EINVAL;

This only confirms 4 bytes remain from the original *buffer, not from
the aligned ptr. If *buffer is unaligned and *buffer_size is between 4
and (pad + 3) bytes, *(ptr++) reads up to 3 bytes past the end of the
buffer.

*buffer_size is also under-decremented on every call, aligned or not:

  *buffer_size -= sizeof(int);

*buffer is advanced to the aligned, post-read position, but
*buffer_size only accounts for the 4 bytes of the integer itself, not
the alignment padding skipped to reach it. Each unaligned read leaves
*buffer_size overstating the true remaining space by the pad amount,
an error that compounds across repeated calls against the same buffer
(hp_get_common_data_from_buffer() calls this in a sequence), making
later bounds checks against *buffer_size progressively less reliable.

Compute the padding explicitly, check for it, and account for it when
advancing *buffer_size, so the pointer and the remaining-length count
stay consistent with each other.

Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 0edc6e7cfa9a..32b99a862082 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -39,14 +39,18 @@ struct kobj_attribute common_display_langcode =
 int hp_get_integer_from_buffer(u8 **buffer, u32 *buffer_size, u32 *integer)
 {
 	int *ptr = PTR_ALIGN((int *)*buffer, sizeof(int));
+	u32 pad = (u8 *)ptr - *buffer;
 
-	/* Ensure there is enough space remaining to read the integer */
-	if (*buffer_size < sizeof(int))
+	/*
+	 * Ensure there is enough space remaining to read the integer,
+	 * including any padding PTR_ALIGN() introduced to reach it.
+	 */
+	if (*buffer_size < pad + sizeof(int))
 		return -EINVAL;
 
 	*integer = *(ptr++);
 	*buffer = (u8 *)ptr;
-	*buffer_size -= sizeof(int);
+	*buffer_size -= pad + sizeof(int);
 
 	return 0;
 }
-- 
2.55.0

[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@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 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