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

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

[Pengpeng Hou]

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.

Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management Driver over WMI for Dell Systems")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 .../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)
+		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);
-- 
2.50.1 (Apple Git-155)

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@iscas.ac.cn>
> ---
>  .../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.

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

[Pengpeng Hou]

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.

Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management Driver over WMI for Dell Systems")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- fold changelog paragraphs to 72 columns
- use WARN_ON_ONCE() if the destination string is already out of bounds

 .../dell/dell-wmi-sysman/enum-attributes.c    | 32 +++++++++++++++----
 1 file changed, 26 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..11fa6f9f9322 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 (WARN_ON_ONCE(dest_len >= MAX_BUFF))
+		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,9 @@ 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 +213,9 @@ 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);
-- 
2.50.1 (Apple Git-155)

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

[Ilpo Järvinen]

On Wed, 08 Apr 2026 08:38:21 +0800, 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.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: dell-wmi-sysman: bound enumeration string aggregation
      commit: 3c34471c26abc52a37f5ad90949e2e4b8027eb14

--
 i.