Re: [PATCH v5 5/6] HID: steam: Clean up locking

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.

[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?

[ ... ]

[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