Re: [PATCH v5 08/13] iio: light: hid-sensor-als: convert probe and teardown to devm-managed resources

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

Pre-existing issues:
- [High] Definitive UAF Race Condition: Concurrent execution of sensor_hub_raw_event and driver teardown causes use-after-free on IIO state.
--

--- Patch [8]: [PATCH v5 08/13] iio: light: hid-sensor-als: convert probe and teardown to devm-managed resources ---
commit f3d5af6f98e23ba8f31c7ee56aed1d7b68b69703
Author: Sanjay Chitroda <[email protected]>

iio: light: hid-sensor-als: convert probe and teardown to devm-managed resources

Convert HID IIO driver resource management to use devm-managed helpers,
tying the lifetime of all resources to the device. This simplifies the probe
path by removing goto-based error handling and eliminates the remove callback
entirely.

> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 10bf46ea4f475..633fd15699873 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c

[ ... ]

> @@ -406,37 +407,14 @@ static int hid_als_probe(struct platform_device *pdev)
>  	als_state->callbacks.send_event = als_proc_event;
>  	als_state->callbacks.capture_sample = als_capture_sample;
>  	als_state->callbacks.pdev = pdev;
> -	ret = sensor_hub_register_callback(hsdev, hsdev->usage, &als_state->callbacks);
> +	ret = devm_sensor_hub_register_callback(dev, hsdev, hsdev->usage,
> +						&als_state->callbacks);

[Severity: High]
This is a pre-existing issue, but does this code leave us vulnerable to a
use-after-free during driver unbind?

If a user unbinds the driver via sysfs while the HID sensor hub hardware is
actively sending events on another CPU, devres will trigger
sensor_hub_remove_callback().

The event thread sensor_hub_raw_event() processes HID events and calls
capture_sample() while holding the pdata->lock spinlock. However,
sensor_hub_remove_callback() only acquires dyn_callback_lock and fails to
acquire pdata->lock:

drivers/hid/hid-sensor-hub.c:sensor_hub_remove_callback() {
    ...
    spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
    list_for_each_entry(callback, &pdata->dyn_callback_list, list)
        if (callback->usage_id == usage_id && callback->hsdev == hsdev) {
            list_del(&callback->list);
            kfree(callback);
            break;
        }
    spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
    ...
}

Because the unbind thread does not wait for actively executing event handlers
to finish, it can lead to this sequence:

CPU1 (Event thread)
sensor_hub_raw_event()
    // holding pdata->lock
    if (callback->capture_sample) {
        // paused before or during capture_sample execution

CPU2 (Unbind)
devres cleanup calls sensor_hub_remove_callback()
    list_del(&callback->list);
    kfree(callback);
devres cleanup frees indio_dev (which contains als_state)

CPU1 (Event thread resumes)
        callback->capture_sample(hsdev, ...) -> accesses freed als_state

Can we safely free als_state without ensuring that concurrently executing
event threads have fully completed?

>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
> -		goto error_remove_trigger;
> -	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-28-apr-iio-redundant-argument-v5-temp-v5-0-f1f92c1d830f@gmail.com?part=8