Re: [PATCH v3 1/2] HID: sensor: custom: Fix use-after-free in enable_sensor

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

Pre-existing issues:
- [Critical] Incorrect mapping between the sysfs attribute name index and the `sensor_inst->fields` array leads to Heap Out-Of-Bounds reads.
- [High] The `/dev/HID-SENSOR-*` character device operations do not synchronize with device removal, leading to Use-After-Free on `devm`-managed structures.
- [High] The `data_fifo` is freed before the sensor hub interrupt callback is unregistered during device removal, creating a race condition that leads to a Use-After-Free.
- [High] The sensor hub device is never closed on driver removal if it was concurrently enabled, causing a permanent power and reference count leak.
--

--- Patch [1]: [PATCH v3 1/2] HID: sensor: custom: Fix use-after-free in enable_sensor ---

commit d433ad359db5f5f5744d4b3f8a4dc8088af760b6
Author: Haoxiang Li [off-list ref]

HID: sensor: custom: Fix use-after-free in enable_sensor

This commit fixes a use-after-free by altering the initialization and cleanup
order of the sysfs attributes and the enable_sensor group. This prevents
enable_sensor from being accessed before its dependencies are fully
initialized or after they have been freed.

Link: https://sashiko.dev/#/patchset/20260623021950.1736413-1-haoxiang_li2024@163.com?part=1

[Severity: Critical]
This is a pre-existing issue, but is there an out-of-bounds read when parsing
the sysfs strings created by this function?

During initialization, if a HID field is skipped, it is not appended to the
sensor_inst->fields array, keeping sensor_field_count small. However, its
original field index is embedded into the sysfs file name.

When a valid sysfs file is later read, show_value() parses the index from
the filename and uses it directly as an array index:

    if (sscanf(attr->attr.name, "input-%x-%x-%s",
               &index, &usage, name) == 3) {
        input = true;
        field_index = index;
    }
    ...
    value = sensor_inst->fields[field_index].attribute.logical_minimum;

Could this bypass the array bounds and leak memory to userspace?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this leave a
use-after-free window?

If a user holds open the misc device /dev/HID-SENSOR-*, misc_deregister()
does not close existing open file descriptors. Since sensor_inst is
allocated with devm_kzalloc(), it is freed when remove() returns.

Would subsequent calls to hid_sensor_custom_read(), poll(), or
hid_sensor_custom_release() dereference the freed sensor_inst object?

[Severity: High]
This is a pre-existing issue, but is there a race condition here?

The data_fifo is freed by hid_sensor_custom_dev_if_remove() before the
interrupt callback is unregistered by sensor_hub_remove_callback().

If an interrupt occurs while sysfs_remove_group() or
hid_sensor_custom_remove_attributes() is blocking, couldn't the callback
hid_sensor_capture_sample() write incoming data to the freed data_fifo
via kfifo_in()?

[Severity: High]
This is also a pre-existing issue, but does this function leak the sensor hub
reference and power state?

If a user writes '1' to the enable_sensor sysfs attribute,
sensor_hub_device_open() powers on the device and takes a reference. Since
this function tears down the interfaces without checking sensor_inst->enable
and calling sensor_hub_device_close(), does the HID device reference leak
permanently?

-	hid_sensor_custom_remove_attributes(sensor_inst);
+	/* Remove enable_sensor first as it uses fields via power_state/report_state. */
 	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
 			   &enable_sensor_attr_group);
+	hid_sensor_custom_remove_attributes(sensor_inst);
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
 }

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