Re: [PATCH v2 3/8] iio: gyro: hid-sensor-gyro-3d: Avoid race between callback setup and device exposure

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Reordering `iio_device_unregister()` to execute before `sensor_hub_remove_callback()` introduces a critical race condition leading to list corruption or an infinite loop.
- [High] The patch introduces a use-after-free vulnerability on the probe error path if `iio_device_register()` fails.

Pre-existing issues:
- [Critical] A preexisting architectural flaw in the `hid-sensor-hub` framework allows a use-after-free if a raw event is processed during driver removal.
--

commit 081315902ee73f264b09ca59da9c7688bdfeefd0
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: gyro: hid-sensor-gyro-3d: Avoid race between callback setup and device exposure

The driver currently exposes the IIO device to userspace before
completing sensor hub callback registration, and similarly removes
callbacks while the device can still be accessed during teardown.

> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> index bbca2111e79bb..c8130b488f103 100644
> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c

[ ... ]

> @@ -337,13 +331,19 @@ static int hid_gyro_3d_probe(struct platform_device *pdev)
>  					&gyro_state->callbacks);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
> -		goto error_iio_unreg;
> +		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;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_GYRO_3D);

[Severity: High]
Does this error path introduce a use-after-free vulnerability?

By moving sensor_hub_register_callback() to execute before
iio_device_register(), if device registration fails, the new error path
removes the callback and returns.

However, sensor_hub_remove_callback() does not wait for concurrently
executing callbacks to finish. If an incoming hardware event triggers
gyro_3d_proc_event() on another CPU right after registration, and then
probe returns, devres will automatically free indio_dev (which contains
gyro_state). The still-executing callback would then dereference the freed
memory.

Could this be prevented by ensuring the callback is only registered when it
is safe to process events?

[Severity: Critical]
This isn't a bug introduced by this patch, but there is a pre-existing
architectural flaw in the hid-sensor-hub framework that exacerbates this
teardown race.

Looking at sensor_hub_remove_callback(), it removes a sensor's callback from
dyn_callback_list under a lock, then immediately returns without waiting for
sensor_hub_raw_event() handlers executing on other CPUs to complete.

Because sensor_hub_raw_event() temporarily drops the list lock and invokes the
driver's function pointers based on a cached pointer, if driver removal or
probe failure completes while this is executing, devres frees the driver
memory. The framework then continues to dereference the cached callback
pointer into freed memory.

Would it be possible to add proper synchronization, such as a completion or
RCU sync, in the sensor_hub_remove_callback() framework to prevent this?

> @@ -356,8 +356,8 @@ static void hid_gyro_3d_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct gyro_3d_state *gyro_state = iio_priv(indio_dev);
>  
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_GYRO_3D);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_GYRO_3D);

[Severity: Critical]
Does reordering these functions introduce a race condition that leads to list
corruption or an infinite loop?

iio_device_unregister() disables IIO buffers and dismantles the buffer list
locklessly using list_del_init(). If we call iio_device_unregister() while
the sensor hub callback is still active, incoming hardware events can still
trigger gyro_3d_proc_event().

gyro_3d_proc_event() calls iio_push_to_buffers_with_timestamp(), which
iterates over the buffer list locklessly:

gyro_3d_proc_event()
   iio_push_to_buffers_with_timestamp()
      list_for_each_entry()

Because list_del_init() points the next pointer to itself, traversing the list
concurrently while it is being dismantled could cause the loop to never
terminate, resulting in a hard CPU hang.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-5-june-hid-iio-race-fixes-v2-0-1cfabcd1881e@gmail.com?part=3