Re: [PATCH v5 07/13] iio: light: hid-sensor-prox: 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] Lack of synchronization between sensor hub callback removal and concurrent raw event execution leads to a potential use-after-free when the driver unbinds.
--

--- Patch [7]: [PATCH v5 07/13] iio: light: hid-sensor-prox: convert probe and teardown to devm-managed resources ---
commit 7c95d6a6926ff8d97206e6616bea5d2357682664
Author: Sanjay Chitroda <[email protected]>
Subject: iio: light: hid-sensor-prox: 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. HID trigger setup, IIO
registration, and sensor hub callback resource are now managed using devm APIs.
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-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index 5e28f9b7f40a1..355b7b02868ca 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
[ ... ]
> @@ -309,38 +310,14 @@ static int hid_prox_probe(struct platform_device *pdev)
>  	prox_state->callbacks.send_event = prox_proc_event;
>  	prox_state->callbacks.capture_sample = prox_capture_sample;
>  	prox_state->callbacks.pdev = pdev;
> -	ret = sensor_hub_register_callback(hsdev, hsdev->usage,
> -					   &prox_state->callbacks);
> +	ret = devm_sensor_hub_register_callback(dev, hsdev, hsdev->usage,
> +						&prox_state->callbacks);

[Severity: High]
This is a pre-existing issue, but does removing the callback lack
synchronization with concurrent raw events, leading to a potential
use-after-free during driver unbind?

When the driver unbinds, sensor_hub_remove_callback() deletes the callback
under dyn_callback_lock but returns immediately without synchronizing against
ongoing events:

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);

	return 0;
}

Concurrently, if a hardware event triggers, sensor_hub_raw_event() can lookup
the callback and execute its functions while holding pdata->lock:

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
...
		callback = sensor_hub_get_callback(hdev,
				report->field[i]->physical ? report->field[i]->physical :
							     report->field[i]->application,
				report->field[i]->usage[0].collection_index,
				&hsdev, &priv);
		if (!callback) {
			ptr += sz;
			continue;
		}
...
			if (report->field[i]->logical)
				callback->capture_sample(hsdev,
					report->field[i]->logical, sz, ptr,
					callback->pdev);
...
}

Because sensor_hub_remove_callback() doesn't wait for pdata->lock (or
otherwise synchronize with the raw event handler), the devres framework can
proceed to free prox_state while sensor_hub_raw_event() is still executing
and trying to call the now-freed capture_sample() or send_event() function
pointers.

>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
> -		goto error_remove_trigger;
> -	}
> -
> -	ret = iio_device_register(indio_dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "device register failed\n");
> -		goto error_remove_callback;
>  	}
>  
> -	return ret;
[ ... ]

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