Re: [PATCH v2 1/8] iio: orientation: hid-sensor-rotation: 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()` creates a TOCTOU race condition that can lead to IIO buffer list corruption and infinite loops.

Pre-existing issues:
- [High] Concurrent execution of sensor hub callbacks can cause use-after-free on probe failure or device removal due to lack of synchronization.
- [High] Malicious or malformed HID devices can trigger an out-of-bounds read of kernel memory due to a lack of bounds checking on the raw data buffer.
--

commit 3524bf526c21d398ff01cdcd77d3cff858988d72
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: orientation: hid-sensor-rotation: Avoid race between callback setup and device exposure

This patch reorders the probe and remove paths in the hid-sensor-rotation
driver to ensure sensor hub callbacks are registered before the IIO device
is exposed, and removed after the device is unregistered. This aims to
prevent userspace from accessing the device before callbacks are ready.

> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
> --- a/drivers/iio/orientation/hid-sensor-rotation.c
> +++ b/drivers/iio/orientation/hid-sensor-rotation.c

[ ... ]

> @@ -345,13 +339,19 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
>  	rot_state->callbacks.send_event = dev_rot_proc_event;
>  	rot_state->callbacks.capture_sample = dev_rot_capture_sample;

[Severity: High]
This isn't a bug introduced by this patch, but does dev_rot_capture_sample()
lack bounds checking on the raw data buffer?

If a maliciously crafted HID descriptor specifies a small size (e.g., 4 bytes)
for the quaternion field, the HID core will allocate a buffer matching that
small size.

In dev_rot_capture_sample(), if the size doesn't match the expected 16 bytes,
it falls back to a blind copy:

        } else {
                memcpy(&rot_state->scan.sampled_vals, raw_data,
                       sizeof(rot_state->scan.sampled_vals));
        }

Since sizeof(rot_state->scan.sampled_vals) is 16 bytes, this reads past the
end of a smaller raw_data buffer and copies adjacent kernel heap memory into
the IIO buffer. Userspace can then read this IIO buffer, resulting in an
information leak.

>  	rot_state->callbacks.pdev = pdev;
>  	ret = sensor_hub_register_callback(hsdev, hsdev->usage,
>  					&rot_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_trigger;
> -	}
>  
>  	return 0;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: High]
This is a pre-existing issue, but does removing the callback here leave a
window for a use-after-free?

sensor_hub_raw_event() looks up a callback and executes it under pdata->lock.
However, sensor_hub_remove_callback() removes the callback from
pdata->dyn_callback_list without waiting for any currently executing
callbacks in sensor_hub_raw_event() to complete.

If a callback is executing concurrently on another CPU when probe fails or
the device is removed, the driver will exit and devres will free the
indio_dev and rot_state memory structures. The still-executing callback
will then continue reading and writing to the freed rot_state fields.

>  error_remove_trigger:
>  	hid_sensor_remove_trigger(indio_dev, &rot_state->common_attributes);
>  	return ret;
> @@ -364,8 +364,8 @@ static void hid_dev_rot_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct dev_rot_state *rot_state = iio_priv(indio_dev);
>  
> -	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: High]
Does reordering iio_device_unregister() before sensor_hub_remove_callback()
create a race condition that can lead to an infinite loop?

iio_device_unregister() disables all buffers, which calls list_del_init() on
the buffer list under the IIO core info_exist_lock. If the sensor hub
callback has not been removed yet, a hardware interrupt can still trigger
the callback, which calls dev_rot_proc_event() and subsequently
iio_push_to_buffers().
        
iio_push_to_buffers() iterates over the buffer list locklessly using
list_for_each_entry(). If list_del_init() executes concurrently with this
lockless iteration, the 'next' pointer of the removed list element points
to itself. This causes the lockless iterator to enter an infinite loop and
hang the kernel.

Should the data producer (the callback) be stopped before destroying the
data consumer (unregistering the IIO device)?

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

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