[PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[Yibo Tan]

sensor_hub_input_attr_read_values() publishes a caller-owned buffer to the
raw-event path. If its interruptible wait times out or is interrupted, it
clears pending.status without taking data->lock and returns.

sensor_hub_raw_event() may already have observed pending.status while
holding that lock. The caller can then release its buffer before raw-event
finishes copying into it.

Take data->lock when cancelling the request. The raw-event path now either
sees the request retired or finishes the copy before cancellation can
return.

On an uninstrumented PREEMPT_RT kernel, a valid 16-byte quaternion report
overwrote a live futex waiter's plist node with the report's 0x41 payload.
Two vulnerable runs produced the same general protection fault in
plist_del(), after 471 and 91 completed trials. The locking fix completed
two 10,000-trial runs without an Oops, panic, warning or payload signature.

The virtual provider setup and FIFO assignment require privilege. The IIO
read, signal handling and futex operations run as uid 65534 without
effective capabilities. No physical-device or normal-priority hit was
tested.

A source reproducer, kernel configuration, complete serial logs and the
vulnerable/fixed result table are available at:

https://github.com/kimaiden1984-boop/linux-kernel-poc-collections/tree/main/cases/hid-sensor-quaternion-root-a

Fixes: f784fcea4506 ("HID: sensor-hub: Add sensor_hub_input_attr_read_values() for multi-byte reads")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260610083849.067A11F00893@smtp.kernel.org/
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT-5
Signed-off-by: Yibo Tan <lhfff@tju.edu.cn>
---
 drivers/hid/hid-sensor-hub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 6470a290ebfc..80f18aff6f1f 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -335,7 +335,9 @@ int sensor_hub_input_attr_read_values(struct hid_sensor_hub_device *hsdev,
 		else if (cycles < 0)
 			ret = cycles;
 
+		spin_lock_irqsave(&data->lock, flags);
 		hsdev->pending.status = false;
+		spin_unlock_irqrestore(&data->lock, flags);
 	}
 	mutex_unlock(hsdev->mutex_ptr);
 
-- 
2.39.5

Re: [PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The patch fixes a missing lock race condition in `sensor_hub_input_attr_read_values` but fails to apply the same fix to `sensor_hub_input_attr_get_raw_value`, leaving an identical race condition that causes memory leaks and completion queue corruption.
- [Critical] Unvalidated access to `report->field[i]->usage` in `sensor_hub_raw_event` causes an out-of-bounds read if a field defines zero usages.
- [Critical] Missing bounds check on `ptr` against the total received `size` in `sensor_hub_raw_event` allows an out-of-bounds read of kernel memory.
--

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

    HID: sensor-hub: synchronize multi-value read cancellation
    
    sensor_hub_input_attr_read_values() publishes a caller-owned buffer to the
    raw-event path. If its interruptible wait times out or is interrupted, it
    clears pending.status without taking data->lock and returns.

Link: https://lore.kernel.org/r/20260610083849.067A11F00893@smtp.kernel.org/

> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 6470a290ebfc5..80f18aff6f1f8 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -335,7 +335,9 @@ int sensor_hub_input_attr_read_values(struct hid_sensor_hub_device *hsdev,
>  		else if (cycles < 0)
>  			ret = cycles;
>  
> +		spin_lock_irqsave(&data->lock, flags);
>  		hsdev->pending.status = false;
> +		spin_unlock_irqrestore(&data->lock, flags);
>  	}
>  	mutex_unlock(hsdev->mutex_ptr);

[Severity: Critical]
This is a pre-existing issue, but doesn't the exact same race condition
fixed here also exist in sensor_hub_input_attr_get_raw_value() in
drivers/hid/hid-sensor-hub.c?

If the wait times out or is interrupted, the state is modified and memory
freed without holding data->lock:

drivers/hid/hid-sensor-hub.c:sensor_hub_input_attr_get_raw_value() {
[ ... ]
		kfree(hsdev->pending.raw_data);
		hsdev->pending.status = false;
[ ... ]
}

Concurrently, sensor_hub_raw_event() can observe status == true while holding
the lock, and perform an allocation:

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
[ ... ]
				hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
				if (hsdev->pending.raw_data)
[ ... ]
				complete(&hsdev->pending.ready);
[ ... ]
}

Can this cause a memory leak when the dynamically allocated buffer is
stranded, and waitqueue list corruption if the waiter has already exited
and reinitialized the completion queue for a new request?

[Severity: Critical]
This is a pre-existing issue, but could sensor_hub_raw_event() in
drivers/hid/hid-sensor-hub.c unconditionally access the usage array without
validating maxusage > 0?

drivers/hid/hid-sensor-hub.c:sensor_hub_raw_event() {
[ ... ]
	for (i = 0; i < report->maxfield; ++i) {
		hid_dbg(hdev, "%d collection_index:%x hid:%x sz:%x\n",
				i, report->field[i]->usage->collection_index,
				report->field[i]->usage->hid,
[ ... ]
				report->field[i]->usage[0].collection_index,
[ ... ]
}

If a malicious USB device provides a crafted report descriptor defining a
field with zero usages, would this out-of-bounds array access to usage[0]
cause a kernel panic?

[Severity: Critical]
This is a pre-existing issue, but is it possible for sensor_hub_raw_event()
in drivers/hid/hid-sensor-hub.c to copy data from ptr without ensuring it
remains within the bounds of the total received report size?

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

If a malicious device supplies a report descriptor defining large fields but
sends a truncated input report with a small size, could this read beyond the
end of the raw_data buffer and leak adjacent kernel memory?

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

Re: [PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[Andy Shevchenko]

On Thu, Sep 10, 2026 at 07:23:38PM +0800, Yibo Tan wrote:
> sensor_hub_input_attr_read_values() publishes a caller-owned buffer to the
> raw-event path. If its interruptible wait times out or is interrupted, it
> clears pending.status without taking data->lock and returns.
> 
> sensor_hub_raw_event() may already have observed pending.status while
> holding that lock. The caller can then release its buffer before raw-event
> finishes copying into it.
> 
> Take data->lock when cancelling the request. The raw-event path now either
> sees the request retired or finishes the copy before cancellation can
> return.
> 
> On an uninstrumented PREEMPT_RT kernel, a valid 16-byte quaternion report
> overwrote a live futex waiter's plist node with the report's 0x41 payload.
> Two vulnerable runs produced the same general protection fault in
> plist_del(), after 471 and 91 completed trials. The locking fix completed
> two 10,000-trial runs without an Oops, panic, warning or payload signature.
> 
> The virtual provider setup and FIFO assignment require privilege. The IIO
> read, signal handling and futex operations run as uid 65534 without
> effective capabilities. No physical-device or normal-priority hit was
> tested.
> 
> A source reproducer, kernel configuration, complete serial logs and the
> vulnerable/fixed result table are available at:

> https://github.com/kimaiden1984-boop/linux-kernel-poc-collections/tree/main/cases/hid-sensor-quaternion-root-a

Make it a Link tag and refer in the text like [1].

Link: ...$URL... [1]

> Fixes: f784fcea4506 ("HID: sensor-hub: Add sensor_hub_input_attr_read_values() for multi-byte reads")
> Reported-by: Sashiko <sashiko-bot@kernel.org>

> Link: https://lore.kernel.org/r/20260610083849.067A11F00893@smtp.kernel.org/

What's this for? Make sure you have a reference in the text (see above the example).

> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Yibo Tan <lhfff@tju.edu.cn>
> ---

...

> +++ b/drivers/hid/hid-sensor-hub.c

> +		spin_lock_irqsave(&data->lock, flags);
>  		hsdev->pending.status = false;
> +		spin_unlock_irqrestore(&data->lock, flags);

Seems legit. Can you also amend the kernel-doc of this lock at the top of this
file? Currently it says

* @lock:		Spin lock to protect pending request structure.

I would replace the tail and make it

* @lock:		Spin lock to protect struct sensor_hub_pending request data.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[srinivas pandruvada]

On Thu, 2026-09-10 at 19:23 +0800, Yibo Tan wrote:
> sensor_hub_input_attr_read_values() publishes a caller-owned buffer
> to the
> raw-event path. If its interruptible wait times out or is
> interrupted, it
> clears pending.status without taking data->lock and returns.
> 

Hi Lixu,

Please give me quick test. Change itself looks good, not sure if we
need something more.

Thanks,
Srinivas


> sensor_hub_raw_event() may already have observed pending.status while
> holding that lock. The caller can then release its buffer before raw-
> event
> finishes copying into it.
> 
> Take data->lock when cancelling the request. The raw-event path now
> either
> sees the request retired or finishes the copy before cancellation can
> return.
> 
> On an uninstrumented PREEMPT_RT kernel, a valid 16-byte quaternion
> report
> overwrote a live futex waiter's plist node with the report's 0x41
> payload.
> Two vulnerable runs produced the same general protection fault in
> plist_del(), after 471 and 91 completed trials. The locking fix
> completed
> two 10,000-trial runs without an Oops, panic, warning or payload
> signature.
> 
> The virtual provider setup and FIFO assignment require privilege. The
> IIO
> read, signal handling and futex operations run as uid 65534 without
> effective capabilities. No physical-device or normal-priority hit was
> tested.
> 
> A source reproducer, kernel configuration, complete serial logs and
> the
> vulnerable/fixed result table are available at:
> 
> https://github.com/kimaiden1984-boop/linux-kernel-poc-collections/tree/main/cases/hid-sensor-quaternion-root-a
> 
> Fixes: f784fcea4506 ("HID: sensor-hub: Add
> sensor_hub_input_attr_read_values() for multi-byte reads")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link:
> https://lore.kernel.org/r/20260610083849.067A11F00893@smtp.kernel.org/
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Yibo Tan <lhfff@tju.edu.cn>
> ---
>  drivers/hid/hid-sensor-hub.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-
> hub.c
> index 6470a290ebfc..80f18aff6f1f 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -335,7 +335,9 @@ int sensor_hub_input_attr_read_values(struct
> hid_sensor_hub_device *hsdev,
>  		else if (cycles < 0)
>  			ret = cycles;
>  
> +		spin_lock_irqsave(&data->lock, flags);
>  		hsdev->pending.status = false;
> +		spin_unlock_irqrestore(&data->lock, flags);
>  	}
>  	mutex_unlock(hsdev->mutex_ptr);
>  

RE: [PATCH v1] HID: sensor-hub: synchronize multi-value read cancellation

[Zhang, Lixu]

>-----Original Message-----
>From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
>Sent: Friday, September 11, 2026 2:29 AM
>To: Yibo Tan <lhfff@tju.edu.cn>; Jiri Kosina <jikos@kernel.org>; Jonathan
>Cameron <jic23@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>
>Cc: Zhang, Lixu <lixu.zhang@intel.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-input@vger.kernel.org; linux-
>iio@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v1] HID: sensor-hub: synchronize multi-value read
>cancellation
>
>On Thu, 2026-09-10 at 19:23 +0800, Yibo Tan wrote:
>> sensor_hub_input_attr_read_values() publishes a caller-owned buffer to
>> the raw-event path. If its interruptible wait times out or is
>> interrupted, it clears pending.status without taking data->lock and
>> returns.
>>
>
>Hi Lixu,
>
>Please give me quick test. Change itself looks good, not sure if we need
>something more.
Hi Srinivas,

The machine is currently running other tests. Once they are done next week, I will run a quick test on this change and get back to you with feedback.

Thanks,
Lixu

>
>Thanks,
>Srinivas