Re: [PATCH 1/9] iio: orientation: hid-sensor-rotation: Fix race between callback registration and device exposure

[sashiko-bot@kernel.org]

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

New issues:
- [High] Newly introduced Use-After-Free in the probe error path caused by registering the callback before IIO device registration.
- [High] Newly introduced data race leading to list corruption in the remove path by unregistering the IIO device while the sensor hub callback is still active.

Pre-existing issues:
- [High] Pre-existing Use-After-Free due to a lack of synchronization in sensor hub callback removal.
- [High] Pre-existing Out-of-Bounds Read in dev_rot_capture_sample when reading malicious HID reports.
--

commit 53e3688342562957aa2297744b7cc0b03bf921af
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>
Subject: iio: orientation: hid-sensor-rotation: 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 creates race windows in both probe and remove paths.
This patch attempts to fix this by correct ordering of callback registration.

Fixes: fc18dddc0625 ("iio: hid-sensors: Added device rotation support")

> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
> index 4a11e45550991..fd9beb93cefb1 100644
> --- a/drivers/iio/orientation/hid-sensor-rotation.c
> +++ b/drivers/iio/orientation/hid-sensor-rotation.c
> @@ -332,12 +332,6 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "device register failed\n");
> -		goto error_remove_trigger;
> -	}
> -
>  	rot_state->callbacks.send_event = dev_rot_proc_event;
>  	rot_state->callbacks.capture_sample = dev_rot_capture_sample;

[Severity: High]
This is a pre-existing issue, but can dev_rot_capture_sample() read
out-of-bounds when handling malicious HID reports?

If raw_len does not correspond to an array of four 16-bit integers, the
function unconditionally copies 16 bytes:

drivers/iio/orientation/hid-sensor-rotation.c:dev_rot_capture_sample() {
    ...
    } else {
        memcpy(&rot_state->scan.sampled_vals, raw_data,
               sizeof(rot_state->scan.sampled_vals));
    }
    ...
}

If a malicious USB device provides a report descriptor with a field size
smaller than 16 bytes, will this memcpy() read out-of-bounds without verifying
that raw_len is at least 16?

>  	rot_state->callbacks.pdev = pdev;
> @@ -345,13 +339,19 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
>  					&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_callback;
>  	}

[Severity: High]
Is there a use-after-free introduced in this new error path?

By registering the callback before the IIO device, if iio_device_register()
fails, the error path removes the callback and returns, triggering devres
cleanup.

Because sensor_hub_remove_callback() lacks execution synchronization, an event
dispatched immediately after callback registration can run concurrently. When
hid_dev_rot_probe() returns, the rot_state memory is freed while the event
handler is still executing.

[ ... ]

> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but does hid_dev_rot_remove()
complete teardown without synchronization, allowing devres to free memory
while callbacks are still executing?

When sensor_hub_remove_callback() removes the callback entry from the list:

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

It does not wait for any currently executing callback, such as
dev_rot_proc_event(), to finish. When hid_dev_rot_remove() returns, devres
automatically frees indio_dev and rot_state.

If dev_rot_proc_event() is running concurrently on another CPU:

drivers/iio/orientation/hid-sensor-rotation.c:dev_rot_proc_event() {
    ...
    struct iio_dev *indio_dev = platform_get_drvdata(priv);
    struct dev_rot_state *rot_state = iio_priv(indio_dev);
    ...
}

Could it dereference the freed indio_dev pointer?

>  
> -	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: High]
Will moving sensor_hub_remove_callback() after iio_device_unregister()
dismantle the buffer list concurrently with active events?

Leaving the callback active during teardown means an incoming sensor event can
trigger dev_rot_proc_event(), which calls iio_push_to_buffers() while
iio_device_unregister() is disabling buffers.

If iio_push_to_buffers() locklessly iterates over the buffer list:

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

And iio_device_unregister() concurrently uses list_del_init() to remove
buffers from the list, might this cause list corruption?

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

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