Re: [PATCH v1] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Concurrent execution of HID sensor hub callbacks and device unbind leads to a Use-After-Free of `indio_dev`.
- [High] The static global `temperature_callbacks` struct is mutated during probe, causing data corruption and cross-device routing bugs when multiple sensors are present.
--

commit 829c19b8a370c5d8b09345d4e07b865deb47a88f
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

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

This commit replaces devm_iio_device_register() with iio_device_register() 
to ensure deterministic cleanup. It adds an explicit iio_device_unregister() 
call in the remove path to fix potential use-after-free race conditions 
during device removal.

> 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 looking at the probe function
just prior to this registration, is there a risk of corrupting device routing
when multiple sensors are present?

The driver defines temperature_callbacks as a static global struct:

static struct hid_sensor_hub_callbacks temperature_callbacks = {
        .send_event = &temperature_proc_event,
        .capture_sample = &temperature_capture_sample,
};

During hid_temperature_probe(), the current device's pdev is directly
assigned to temperature_callbacks.pdev.

If a system has multiple HID temperature sensors, does the last probed sensor
overwrite this global pointer? It seems like when sensor_hub_raw_event()
processes hardware events, it might fetch the shared struct and pass the
single overwritten pdev to temperature_capture_sample(). Would this cause the
data from all temperature sensors to be written exclusively into the IIO state
of the last probed sensor?

[ ... ]

> @@ -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);
>  	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE);

[Severity: Critical]
This is a pre-existing issue, but I wanted to ask about the teardown order
here.

In hid_temperature_remove(), we call sensor_hub_remove_callback(), which
removes the callback from the sensor hub's list under dyn_callback_lock.
However, it doesn't appear to synchronize with or wait for concurrently
executing callback handlers like sensor_hub_raw_event() to finish before
returning.

Since indio_dev is allocated with devm_iio_device_alloc() during probe, it
will be freed by devres immediately after hid_temperature_remove() returns.

If sensor_hub_raw_event() is executing concurrently and has already retrieved
the callback pointer, could it pass the pdev to temperature_capture_sample()
and perform a use-after-free write on the freed private state?

For example:
temp_st->scan.temperature_data = *(s32 *)raw_data;

Should we be flushing or waiting for running callbacks to complete before
letting the device context be freed?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622052135.1804135-1-sanjayembedded@gmail.com?part=1