Re: [PATCH 7/9] iio: light: hid-sensor-als: 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] Reversing the teardown order in `hid_als_remove()` leaves the data producer active during IIO device unregistration, leading to buffer list corruption and a kernel panic.
- [High] Registering the HID sensor hub callback before `iio_device_register()` introduces a Use-After-Free vulnerability on the probe error path.

Pre-existing issues:
- [High] `sensor_hub_remove_callback()` does not wait for executing callbacks to finish, causing a Use-After-Free when the driver unbinds.
--

commit 7a8f6d5a76d506427e21235e877b3b2cd1616826
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

iio: light: hid-sensor-als: Fix race between callback registration and device exposure

This commit modifies the initialization and teardown sequences in the
hid-sensor-als driver. It registers the sensor hub callback before exposing
the IIO device in the probe path, and reverses the order in the remove
path.

> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index d72e260b82668..93d6038486908 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -406,25 +406,25 @@ static int hid_als_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;
> -	}
> -
>  	als_state->callbacks.send_event = als_proc_event;
>  	als_state->callbacks.capture_sample = als_capture_sample;
>  	als_state->callbacks.pdev = pdev;
>  	ret = sensor_hub_register_callback(hsdev, hsdev->usage, &als_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]
Does registering the HID sensor hub callback before iio_device_register()
introduce a use-after-free vulnerability on the probe error path?

If iio_device_register() fails, the error path calls
sensor_hub_remove_callback() and the probe function returns, triggering
devres to free the allocated indio_dev and als_state memory.

However, sensor_hub_remove_callback() only unlinks the callback from the
list; it does not synchronize or wait for concurrently executing events.
If an event arrived from the hardware between registration and failure,
als_capture_sample() could be executing on another CPU.

That function will continue to run and write to the devres-managed memory
after it has been freed:

drivers/iio/light/hid-sensor-als.c:als_capture_sample() {
    ...
	struct iio_dev *indio_dev = platform_get_drvdata(priv);
	struct als_state *als_state = iio_priv(indio_dev);

	switch (usage_id) {
	case HID_USAGE_SENSOR_LIGHT_ILLUM:
		als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
    ...
}

>  	}
>  
>  	return ret;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  error_remove_trigger:
>  	hid_sensor_remove_trigger(indio_dev, &als_state->common_attributes);
>  	return ret;
> @@ -437,8 +437,8 @@ static void hid_als_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct als_state *als_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 reversing the teardown order here leave the data producer active
during IIO device unregistration?

iio_device_unregister() cleans up buffers by calling
iio_disable_all_buffers(), which unlinks buffers from
indio_dev->buffer_list using list_del_init():

drivers/iio/industrialio-buffer.c:iio_buffer_deactivate() {
    ...
	list_del_init(&buffer->buffer_list);
    ...
}

Because the HID event callback remains registered and active, incoming
sensor events can concurrently trigger iio_push_to_buffers_with_ts(). This
function locklessly iterates over buffer_list:

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

Concurrently traversing a linked list while it is being actively dismantled
via list_del_init() without synchronization causes the reader to follow
self-referential pointers, which can result in an infinite loop, soft
lockup, or kernel panic.

[Severity: High]
This is a pre-existing issue, but does sensor_hub_remove_callback() wait
for executing callbacks to finish?

When the driver is removed, it removes the callback using
sensor_hub_remove_callback(). This function removes the callback from the
list under dyn_callback_lock but fails to synchronize with or wait for
currently running callbacks:

drivers/hid/hid-sensor-hub.c:sensor_hub_remove_callback() {
    ...
	list_for_each_entry(callback, &pdata->dyn_callback_list, list)
		if (callback->usage_id == usage_id &&
						callback->hsdev == hsdev) {
			list_del(&callback->list);
			kfree(callback);
			break;
		}
	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
    ...
}

If an event is already in-flight, als_capture_sample() can continue
executing after hid_als_remove() completes and devres frees the indio_dev.
The concurrent execution will then dereference the freed als_state
structure, causing a use-after-free.

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

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