Re: [PATCH v2 1/2] HID: sensor: custom: Remove enable_sensor before freeing fields

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Out-of-bounds array access in `show_value()` and `store_value()` due to flawed sysfs attribute index parsing.
- [High] A race condition in `hid_sensor_custom_remove()` allows incoming sensor events to trigger a Use-After-Free.
- [High] Using `devm_kzalloc` for the driver context linked to a `miscdevice` causes a Use-After-Free if the device is unbound while the file is open.
- [Medium] Concurrent reads on the `miscdevice` corrupt the `kfifo` state, leading to broken data streams.
--

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

HID: sensor: custom: Remove enable_sensor before freeing fields

enable_sensor_store() can call set_power_report_state(), which
dereferences sensor_inst->power_state and sensor_inst->report_state.
These pointers refer to entries in sensor_inst->fields.

hid_sensor_custom_remove() currently frees the field attributes before
removing the enable_sensor sysfs attribute, leaving a window where a
concurrent sysfs write can dereference freed memory.

Remove enable_sensor before freeing the field attributes.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index afffea8940214..d7bdbae96b501 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -1042,9 +1042,9 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
>  	}
>  
>  	hid_sensor_custom_dev_if_remove(sensor_inst);
> -	hid_sensor_custom_remove_attributes(sensor_inst);
>  	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);

[Severity: High]
This is a pre-existing issue, but does freeing the kfifo resource before
unregistering the interrupt-driven callback leave a window for a
use-after-free?

If hid_sensor_custom_dev_if_remove() frees sensor_inst->data_fifo, and an
incoming hardware event occurs before sensor_hub_remove_callback() is called,
could an open file descriptor trigger hid_sensor_capture_sample() to write to
the freed kfifo?

drivers/hid/hid-sensor-custom.c:hid_sensor_capture_sample() {
    ...
    if (kfifo_avail(&sensor_inst->data_fifo) >= required_size) {
        kfifo_in(&sensor_inst->data_fifo, ...);
    ...
}

>  }

[Severity: High]
This isn't a bug introduced by this patch, but it appears using devm_kzalloc()
for the driver context linked to a miscdevice might cause a use-after-free.

If the device is unbound (e.g., via USB unplug), devres frees sensor_inst.
However, if a user process holds the miscdevice file descriptor open, is it
possible for a subsequent call to read() to extract and dereference the freed
pointer?

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_read() {
    sensor_inst = container_of(file->private_data, struct hid_sensor_custom, custom_dev);
    ...
    if (kfifo_is_empty(&sensor_inst->data_fifo)) {
    ...
}

[Severity: Critical]
This is a pre-existing issue, but we might have an out-of-bounds array access
when parsing the attribute name for an index.

In hid_sensor_custom_add_field(), the raw HID loop index is baked into the
sysfs filename:

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_add_field() {
    ...
    snprintf(sensor_field->group_name, sizeof(sensor_field->group_name),
             "input-%x-%x", sensor_field->attribute.index,
             sensor_field->attribute.attrib_id);
    ...
}

If fields are skipped and the array is reallocated to a smaller size, would a
user reading the sysfs file trigger an out-of-bounds access in show_value(),
given it uses the raw index directly from the filename?

drivers/hid/hid-sensor-custom.c:show_value() {
    ...
    if (sscanf(attr->attr.name, "input-%x-%x-%s", &index, &usage, name) == 3) {
        ...
        field_index = index;
        ...
        attribute = &sensor_inst->fields[field_index].attribute;
    ...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but can concurrent readers corrupt
the internal indices by invoking the kfifo consumer API without serialization?

While open() prevents multiple independent opens, a process can fork() or
spawn threads to invoke read() concurrently on the same file descriptor.

Since hid_sensor_custom_read() does not use a mutex around kfifo_to_user(),
will racing threads corrupt the kfifo state?

drivers/hid/hid-sensor-custom.c:hid_sensor_custom_read() {
    ...
    ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count, &copied);
    ...
}

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