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 <[email protected]>

HID: steam: Clean up locking

This cleans up several issues with locking behavior, including RCU accesses
not being guarded behind a lock.

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

[ ... ]

> @@ -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/[email protected]?part=5