[PATCH] HID: hid-sensor-custom: Fix sysfs group leak on failure

[PATCH] HID: hid-sensor-custom: Fix sysfs group leak on failure

[Haoxiang Li]

hid_sensor_custom_add_attributes() creates one sysfs group for each
custom sensor field. If sysfs_create_group() fails after some groups
have already been created, the function currently breaks out of the
loop and returns the error directly.

Fix this by adding a local unwind path when sysfs_create_group() fails.
The unwind path removes all sysfs groups that were successfully created
before the failure and frees sensor_inst->fields.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
 drivers/hid/hid-sensor-custom.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index afffea894021..cd676516e6b0 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -609,7 +609,7 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
 					 &sensor_inst->fields[i].
 					 hid_custom_attribute_group);
 		if (ret)
-			break;
+			goto err_remove_groups;
 
 		/* For power or report field store indexes */
 		if (sensor_inst->fields[i].attribute.attrib_id ==
@@ -621,6 +621,13 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
 	}
 
 	return ret;
+
+err_remove_groups:
+	while (--i >= 0)
+		sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
+				   &sensor_inst->fields[i].hid_custom_attribute_group);
+	kfree(sensor_inst->fields);
+	return ret;
 }
 
 static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
-- 
2.25.1

Re: [PATCH] HID: hid-sensor-custom: Fix sysfs group leak on failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Freeing `sensor_inst->fields` on `sysfs_create_group()` failure creates a Use-After-Free window because the `enable_sensor` sysfs attribute is already active and its store method uses pointers into the `fields` array.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver teardown path frees `sensor_inst->fields` before removing the `enable_sensor` sysfs attribute, allowing a concurrent Use-After-Free.
--

commit 0c3a9f337fe5155646ac08e2a7d6eae0a3e612f0
Author: Haoxiang Li <haoxiang_li2024@163.com>

HID: hid-sensor-custom: Fix sysfs group leak on failure

hid_sensor_custom_add_attributes() creates one sysfs group for each
custom sensor field. If sysfs_create_group() fails after some groups
have already been created, the function currently breaks out of the
loop and returns the error directly.

Fix this by adding a local unwind path when sysfs_create_group() fails.
The unwind path removes all sysfs groups that were successfully created
before the failure and frees sensor_inst->fields.

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index afffea8940214..cd676516e6b04 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
[ ... ]
> @@ -621,6 +621,13 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
>  	}
>  
>  	return ret;
> +
> +err_remove_groups:
> +	while (--i >= 0)
> +		sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
> +				   &sensor_inst->fields[i].hid_custom_attribute_group);
> +	kfree(sensor_inst->fields);

[Severity: High]
Does freeing sensor_inst->fields here introduce a use-after-free window?

Because hid_sensor_custom_probe() registers the enable_sensor sysfs group
before this function is called, the sysfs file is accessible during this
failure unwind window.

If a concurrent write to enable_sensor triggers enable_sensor_store(), it
will dereference sensor_inst->power_state and sensor_inst->report_state in
set_power_report_state(). Since those pointers point into the freed
sensor_inst->fields array and are not cleared here, this might cause memory
corruption.

> +	return ret;
>  }
>  
>  static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *

[Severity: High]
This isn't a bug introduced by this patch, but the driver teardown path has a
similar issue.

In hid_sensor_custom_remove(), the code calls
hid_sensor_custom_remove_attributes() which unconditionally frees
sensor_inst->fields. Only after this does it call sysfs_remove_group() to
remove the enable_sensor_attr_group.

Could a concurrent sysfs write during this window trigger
enable_sensor_store(), causing it to dereference the dangling
sensor_inst->power_state pointer in set_power_report_state()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623021950.1736413-1-haoxiang_li2024@163.com?part=1