Re: [PATCH v3] 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:
- [High] Lockless update of hsdev->pending.status upon timeout leads to stack Use-After-Free, memory leaks, and memory corruption.
--

commit a0af07f0a44e5b5c9aefab2afc1fc904fb7bc800
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.
    Multi-value IIO callers then format their untouched automatic buffer as a
    successful result.

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..a9bd72218c070 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.shutdown)
> +			ret = -ENODEV;
>  
>  		hsdev->pending.status = false;

[Severity: High]
This is a pre-existing issue, but does clearing hsdev->pending.status here
without acquiring data->lock create a race condition with the interrupt
handler?

If a read request times out, this function locklessly assigns status to false
and returns, freeing the caller's stack frame. 

Can the interrupt handler in sensor_hub_raw_event() then concurrently read
status as true and call memcpy() into the now-popped stack frame?

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
	...
	memcpy(hsdev->pending.raw_data + hsdev->pending.index,
	       ptr, copy_size);
	...
}

Would a similar race occur in sensor_hub_input_attr_get_raw_value(), where
the interrupt handler might allocate memory via kmemdup() after the waiter
has already exited, permanently leaking the memory?

>  	}
> @@ -805,8 +807,10 @@ static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
>  {
>  	struct hid_sensor_hub_device *hsdev = dev->platform_data;
>  
> -	if (hsdev->pending.status)
> +	if (hsdev->pending.status) {
> +		hsdev->pending.shutdown = true;
>  		complete(&hsdev->pending.ready);
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This is also a pre-existing issue, but can this read a stale status due to
the lockless update on the timeout path?

If a timeout just occurred and the previous reader locklessly set status to
false, could sensor_hub_finalize_pending_fn() still observe status as true
during device teardown?

If a new reader then concurrently starts and calls memset() and
init_completion(), could the complete() call here corrupt the completion's
internal spinlock?

[ ... ]

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