diff mbox series

platform/x86/hp-bioscfg: Fix buffer alignment and conversion

Message ID 20250304062319.86270-1-marchese@hp.com (mailing list archive)
State New
Headers show
Series platform/x86/hp-bioscfg: Fix buffer alignment and conversion | expand

Commit Message

Tadeu Marchese March 4, 2025, 6:23 a.m. UTC
Use PTR_ALIGN to access buffer in hp_get_string_from_buffer().
Remove code that escapes characters with backslashes.
Use kstrtouint() for unsigned string-to-integer conversion.
Increase the string_data buffer size by defining MAX_STRING_BUFF_SIZE.

Signed-off-by: Tadeu Marchese <marchese@hp.com>
---
This patch ensures proper alignment when reading the string size from the 
buffer and simplifies Unicode-to-UTF-8 conversion by removing 
unnecessary character escaping.

Issues fixed at the module initialization:
[  433.823905] hp_bioscfg: Prerequisites size value exceeded the maximum 
  number of elements supported or data may be malformed
[  433.837747] Unable to convert string to integer: 4294967295

The buffer size was too small for string attributes such as 
'HP_Disk0MapForUefiBootOrder'.

 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c  | 82 ++++++-------------
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.h  |  6 +-
 .../x86/hp/hp-bioscfg/int-attributes.c        |  8 +-
 3 files changed, 33 insertions(+), 63 deletions(-)
diff mbox series

Patch

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 0b277b7e37dd..b537fbaac15e 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -51,19 +51,21 @@  int hp_get_integer_from_buffer(u8 **buffer, u32 *buffer_size, u32 *integer)
 
 int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_size)
 {
-	u16 *src = (u16 *)*buffer;
+	u16 *src = PTR_ALIGN((u16 *)*buffer, sizeof(u16));
 	u16 src_size;
 
-	u16 size;
-	int i;
+	u16 length;
 	int conv_dst_size;
+	int result;
 
 	if (*buffer_size < sizeof(u16))
 		return -EINVAL;
 
+	/* String size is in the first two bytes of the buffer */
 	src_size = *(src++);
-	/* size value in u16 chars */
-	size = src_size / sizeof(u16);
+
+	/* Get the string length considering it is u16 */
+	length = src_size / sizeof(u16);
 
 	/* Ensure there is enough space remaining to read and convert
 	 * the string
@@ -71,54 +73,22 @@  int hp_get_string_from_buffer(u8 **buffer, u32 *buffer_size, char *dst, u32 dst_
 	if (*buffer_size < src_size)
 		return -EINVAL;
 
-	for (i = 0; i < size; i++)
-		if (src[i] == '\\' ||
-		    src[i] == '\r' ||
-		    src[i] == '\n' ||
-		    src[i] == '\t')
-			size++;
-
-	/*
-	 * Conversion is limited to destination string max number of
-	 * bytes.
-	 */
-	conv_dst_size = size;
-	if (size > dst_size)
-		conv_dst_size = dst_size - 1;
+	conv_dst_size = length;
+	if (dst_size < conv_dst_size)
+		return -EINVAL;
 
 	/*
-	 * convert from UTF-16 unicode to ASCII
+	 * Convert from UTF-16 unicode to UTF-8 and ensure
+	 * the string is null terminated
 	 */
-	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++;
-	}
+	result = utf16s_to_utf8s(src, src_size, UTF16_HOST_ENDIAN, dst, conv_dst_size);
+	dst[result] = 0;
 
-	*buffer = (u8 *)src;
-	*buffer_size -= size * sizeof(u16);
+	/* Update buffer to point to the next position */
+	*buffer = (u8 *)src + src_size;
+	*buffer_size -= src_size;
 
-	return size;
+	return result;
 }
 
 int hp_get_common_data_from_buffer(u8 **buffer_ptr, u32 *buffer_size,
@@ -999,37 +969,37 @@  static int __init hp_init(void)
 	 */
 	ret = create_attributes_level_sysfs_files();
 	if (ret)
-		pr_debug("Failed to create sysfs level attributes\n");
+		pr_warn("Failed to create sysfs level attributes\n");
 
 	ret = hp_init_bios_attributes(HPWMI_STRING_TYPE, HP_WMI_BIOS_STRING_GUID);
 	if (ret)
-		pr_debug("Failed to populate string type attributes\n");
+		pr_warn("Failed to populate string type attributes\n");
 
 	ret = hp_init_bios_attributes(HPWMI_INTEGER_TYPE, HP_WMI_BIOS_INTEGER_GUID);
 	if (ret)
-		pr_debug("Failed to populate integer type attributes\n");
+		pr_warn("Failed to populate integer type attributes\n");
 
 	ret = hp_init_bios_attributes(HPWMI_ENUMERATION_TYPE, HP_WMI_BIOS_ENUMERATION_GUID);
 	if (ret)
-		pr_debug("Failed to populate enumeration type attributes\n");
+		pr_warn("Failed to populate enumeration type attributes\n");
 
 	ret = hp_init_bios_attributes(HPWMI_ORDERED_LIST_TYPE, HP_WMI_BIOS_ORDERED_LIST_GUID);
 	if (ret)
-		pr_debug("Failed to populate ordered list object type attributes\n");
+		pr_warn("Failed to populate ordered list object type attributes\n");
 
 	ret = hp_init_bios_attributes(HPWMI_PASSWORD_TYPE, HP_WMI_BIOS_PASSWORD_GUID);
 	if (ret)
-		pr_debug("Failed to populate password object type attributes\n");
+		pr_warn("Failed to populate password object type attributes\n");
 
 	bioscfg_drv.spm_data.attr_name_kobj = NULL;
 	ret = hp_add_other_attributes(HPWMI_SECURE_PLATFORM_TYPE);
 	if (ret)
-		pr_debug("Failed to populate secure platform object type attribute\n");
+		pr_warn("Failed to populate secure platform object type attribute\n");
 
 	bioscfg_drv.sure_start_attr_kobj = NULL;
 	ret = hp_add_other_attributes(HPWMI_SURE_START_TYPE);
 	if (ret)
-		pr_debug("Failed to populate sure start object type attribute\n");
+		pr_warn("Failed to populate sure start object type attribute\n");
 
 	return 0;
 
diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
index 3166ef328eba..99a95c709061 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
@@ -17,11 +17,11 @@ 
 
 #define DRIVER_NAME		"hp-bioscfg"
 
+#define MAX_STRING_BUFF_SIZE	1024
 #define MAX_BUFF_SIZE		512
 #define MAX_KEY_MOD_SIZE	256
 #define MAX_PASSWD_SIZE		64
 #define MAX_PREREQUISITES_SIZE	20
-#define MAX_REQ_ELEM_SIZE	128
 #define MAX_VALUES_SIZE		16
 #define MAX_ENCODINGS_SIZE	16
 #define MAX_ELEMENTS_SIZE	16
@@ -131,8 +131,8 @@  struct common_data {
 struct string_data {
 	struct common_data common;
 	struct kobject *attr_name_kobj;
-	u8 current_value[MAX_BUFF_SIZE];
-	u8 new_value[MAX_BUFF_SIZE];
+	u8 current_value[MAX_STRING_BUFF_SIZE];
+	u8 new_value[MAX_STRING_BUFF_SIZE];
 	u32 min_length;
 	u32 max_length;
 };
diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
index 6c7f4d5fa9cb..3e8f99b4174d 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
@@ -38,7 +38,7 @@  static int validate_integer_input(int instance_id, char *buf)
 	if (integer_data->common.is_readonly)
 		return -EIO;
 
-	ret = kstrtoint(buf, 10, &in_val);
+	ret = kstrtouint(buf, 10, &in_val);
 	if (ret < 0)
 		return ret;
 
@@ -55,7 +55,7 @@  static void update_integer_value(int instance_id, char *attr_value)
 	int ret;
 	struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
 
-	ret = kstrtoint(attr_value, 10, &in_val);
+	ret = kstrtouint(attr_value, 10, &in_val);
 	if (ret == 0)
 		integer_data->current_value = in_val;
 	else
@@ -185,7 +185,7 @@  static int hp_populate_integer_elements_from_package(union acpi_object *integer_
 		/* Assign appropriate element value to corresponding field*/
 		switch (eloc) {
 		case VALUE:
-			ret = kstrtoint(str_value, 10, &int_value);
+			ret = kstrtouint(str_value, 10, &int_value);
 			if (ret)
 				continue;
 
@@ -328,7 +328,7 @@  static int hp_populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_
 	integer_data->current_value = 0;
 
 	hp_get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
-	ret = kstrtoint(dst, 10, &integer_data->current_value);
+	ret = kstrtouint(dst, 10, &integer_data->current_value);
 	if (ret)
 		pr_warn("Unable to convert string to integer: %s\n", dst);
 	kfree(dst);