Re: [PATCH v4 4/6] HID: steam: Don't set feature reports when disconnecting

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

Pre-existing issues:
- [High] Use-after-free of `steam->client_hdev` in `steam_raw_event` due to lockless state read and improper teardown order.
- [High] Use-after-free of the `steam` object due to delayed works being scheduled after they are cancelled in `steam_remove`.
--

--- Patch [4]: [PATCH v4 4/6] HID: steam: Don't set feature reports when disconnecting ---
commit a4f986bc6df63f014c012a55b0b8e0ff17917cf8
Author: Vicki Pfau <[email protected]>

HID: steam: Don't set feature reports when disconnecting

This commit updates the driver to fail silently when setting feature reports
fails with -ENODEV during device disconnect. It prevents expected errors
from flooding the kernel log when lizard mode and IMU mode are reset.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 8d3ef523682bd..d95296b28f92b 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -570,7 +574,11 @@ static int steam_send_report_id(struct steam_device *steam,
>  	} while (--retries);
>  
>  	kfree(buf);
> -	if (ret < 0)
> +	/*
> +	 * Don't log if the failure is -ENODEV, as this
> +	 * can happen normally on disconnect.
> +	 */
> +	if (ret < 0 && ret != -ENODEV)
>  		hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
>  				ret, size, cmd);
>  	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the disconnect
handling, is it possible for the interrupt handler to access freed memory?

Looking at steam_remove(), the client device is destroyed before the
client_opened flag is cleared and before hardware interrupts are stopped:

drivers/hid/hid-steam.c:steam_remove() {
    ...
    hid_destroy_device(steam->client_hdev);
    spin_lock_irqsave(&steam->lock, flags);
    steam->client_opened = 0;
    ...
}

If a USB report arrives in this window, could steam_raw_event() locklessly
read steam->client_opened (which still evaluates to true) and call
hid_input_report() on the already freed steam->client_hdev?

drivers/hid/hid-steam.c:steam_raw_event() {
    ...
    if (steam->client_opened)
        hid_input_report(steam->client_hdev, report->type, data, size, 0);
    ...
}

[Severity: High]
This is also a pre-existing issue, but does steam_remove() leave a window
where cancelled works can be re-queued?

In steam_remove(), the works are cancelled and the device is unregistered
before hardware interrupts are finally stopped via hid_hw_stop():

drivers/hid/hid-steam.c:steam_remove() {
    ...
    cancel_work_sync(&steam->work_connect);
    ...
    steam_unregister(steam);
    hid_hw_stop(hdev);
    ...
}

Because hardware interrupts can remain active in this window, incoming HID
events can still trigger steam_raw_event(). If a user presses the start
button in this window, could steam_do_deck_input_event() queue the
mode_switch delayed work?

drivers/hid/hid-steam.c:steam_do_deck_input_event() {
    ...
    schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
    ...
}

Similarly, could incoming wireless events trigger steam_do_connect_event()
and queue work_connect after it was already cancelled?

drivers/hid/hid-steam.c:steam_do_connect_event() {
    ...
    if (changed && schedule_work(&steam->work_connect) == 0)
    ...
}

Once steam_remove() returns, the steam struct is freed via devres. Could
these pending works then execute on the freed steam object, causing a
use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4