Re: [PATCH 6/9] iio: light: hid-sensor-prox: 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:
- [Critical] Reordering `iio_device_unregister()` before `sensor_hub_remove_callback()` introduces a critical data race that can lead to an infinite loop, list corruption, or Use-After-Free during device teardown.

Pre-existing issues:
- [High] Lack of synchronization in `sensor_hub_remove_callback()` fails to flush in-flight events, causing a Use-After-Free of devres-managed memory during probe failure.
- [High] Incorrect parsing of little-endian HID report data using host-endian pointer casts.
--

commit 60bbe69d1f4ccbb5130ebd8324841d9eca79e70a
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: light: hid-sensor-prox: 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, which can
lead to NULL pointer dereferences or use-after-free.

Fix this by correct ordering of callback registration and
IIO device registration in probe and remove paths.

> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index edc9274a2c076..3df7f28d3b445 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c

[ ... ]

> @@ -325,13 +319,19 @@ static int hid_prox_probe(struct platform_device *pdev)
>  					   &prox_state->callbacks);
>  	if (ret < 0) {
>  		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]
This is a pre-existing issue, but this patch newly exposes it in the probe
error path by moving callback registration before iio_device_register().

When hid_prox_probe() fails here and completes, devres automatically frees the
indio_dev and prox_state structures.

sensor_hub_remove_callback() acquires pdata->dyn_callback_lock to remove the
callback from the list, but it does not wait for currently executing callbacks
in sensor_hub_raw_event() (which run under pdata->lock) to complete.

Could a still-executing event handler on another CPU dereference the freed
memory, leading to a Use-After-Free?

>  
>  	return ret;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  error_remove_trigger:

[ ... ]

> @@ -344,8 +344,8 @@ static void hid_prox_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct prox_state *prox_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: Critical]
Does reordering the teardown sequence so that sensor_hub_remove_callback()
runs after iio_device_unregister() leave the sensor hub callback fully
active while the IIO device is being dismantled?

iio_device_unregister() calls iio_disable_all_buffers(), which iterates over
the buffer_list using list_del_init() and frees internal state. Concurrently,
incoming hardware events trigger prox_proc_event(). While
iio_disable_all_buffers() eventually decrements data_ready to 0,
prox_proc_event() checks data_ready without any locking.

If prox_proc_event() observes data_ready > 0 and calls iio_push_to_buffers()
concurrently with buffer teardown, will it perform a lockless list traversal
over the actively modifying buffer_list?

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

Could this concurrent modification cause the iterator's next pointer to become
self-referential, resulting in an infinite loop, or access freed memory
causing a Use-After-Free?

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

[Severity: High]
This isn't a bug introduced by this patch, but looking at how raw data
is handled in prox_capture_sample() from drivers/iio/light/hid-sensor-prox.c:

	switch (raw_len) {
	case 1:
		prox_state->human_presence[chan] = *(u8 *)raw_data * multiplier;
		return 0;
	case 2:
		prox_state->human_presence[chan] = *(u16 *)raw_data * multiplier;
		return 0;
	case 4:
		prox_state->human_presence[chan] = *(u32 *)raw_data * multiplier;
		return 0;
	}

The raw data received from the HID sensor hub is in little-endian format as
per the HID specification.

When raw_data is directly cast to u16 * or u32 * and dereferenced on a Big
Endian architecture, will this host-endian read result in incorrectly swapped
bytes, producing corrupted sensor readings?

Furthermore, since raw_data points directly into the unaligned HID report byte
array, can this cast trigger an unaligned memory access exception (kernel oops)
on strict architectures? Should this use get_unaligned_le16() or
get_unaligned_le32() instead to correctly parse the little-endian data stream?

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