Re: [PATCH 07/10] HID: steam: Rearrange deinitialization sequence

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free due to work item rescheduling during device removal
- [High] Double-free and Use-After-Free due to lockless concurrent execution of steam_unregister()
--

commit 3f510b9fc8a4fe35042e364f53f44a52a7f1b526
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Rearrange deinitialization sequence
    
This fixes a narrow window during the deinitialization where callbacks
could still be scheduled during cleanup that would then have a dangling
pointer to the now-freed steam struct.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1443,18 +1443,18 @@ static void steam_remove(struct hid_device *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;
> +	hid_hw_close(hdev);
> +	cancel_work_sync(&steam->work_connect);

[Severity: High]
This is a pre-existing issue, but does this still leave a window where the
work item can be rescheduled?

Because hid_hw_stop() has not been called yet, the hardware can still deliver
a raw event like ID_CONTROLLER_WIRELESS. This calls steam_raw_event() which
calls steam_do_connect_event() and unconditionally queues the work item:

steam_do_connect_event()
    if (changed && schedule_work(&steam->work_connect) == 0)

If this happens after cancel_work_sync() but before hid_hw_stop(), will the
delayed work item execute on a freed steam struct?

>  	steam->client_opened = 0;
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);

[Severity: High]
This is a pre-existing issue, but could concurrent calls to steam_unregister()
result in a double-free?

If a wireless disconnect event arrives concurrently with a device unplug,
the worker thread executing steam_work_connect_cb() and the unplug thread
executing steam_remove() might both call steam_unregister().

Since steam_unregister() relies on a lockless check of steam->serial_no[0],
could both threads pass the check simultaneously and call
steam_input_unregister()? Both threads would then read steam->input via RCU
and call input_unregister_device() on the same pointer.

Also, does this lack of serialization allow the probe error path to race
with steam_register(), potentially leaving active input devices pointing
to a freed steam struct?

> +	cancel_work_sync(&steam->rumble_work);
> +	cancel_delayed_work_sync(&steam->mode_switch);
> +	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> +	hid_hw_stop(hdev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702222145.1863104-1-vi@endrift.com?part=7