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

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 [off-list ref]

    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/ (local)

[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