Re: [PATCH 2/9] iio: orientation: hid-sensor-incl-3d: Fix race between callback registration and device exposure

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a Use-After-Free (UAF) vulnerability in the probe error path by registering the sensor hub callback before completing IIO device registration.
- [High] The patch inverts the teardown order in the remove path, leaving the HID callback active during IIO buffer teardown, which leads to lockless list corruption or Use-After-Free.

Pre-existing issues:
- [High] The driver suffers from a pre-existing Use-After-Free vulnerability during device removal because `sensor_hub_remove_callback()` does not synchronize with currently executing HID callbacks.
--

commit 7127797d9af9cf3f6a23d56c71b46a308c88b097
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: orientation: hid-sensor-incl-3d: Fix race between callback registration and device exposure

This commit modifies the probe and remove paths in the HID 3D inclinometer
driver to adjust the registration order between the IIO device and the sensor
hub callbacks. It attempts to fix a race condition by registering callbacks
before the IIO device in the probe function, and unregistering the IIO device
before removing callbacks in the remove function.

> diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c b/drivers/iio/orientation/hid-sensor-incl-3d.c
> --- a/drivers/iio/orientation/hid-sensor-incl-3d.c
> +++ b/drivers/iio/orientation/hid-sensor-incl-3d.c
[ ... ]
> @@ -370,13 +364,19 @@ static int hid_incl_3d_probe(struct platform_device *pdev)
>  					&incl_state->callbacks);
>  	if (ret) {
>  		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 0;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_INCLINOMETER_3D);

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

By registering the callback before IIO device registration completes, the HID
event callback is added to the active list and can dispatch events
asynchronously immediately.

If iio_device_register() fails, the error path calls
sensor_hub_remove_callback(). However, this function only deletes the
callback from a lock-protected list and does not synchronize with
concurrently executing callbacks:

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
    ...
    if (callback && collection && callback->send_event)
        callback->send_event(hsdev, collection->usage, callback->pdev);
    ...
}

When the probe function returns, the devres subsystem will free the indio_dev
memory which contains the callbacks structure.

If a concurrent HID event was dispatched just before the callback was
removed, could it locklessly dereference the freed callback->send_event
function pointer?

>  error_remove_trigger:
>  	hid_sensor_remove_trigger(indio_dev, &incl_state->common_attributes);
>  	return ret;
[ ... ]
> @@ -389,8 +389,8 @@ static void hid_incl_3d_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct incl_3d_state *incl_state = iio_priv(indio_dev);
>  
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_INCLINOMETER_3D);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_INCLINOMETER_3D);

[Severity: High]
Might this inverted teardown order cause list corruption or a use-after-free?

By leaving the HID callback active during IIO device unregistration, the
hardware event source remains active while dismantling structures.

When iio_device_unregister() disables buffers, it removes them from
indio_dev->buffer_list using list_del_init() without holding locks that
protect the push path.

If other sensors keep the shared HID hub active, events for this sensor can
arrive concurrently. The event handler calls iio_push_to_buffers(), which
iterates locklessly:

drivers/iio/industrialio-buffer.c:iio_push_to_buffers() {
    ...
    list_for_each_entry(buf, &iio_dev_opaque->buffer_list, buffer_list) {
        ret = iio_push_to_buffer(buf, data);
    ...
}

If these race, could the concurrent modification result in list corruption or
dereferencing freed buffers?

[Severity: High]
This is a pre-existing issue, but does sensor_hub_remove_callback() properly
synchronize with executing HID callbacks before device removal completes?

Looking at the implementation, sensor_hub_remove_callback() removes the
callback from a lock-protected list and returns immediately:

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

It does not wait for any actively running callbacks in sensor_hub_raw_event()
to finish. Once hid_incl_3d_remove() completes, devres frees the indio_dev
memory containing the incl_state and its callbacks.

If the HID event thread is still executing the callback locklessly, might it
dereference a freed function pointer and access freed memory?

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

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