Re: [PATCH v2 8/8] iio: accel: hid-sensor-accel-3d: Avoid race between callback setup and device exposure

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

Pre-existing issues:
- [Critical] Pre-existing Use-After-Free during driver removal or probe failure due to lack of callback synchronization.
- [High] Pre-existing race condition between data push and buffer disable leading to infinite loop.
- [High] Missing length validation on `raw_data` in `accel_3d_capture_sample` leads to out-of-bounds reads and potential Information Leak.
--

commit d6d7cec4acad5d2e20a4e415413ae49c117fe317
Author: Sanjay Chitroda [off-list ref]

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

This patch reorders the probe and remove paths in the hid-sensor-accel-3d
driver to ensure callbacks are registered before device exposure and removed
after the device is unregistered. This aims to prevent race conditions where
userspace might enable buffers before callbacks are fully set up.

[Severity: High]
This isn't a bug introduced by this patch, but does this callback setup leave
us vulnerable to an out-of-bounds read?

Looking at accel_3d_capture_sample(), it casts the incoming raw_data pointer
without checking if raw_len is sufficiently large:

accel_3d_capture_sample()
    accel_state->scan.accel_val[CHANNEL_SCAN_INDEX_X + offset] =
        *(u32 *)raw_data;

Since raw_len is derived directly from the HID report descriptor provided by
the device, could a malicious device declare an artificially small field
(e.g., 1 byte) causing us to read adjacent kernel slab memory when
dereferencing as a u32 or int64_t?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could this unregister sequence
trigger an infinite loop during a concurrent data push?

If accel_3d_proc_event() is executing asynchronously in the HID event
context and observes data_ready > 0, it prepares to call
iio_push_to_buffers().

If preempted, another thread could execute this remove path:

hid_accel_3d_remove()
  -> iio_device_unregister()
    -> iio_disable_all_buffers()
      -> iio_buffer_deactivate_all()
        -> iio_buffer_deactivate()

iio_buffer_deactivate() uses list_del_init(&buffer->buffer_list), which makes
the list node self-referential. When iio_push_to_buffers() resumes its
lockless list_for_each_entry() traversal, will it infinite loop on this node?

+	sensor_hub_remove_callback(hsdev, hsdev->usage);

[Severity: Critical]
This is a pre-existing issue, but does this callback removal leave a window
for a use-after-free?

sensor_hub_remove_callback() removes the callback from the list under a
spinlock but does not appear to wait for concurrently executing
sensor_hub_raw_event() handlers to complete.

Once hid_accel_3d_remove() returns, the devres framework will immediately
free indio_dev and accel_state. If a concurrent sensor_hub_raw_event() already
fetched the callback pointer:

sensor_hub_raw_event()
    if (callback->capture_sample) {
        if (report->field[i]->logical)
            callback->capture_sample(hsdev, ...);

Could it dereference the freed accel_state to execute capture_sample and
send_event, resulting in a use-after-free?

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

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