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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unsynchronized cancellation of pending requests leads to stack corruption or memory leaks when wait_for_completion_interruptible_timeout() exits early.
- [Critical] Missing validation of `field->maxusage` in `sensor_hub_raw_event()` allows an Out-Of-Bounds read on maliciously crafted HID descriptors.
- [Critical] Unconditional 16-byte read from HID report payload regardless of field size causes an Out-Of-Bounds read in dev_rot_capture_sample().
- [High] Overwriting hsdev->pending.raw_data without freeing previous allocations causes a memory leak when a report contains duplicate usages.
--

commit df43a8787a3e2f8bf5173b9ad940b826c008809a
Author: Yibo Tan <lhfff@tju.edu.cn>
Subject: 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

> 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 can clearing hsdev->pending.status locklessly
here and in sensor_hub_input_attr_get_raw_value() race with hardware event
processing?

If wait_for_completion_interruptible_timeout() returns early (due to a signal
or timeout), the waiter thread cleans up locklessly and returns, popping the
stack frame if the buffer was stack-allocated.

Concurrently, an incoming HID report could trigger sensor_hub_raw_event() which
reads status locklessly or races before it is cleared. The interrupt handler
then writes to the now-popped stack frame using memcpy(), causing stack
corruption, or allocates new memory using kmemdup() that will never be freed
after the waiter thread calls kfree().

[ ... ]

Also looking at sensor_hub_raw_event() in drivers/hid/hid-sensor-hub.c:

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,

[Severity: Critical]
This is a pre-existing issue, but is it safe to dereference usage[0] here
without verifying that the array contains any elements (i.e. maxusage > 0)?

If a maliciously crafted HID device descriptor specifies an input field with 0
usages, the HID core allocates exactly sizeof(struct hid_field) without any
trailing usages array, but field->usage is set to point directly after the
struct. Accessing report->field[i]->usage->collection_index (which is
usage[0].collection_index) would read out of bounds of the allocated slab
object.


				(report->field[i]->report_size *
					report->field[i]->report_count)/8);
		...
		if (hsdev->pending.status && ...) {
			...
			} else {
				hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);

[Severity: High]
This is a pre-existing issue, but could overwriting hsdev->pending.raw_data
without freeing previous allocations cause a memory leak if a report contains
duplicate usages?

If a single report is received containing multiple fields sharing the same usage
ID, sensor_hub_raw_event() iterates over all of them. For each match, if
max_raw_size is 0, it executes kmemdup() and assigns the pointer to
hsdev->pending.raw_data without calling kfree() on the previous pointer.

Since the entire loop executes synchronously under a spinlock, the waiter thread
cannot free the intermediate allocations, leading to permanent leakage of all
but the last allocation.


				if (hsdev->pending.raw_data)
					hsdev->pending.raw_size = sz;
				else
					hsdev->pending.raw_size = 0;
				complete(&hsdev->pending.ready);
			}
		}
	}
}

Also looking at dev_rot_capture_sample() in drivers/iio/orientation/hid-sensor-rotation.c:

drivers/iio/orientation/hid-sensor-rotation.c:dev_rot_capture_sample() {
	...
	if (usage_id == HID_USAGE_SENSOR_ORIENT_QUATERNION) {
		if (raw_len / 4 == sizeof(s16)) {
			...
		} else {
			memcpy(&rot_state->scan.sampled_vals, raw_data,
			       sizeof(rot_state->scan.sampled_vals));

[Severity: Critical]
This is a pre-existing issue, but are we performing an unconditional 16-byte
read here without validating that the incoming raw_data buffer is at least 16
bytes?

If a maliciously crafted HID device descriptor specifies a quaternion field with
a small size (e.g. 4 bytes total), the HID core allocates a small report buffer.
When a report is received, sensor_hub_raw_event() passes this small buffer to
the capture_sample callback, passing raw_len = 4.

Because raw_len / 4 != sizeof(s16) (1 != 2), the callback takes the else branch
and calls memcpy(&rot_state->scan.sampled_vals, raw_data, 16), reading 12 bytes
past the end of the small HID report buffer. This can leak kernel heap contents
to the userspace IIO buffer.


		}
	...
}

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