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

[sashiko-bot@kernel.org]

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