Re: [PATCH 4/9] iio: pressure: hid-sensor-press: Fix race between callback registration and device exposure

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Reordering teardown introduces list corruption and UAF via concurrent buffer access.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the lack of synchronization in sensor hub callback removal leads to Use-After-Free, and this patch exposes the race on the probe error path.
--

commit 82bc619c3ad12fb028f336e0c55f37d3a740bf6e
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: pressure: hid-sensor-press: Fix race between callback registration and device exposure

The driver registers the IIO device before completing sensor hub
callback registration and unregisters callbacks while the IIO device
is still exposed during teardown. This patch reorders callback
registration and IIO device registration in probe and remove paths.

> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
> index a039b99d9851d..09795473948a9 100644
> --- a/drivers/iio/pressure/hid-sensor-press.c
> +++ b/drivers/iio/pressure/hid-sensor-press.c
[ ... ]
> @@ -304,13 +298,19 @@ static int hid_press_probe(struct platform_device *pdev)
>  					&press_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_PRESSURE);

[Severity: High]
This is a pre-existing issue, but the lack of synchronization in sensor
hub callback removal might lead to a use-after-free, and this patch
exposes it on the probe error path.

When sensor_hub_remove_callback() deletes the list entry:

drivers/hid/hid-sensor-hub.c:sensor_hub_remove_callback() {
    list_del(&callback->list);
    kfree(callback);
    ...
}

it doesn't appear to wait for active readers to finish processing. If
iio_device_register() fails, the driver calls sensor_hub_remove_callback()
and returns, allowing devres to free the press_state memory. 

Could a suspended or delayed sensor_hub_raw_event() thread resume and
dereference the freed memory?

>  error_remove_trigger:
>  	hid_sensor_remove_trigger(indio_dev, &press_state->common_attributes);
>  	return ret;
> @@ -323,8 +323,8 @@ static void hid_press_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct press_state *press_state = iio_priv(indio_dev);
>  
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PRESSURE);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PRESSURE);

[Severity: Critical]
Does this reversed teardown order introduce list corruption or a
use-after-free through concurrent buffer access?

By calling iio_device_unregister() before removing the sensor hub
callback, incoming HID events can still trigger press_proc_event().
This locklessly pushes to buffers:

drivers/iio/pressure/hid-sensor-press.c:press_proc_event()
  iio_push_to_buffers_with_ts()
    iio_push_to_buffers()
      list_for_each_entry(buf, &iio_dev_opaque->buffer_list, buffer_list)

While this runs, iio_device_unregister() actively removes buffers
from the list:

iio_device_unregister()
  iio_disable_all_buffers()
    list_del_init()

Could iterating over the buffer list while it is being modified by
list_del_init() cause the lockless reader to follow the self-referential
next pointer, leading to an infinite loop or use-after-free?

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