Re: [PATCH v2 7/8] iio: magnetometer: hid-sensor-magn-3d: 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] Reversing teardown order introduces severe race condition leading to list corruption and use-after-free.
- [High] Reordering probe sequence introduces a Use-After-Free (UAF) vulnerability on the error path.

Pre-existing issues:
- [High] Direct unaligned and out-of-bounds dereference of `raw_data` buffer.
--

commit 88cf07ac8710a7390dc946776d5f89db30869a24
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>

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

The driver currently exposes the IIO device to userspace before
completing sensor hub callback registration, and similarly removes
callbacks while the device can still be accessed during teardown.

> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
[ ... ]
> @@ -526,13 +520,19 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
>  					&magn_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;
>  	}
>  
>  	return ret;
>  
> -error_iio_unreg:
> -	iio_device_unregister(indio_dev);
> +error_remove_callback:
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_COMPASS_3D);

[Severity: High]
If iio_device_register() fails, does this error path allow a use-after-free
if a concurrent HID event is processing?

sensor_hub_remove_callback() removes the callback from the list lock, but it
does not synchronize with currently executing events. If an event is executing
inside sensor_hub_raw_event() calling magn_3d_capture_sample(), and we return
an error here, devres will immediately free the indio_dev and magn_state memory.

Could the still-executing callback dereference freed memory?

>  error_remove_trigger:
>  	hid_sensor_remove_trigger(indio_dev, &magn_state->magn_flux_attributes);
>  	return ret;
> @@ -545,8 +545,8 @@ static void hid_magn_3d_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct magn_3d_state *magn_state = iio_priv(indio_dev);
>  
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_COMPASS_3D);
>  	iio_device_unregister(indio_dev);
> +	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_COMPASS_3D);

[Severity: High]
Does changing this teardown order introduce list corruption?

iio_device_unregister() disables IIO buffers, which iterates and modifies
the IIO device's buffer_list using list_del_init(). Because the callback
is still active here, a concurrent hardware event can invoke
magn_3d_proc_event() which calls iio_push_to_buffers_with_timestamp().

iio_push_to_buffers_with_timestamp() iterates the exact same buffer_list
locklessly. Can iterating this linked list concurrently with node deletion
cause list corruption and an eventual use-after-free?

>  	hid_sensor_remove_trigger(indio_dev, &magn_state->magn_flux_attributes);
>  }

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential alignment
and out-of-bounds problem in magn_3d_capture_sample():

magn_3d_capture_sample() {
    ...
    magn_state->timestamp =
        hid_sensor_convert_timestamp(&magn_state->magn_flux_attributes,
                                     *(s64 *)raw_data);
    ...
}

Does casting the raw_data buffer directly to an s64 pointer guarantee
alignment and bounds safety?

Since raw_data points directly into the parsed HID report, it might not be
8-byte aligned, which could cause an alignment fault on architectures that
do not support hardware unaligned access.

Additionally, if the device hardware reports a smaller data length (such as a
32-bit timestamp), dereferencing it as an s64 will read past the valid
boundaries of the HID report payload.

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