Re: [PATCH v2] HID: sensor-hub: Fail unfinished multi-value reads on removal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing spinlock synchronization when aborting synchronous reads on timeout or signal allows a concurrent HID report to overwrite a popped stack frame or leak heap memory.
--

commit d72580296b294439ca880b1a4de796414e3195a6
Author: Yibo Tan <lhfff@tju.edu.cn>

    HID: sensor-hub: Fail unfinished multi-value reads on removal
    
    sensor_hub_remove() completes pending reads after stopping the HID device,
    but does not record why they completed.  A successful completion wait
    therefore returns zero even if no complete input report was received.

Link: https://github.com/kimaiden1984-boop/linux-kernel-poc-collections/tree/main/cases/hid-sensor-quaternion-root-b-kaslr [1]

> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 6470a290ebfc5..687f8defa2f0d 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -334,6 +334,8 @@ int sensor_hub_input_attr_read_values(struct hid_sensor_hub_device *hsdev,
>  			ret = -ETIMEDOUT;
>  		else if (cycles < 0)
>  			ret = cycles;
> +		else if (hsdev->pending.raw_size < 0)
> +			ret = hsdev->pending.raw_size;
>  
>  		hsdev->pending.status = false;

[Severity: Critical]
This is a pre-existing issue, but missing spinlock synchronization when aborting
synchronous reads on timeout or signal allows a concurrent HID report to
overwrite a popped stack frame.

Can a local unprivileged user trigger a stack buffer overflow here by reading
a multi-value IIO attribute, sleeping in
wait_for_completion_interruptible_timeout(), and then sending a signal to
their own process?

If the wait is aborted by a signal, this code locklessly clears
hsdev->pending.status.

If a hardware HID report interrupt arrives concurrently, it acquires
pdata->lock in sensor_hub_raw_event():

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
    ...
    spin_lock_irqsave(&pdata->lock, flags);
    ...
    if (hsdev->pending.status && ...) {
        if (hsdev->pending.max_raw_size) {
            ...
            memcpy(hsdev->pending.raw_data + hsdev->pending.index,
                   ptr, copy_size);

Could the interrupt observe status as true and execute memcpy() copying
attacker-influenced HID data into hsdev->pending.raw_data, which now points to
a freed stack frame since sensor_hub_input_attr_read_values() already returned?

This same pattern also appears in sensor_hub_input_attr_get_raw_value():

drivers/hid/hid-sensor-hub.c:sensor_hub_input_attr_get_raw_value() {
    ...
    if (flag == SENSOR_HUB_SYNC) {
        wait_for_completion_interruptible_timeout(&hsdev->pending.ready, HZ*5);
        ...
        kfree(hsdev->pending.raw_data);
        hsdev->pending.status = false;
    }

If an interrupt arrives immediately after the wait is aborted and executes:

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
    ...
    } else {
        hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
        ...
    }

Does this memory leak, since the kmemdup() allocates a buffer that the waiting
thread has already abandoned?

>  	}
>  	mutex_unlock(hsdev->mutex_ptr);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912050257.837340-1-lhfff@tju.edu.cn?part=1