Re: [PATCH v2 2/8] iio: orientation: hid-sensor-incl-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:
- [High] Reordering `iio_device_unregister()` before `sensor_hub_remove_callback()` introduces a race condition leading to list corruption and use-after-free.

Pre-existing issues:
- [High] `sensor_hub_remove_callback()` does not wait for concurrently executing callbacks to finish, leading to Use-After-Free.
- [High] Out-Of-Bounds read in `incl_3d_capture_sample` due to lack of `raw_len` bounds checking.
--

commit 4947d2a547210c33e3a2154a81959edb205f29de
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

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

This commit reorders the probe and remove paths for the incl-3d sensor driver.
It aims to ensure callbacks are registered before device registration and
removed after device unregistration, closing a timing window where userspace
might enable the buffer before callbacks are available.

> diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c b/drivers/iio/orientation/hid-sensor-incl-3d.c
> index c7fbff498be7a..5696e4ef36334 100644
> --- 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]
This is a pre-existing issue, but does sensor_hub_remove_callback() wait for
currently executing callbacks to finish?

When sensor_hub_remove_callback() is called here or during device removal, it
removes the callback from the internal list under a spinlock but doesn't
appear to synchronize with event handlers executing on another CPU.

Since the devres subsystem subsequently frees the memory containing incl_state,
could a concurrently executing callback dereference the freed memory, leading
to a use-after-free?

>  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]
Does this reordering introduce a race condition leading to list corruption
and a use-after-free?

If iio_device_unregister() executes while the HID sensor callback is still
actively registered, iio_disable_all_buffers() will locklessly clear the
buffer_list using list_del_init().

Because the hardware is not synchronously powered down, incoming events can
continue to trigger incl_3d_proc_event(), which calls
iio_push_to_buffers_with_timestamp().

Since iio_push_to_buffers_with_timestamp() uses a lockless
list_for_each_entry loop to traverse the same buffer_list:

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

won't modifying this list concurrently cause list corruption or a panic?

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

[Severity: High]
This is a pre-existing issue, but does incl_3d_capture_sample() check the
raw_len bounds before dereferencing raw_data?

drivers/iio/orientation/hid-sensor-incl-3d.c:incl_3d_capture_sample() {
    ...
    incl_state->scan.incl_val[CHANNEL_SCAN_INDEX_X] = *(u32 *)raw_data;
    ...
}

If a malicious USB HID device or a userspace program using uhid advertises a
custom HID descriptor with a field size of 1 byte, could dereferencing
raw_data as a 32-bit or 64-bit integer read past the end of the field and
leak kernel heap memory into the IIO buffer?

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