Re: [PATCH] platform/x86: dell-wmi-sysman: bound enumeration string aggregation

[Ilpo Järvinen]

On Sun, 29 Mar 2026, Pengpeng Hou wrote:

> populate_enum_data() aggregates firmware-provided value-modifier and possible-value strings into fixed 512-byte struct members. The current code bounds each individual source string but then appends every string and separator with raw strcat() and no remaining-space check.
> 
> Switch the aggregation loops to a bounded append helper and reject enumeration packages whose combined strings do not fit in the destination buffers.

Please fold changelog paragraphs to 72 chars.
 
> Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management Driver over WMI for Dell Systems")
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
>  .../dell/dell-wmi-sysman/enum-attributes.c    | 34 +++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/enum-attributes.c b/drivers/platform/x86/dell/dell-wmi-sysman/enum-attributes.c
> index 09996fbdc707..14decb219128 100644
> --- a/drivers/platform/x86/dell/dell-wmi-sysman/enum-attributes.c
> +++ b/drivers/platform/x86/dell/dell-wmi-sysman/enum-attributes.c
> @@ -10,6 +10,26 @@
>  
>  get_instance_id(enumeration);
>  
> +static int append_enum_string(char *dest, const char *src)
> +{
> +	size_t dest_len = strlen(dest);
> +	ssize_t copied;
> +
> +	if (dest_len >= MAX_BUFF)

Doesn't this imply that something is already broken (buggy) if we blew 
past the allocated buffer? If so, I suppose it would warrant 
WARN_ON_ONCE() if something caused such invalid string in dest.

> +		return -EINVAL;
> +
> +	copied = strscpy(dest + dest_len, src, MAX_BUFF - dest_len);
> +	if (copied < 0)
> +		return -EINVAL;
> +
> +	dest_len += copied;
> +	copied = strscpy(dest + dest_len, ";", MAX_BUFF - dest_len);
> +	if (copied < 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>  {
>  	int instance_id = get_enumeration_instance_id(kobj);
> @@ -176,9 +196,10 @@ int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
>  			return -EINVAL;
>  		if (check_property_type(enumeration, next_obj, ACPI_TYPE_STRING))
>  			return -EINVAL;
> -		strcat(wmi_priv.enumeration_data[instance_id].dell_value_modifier,
> -			enumeration_obj[next_obj++].string.pointer);
> -		strcat(wmi_priv.enumeration_data[instance_id].dell_value_modifier, ";");
> +		if (append_enum_string(
> +			    wmi_priv.enumeration_data[instance_id].dell_value_modifier,
> +			    enumeration_obj[next_obj++].string.pointer))
> +			return -EINVAL;
>  	}
>  
>  	if (next_obj >= enum_property_count)
> @@ -193,9 +214,10 @@ int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
>  			return -EINVAL;
>  		if (check_property_type(enumeration, next_obj, ACPI_TYPE_STRING))
>  			return -EINVAL;
> -		strcat(wmi_priv.enumeration_data[instance_id].possible_values,
> -			enumeration_obj[next_obj++].string.pointer);
> -		strcat(wmi_priv.enumeration_data[instance_id].possible_values, ";");
> +		if (append_enum_string(
> +			    wmi_priv.enumeration_data[instance_id].possible_values,
> +			    enumeration_obj[next_obj++].string.pointer))
> +			return -EINVAL;
>  	}
>  
>  	return sysfs_create_group(attr_name_kobj, &enumeration_attr_group);
> 

-- 
 i.