Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Missing `report_mutex` protection in `steam_probe` causes HID report interleaving and initialization failures.
Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race in `steam_unregister` leads to resource leakage and Use-After-Free.
- [High] Use-After-Free or NULL pointer dereference in `steam_remove` due to incorrect teardown order.
- [High] Information leak of uninitialized kernel heap memory to the USB device.
--
--- Patch [5]: [PATCH v5 5/6] HID: steam: Clean up locking ---
commit 0b18780ca8de0e3b0d434638ab8287e9b2859654
Author: Vicki Pfau [off-list ref]
HID: steam: Clean up locking
This cleans up several issues with locking behavior, including RCU accesses
not being guarded behind a lock.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 7226c0681ed18..7bb892972447f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -620,7 +621,6 @@ static int steam_exchange_report_id(struct steam_device *steam, u8 *cmd, int csi
unsigned int retries = 5;
int ret;
- guard(mutex)(&steam->report_mutex);
do {
ret = steam_send_report_id(steam, cmd, csize, report_id);
if (ret < 0)
[Severity: High]
Since the guard was removed here, should callers now hold report_mutex?
This regression appears in steam_probe(), where steam_get_conn_status()
calls this without the mutex being held:
drivers/hid/hid-steam.c:steam_probe() {
...
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
...
steam->connected = false;
ret = steam_get_conn_status(steam);
...
}
Could this allow concurrent wireless events to interleave HID requests and
cause initialization failures?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1440,10 +1444,12 @@ static void steam_unregister(struct steam_device *steam)
hid_info(steam->hdev, "Steam %s '%s' disconnected",
steam->quirks & STEAM_QUIRK_DECK ? "Deck" : "Controller",
steam->serial_no);
+ mutex_lock(&steam->registration_mutex);
steam->registered = false;
steam_battery_unregister(steam);
steam_sensors_unregister(steam);
steam_input_unregister(steam);
+ mutex_unlock(&steam->registration_mutex);
cancel_work_sync(&steam->rumble_work);
cancel_delayed_work_sync(&steam->mode_switch);
cancel_delayed_work_sync(&steam->coalesce_rumble_work);
[Severity: High]
This is a pre-existing issue, but does checking the registered flag outside
this new mutex allow a time-of-check to time-of-use race?
If steam_unregister() is called concurrently from the workqueue, it might
check if (!steam->registered) and return early before the registration
finishes.
Later in steam_remove(), it completely bypasses unregistration if connected
is false, leaving registered input nodes pointing to freed memory:
drivers/hid/hid-steam.c:steam_remove() {
...
if (steam->connected)
steam_unregister(steam);
...
}
[Severity: High]
This is a pre-existing issue, but is there a use-after-free window in
steam_remove() during device teardown?
The client HID device is destroyed before the hardware is stopped:
drivers/hid/hid-steam.c:steam_remove() {
...
hid_destroy_device(steam->client_hdev);
...
hid_hw_stop(hdev);
...
}
If the hardware generates an interrupt in this window, could
steam_raw_event() attempt to access the freed client_hdev?
[Severity: High]
This is a pre-existing issue, but does steam_send_report_id() leak
uninitialized kernel heap memory to the USB device?
When allocating the report buffer, it uses hid_alloc_report_buf() which
relies on kmalloc(), not kzalloc():
drivers/hid/hid-steam.c:steam_send_report_id() {
...
buf = hid_alloc_report_buf(r, GFP_KERNEL);
if (!buf)
return -ENOMEM;
buf[0] = report_id;
memcpy(buf + 1, cmd, size);
...
ret = hid_hw_raw_request(steam->hdev, report_id,
buf, max(size + 1, len),
HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
...
}
Since only size bytes are copied into buf, will the remaining bytes up to
max(size + 1, len) contain uninitialized memory when sent over the bus?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812011401.3250968-1-vi@endrift.com?part=5