Re: [PATCH v3 2/2] HID: sensor: custom: Fix field sysfs group cleanup on failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lifetime mismatch between `devm_kzalloc` allocated `sensor_inst` and `miscdevice` file operations leads to a Use-After-Free.
- [High] Race condition in `hid_sensor_custom_remove()` between event callback removal and kfifo destruction allows Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/2] HID: sensor: custom: Fix field sysfs group cleanup on failure ---
commit aa8aaa0518fc58fa29db9643742276c6a56cafe6
Author: Haoxiang Li <haoxiang_li2024@163.com>

HID: sensor: custom: Fix field sysfs group cleanup 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 returns the error without
removing the previously created groups.
    
Add a local unwind path to remove the groups that were already created.
With enable_sensor exposed only after the field attributes are ready,
this path can free sensor_inst->fields without leaving enable_sensor
able to access pointers into that array.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 6b0da2e0e1c93..c2b425afd951b 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
[ ... ]
>  	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 *

[Severity: High]
This is a pre-existing issue, but is there a lifetime mismatch between the
devm_kzalloc() allocated sensor_inst and the miscdevice file operations?

In hid_sensor_custom_probe(), sensor_inst is allocated with device-managed
memory:

    sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
                               GFP_KERNEL);

When the device is unbound, devres will automatically free sensor_inst.
However, misc_deregister() does not revoke existing open file descriptors.

If a user has the character device open, unbinds the device, and then
interacts with the file descriptor, could this lead to a use-after-free?

For example, hid_sensor_custom_read() accesses the freed pointer:

    sensor_inst = container_of(file->private_data,
                               struct hid_sensor_custom, custom_dev);

Similarly, closing the file calls hid_sensor_custom_release() which modifies
the freed memory:

    clear_bit(0, &sensor_inst->misc_opened);

[Severity: High]
This is a pre-existing issue, but is there a race condition in
hid_sensor_custom_remove() during device teardown?

The function first frees the data buffer:

    hid_sensor_custom_dev_if_remove(sensor_inst); // calls kfifo_free()

It removes the callback later in the function:

    sensor_hub_remove_callback(hsdev, hsdev->usage);

If the sensor hub receives an asynchronous hardware event between these two
calls, wouldn't the still-active callback invoke hid_sensor_capture_sample()
and write incoming hardware data to the freed data_fifo?

    kfifo_in(&sensor_inst->data_fifo, (unsigned char *)raw_data,
             raw_len);

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