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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 4 Mar 2025, Tadeu Marchese wrote:

> 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 --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)
>  {

Oh great, thanks for looking into this! I recall raising this custom 
string conversion madness during review but back then the patch got 
applied without my comments being addressed.

> -	u16 *src = (u16 *)*buffer;
> +	u16 *src = PTR_ALIGN((u16 *)*buffer, sizeof(u16));

What happens here if the PTR_ALIGN() actually aligns the pointer by 
skipping over the first character?? Is the string going to get read 
out-of-sync with the true character boundaries??

>  	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");

This patch should be split into a series with each patch doing a single 
thing as you seem to have include changes that look independent of the 
string conversion cleanup.

>  
>  	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);
> 

-- 
 i.