Re: [PATCH v5 07/11] HID: steam: Rearrange teardown sequence

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

Pre-existing issues:
- [High] Use-after-free due to `work_connect` rescheduling after cancellation in `steam_remove`.
- [High] Race condition leading to use-after-free in `steam_probe` error path.
- [High] Lockless TOCTOU race in `steam_raw_event` leads to Use-After-Free of `steam->client_hdev`.
--

--- Patch [7]: [PATCH v5 07/11] HID: steam: Rearrange teardown sequence ---
commit 0f9077495b7412b91d8e827f54f32dc26780b709
Author: Vicki Pfau <[email protected]>

HID: steam: Rearrange teardown sequence

This patch reorders the teardown sequence for the Steam controller driver to
address a narrow window where callbacks could be scheduled with a dangling
pointer. It also moves the cancellation of rumble and mode switch work to
steam_unregister().

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 12203d61922f7..663fda8a86fdb 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1441,25 +1444,24 @@ static int steam_probe(struct hid_device *hdev,
>  static void steam_remove(struct hid_device *hdev)
>  {
>  	struct steam_device *steam = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
>  	if (!steam || hdev->group == HID_GROUP_STEAM) {
>  		hid_hw_stop(hdev);
>  		return;
>  	}
>  
> +	hid_hw_close(hdev);
>  	hid_destroy_device(steam->client_hdev);
> -	cancel_delayed_work_sync(&steam->mode_switch);
> -	cancel_work_sync(&steam->work_connect);
> -	cancel_work_sync(&steam->rumble_work);
> -	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	steam->client_hdev = NULL;
> +	spin_lock_irqsave(&steam->lock, flags);
>  	steam->client_opened = 0;

[Severity: High]
This is a pre-existing issue, but does this sequence leave a window for a
use-after-free on steam->client_hdev?

In steam_remove(), hid_destroy_device(steam->client_hdev) is called to free
the virtual client device while the hardware is still active, since
hid_hw_stop() has not been called yet.

Meanwhile, steam_raw_event() locklessly checks if the client is opened before
dereferencing steam->client_hdev:

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

If an interrupt fires and passes the client_opened check, could it get
preempted, and then resume to call hid_input_report() on the freed
client_hdev pointer after steam_remove() completes the destruction?

> +	spin_unlock_irqrestore(&steam->lock, flags);
> +	cancel_work_sync(&steam->work_connect);
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);
> +	hid_hw_stop(hdev);
>  }

[Severity: High]
This is a pre-existing issue, but can steam->work_connect still be
rescheduled after it is cancelled here?

In steam_remove(), cancel_work_sync(&steam->work_connect) is called, but
hid_hw_stop(hdev) isn't called until the very end.

If a wireless connect or disconnect event arrives in between,
steam_do_connect_event() unconditionally reschedules the work:

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

When steam_remove() finishes, the steam object is freed by devres, leaving a
dangling work item in the system workqueue. Will this lead to a
use-after-free when the scheduled work executes?

[Severity: High]
This is another pre-existing issue, but does the error path in steam_probe()
have a similar race condition with work_connect?

In the steam_probe() error path, steam_unregister() is called before
cancel_work_sync(&steam->work_connect):

drivers/hid/hid-steam.c:steam_probe() {
    ...
err_steam_unregister:
    if (steam->connected)
        steam_unregister(steam);
err_hw_close:
    hid_hw_close(hdev);
err_hw_stop:
    hid_hw_stop(hdev);
err_cancel_work:
    cancel_work_sync(&steam->work_connect);
    ...
}

steam_unregister() clears the device state by setting steam->serial_no[0] = 0.

If steam_work_connect_cb() is currently queued and runs concurrently, it
will see connected == true and call steam_register().

Because serial_no was cleared, steam_register() re-registers all input
devices and adds the steam object back to the global steam_devices list.

After the work finishes, cancel_work_sync() silently returns, steam_probe()
returns an error, and the steam object is freed by devres. Does this leave a
freed object in the global list and registered to input subsystems, leading
to a use-after-free?

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