Re: [PATCH 5/9] iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Explicitly calling `iio_device_unregister()` before stopping the hardware data source causes a hard lockup.

Pre-existing issues:
- [High] The driver uses a single global static `struct hid_sensor_hub_callbacks temperature_callbacks` which is modified during `probe()`. If multiple temperature sensors are probed, the `pdev` pointer within this global struct is overwritten by the most recently probed device. This causes events from all sensors to be routed to the last probed sensor's IIO device, leading to data corruption. Furthermore, if a sensor is removed, the remaining sensors will dereference a dangling `pdev` pointer when handling events, causing a Use-After-Free.
- [High] The sensor hub remove callback lacks synchronization with active event processing, leading to use-after-free on module unload or device unbind.
--

commit 40b43e4e8d671ec0d491c5e0ebefd5ffcf97e7ad
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()

This commit switches from devm_iio_device_register() to the non-devm
iio_device_register() to avoid mixing devm APIs with goto-based unwinding.
It adds an explicit iio_device_unregister() call in the teardown path to
ensure deterministic cleanup.

> diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
> index 9f628a8e5cfbe..34bff7e9f3a33 100644
> --- a/drivers/iio/temperature/hid-sensor-temperature.c
> +++ b/drivers/iio/temperature/hid-sensor-temperature.c
> @@ -244,7 +244,7 @@ static int hid_temperature_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto error_remove_trigger;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> +	ret = iio_device_register(indio_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver use a
single global static struct hid_sensor_hub_callbacks temperature_callbacks
which is modified during probe()?

If multiple temperature sensors are probed, the pdev pointer within this
global struct is overwritten by the most recently probed device:

drivers/iio/temperature/hid-sensor-temperature.c:hid_temperature_probe() {
        ...
        temperature_callbacks.pdev = pdev;
        ret = sensor_hub_register_callback(hsdev,
                                        HID_USAGE_SENSOR_TEMPERATURE,
                                        &temperature_callbacks);
        ...
}

This causes events from all sensors to be routed to the last probed sensor's
IIO device, leading to data corruption.

Furthermore, if a sensor is removed, the remaining sensors will dereference
a dangling pdev pointer when handling events, causing a use-after-free.

>  	if (ret)
>  		goto error_remove_callback;
>  

[ ... ]

> @@ -264,6 +264,7 @@ static void hid_temperature_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct temperature_state *temp_st = iio_priv(indio_dev);
>  
> +	iio_device_unregister(indio_dev);

[Severity: High]
Does explicitly calling iio_device_unregister() before stopping the
hardware data source cause a hard lockup?

The modified teardown order unregisters the IIO device before the sensor
hub callback is removed. This function deletes the buffer from the IIO core
buffer list.

Concurrently, incoming hardware interrupts can invoke
temperature_proc_event() which locklessly iterates over the same buffer
list using list_for_each_entry().

If list_del_init() executes concurrently during the iteration, the lockless
traversal can enter an infinite loop. Because the event handler executes
with IRQs disabled, this could cause a hard lockup.

>  	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE);

[Severity: High]
This is a pre-existing issue, but does sensor_hub_remove_callback() lack
synchronization with active event processing?

When sensor_hub_remove_callback() unlinks the callback from
dyn_callback_list, it returns immediately without waiting for concurrent
executions of sensor_hub_raw_event() to finish.

Since the raw event handler drops dyn_callback_lock before executing the
callbacks, remove() can complete and trigger devres cleanup while the raw
event handler is still actively dereferencing the IIO device on another
CPU, potentially leading to a use-after-free.

>  	hid_sensor_remove_trigger(indio_dev, &temp_st->common_attributes);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-5-june-hid-iio-race-fixes-v1-0-27a848c5758f@gmail.com?part=5