| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3c30721a-cc1f-2d4a-d5df-f9cdec0af5e8@linux.intel.com>
Date: Tue, 4 Mar 2025 16:57:07 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Tadeu Marchese <marchese.kdev@...il.com>
cc: Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, Tadeu Marchese <marchese@...com>
Subject: Re: platform/x86/hp-bioscfg: Fix buffer alignment and conversion
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@...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.
Powered by blists - more mailing lists