[PATCH 2/2] platform/x86: hp-bioscfg: fix heap OOB read and non-ASCII truncation in hp_get_string_from_buffer()

[Muhammad Bilal <meatuni001@gmail.com>]

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:
utf16s_to_utf8s() takes src by value, so the real UTF-16-to-UTF-8
conversion it performs never reaches dst. 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), 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.

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(). 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[<return value>] = 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 <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 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 <linux/kernel.h>
 #include <linux/printk.h>
 #include <linux/string.h>
+#include <linux/string_helpers.h>
 #include <linux/wmi.h>
 #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,
-- 
2.43.0