Re: [PATCH v5 06/11] HID: steam: Fully unregister controller when hidraw is opened

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

New issues:
- [Medium] The local variable `do_add` in `steam_register()` can be evaluated uninitialized.

Pre-existing issues:
- [High] Teardown order in `steam_remove()` causes Use-After-Free and double-free vulnerabilities.
--

--- Patch [6]: [PATCH v5 06/11] HID: steam: Fully unregister controller when hidraw is opened ---
commit 14e48fa0937746f55f6b26048f8ecca7fe02bb3e
Author: Vicki Pfau <[email protected]>
Subject: HID: steam: Fully unregister controller when hidraw is opened

This patch changes the driver to fully unregister the controller
internally when hidraw is opened, leaving only the hidraw active until
it is closed. It also unifies the unregister and connect callbacks to
share identical logic.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 87b3817a2f69d..12203d61922f7 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -1084,6 +1085,7 @@ static int steam_register(struct steam_device *steam)
>  	int ret;
>  	unsigned long client_opened;
>  	unsigned long flags;
> +	bool do_add;
>  
>  	/*
>  	 * This function can be called several times in a row with the
> @@ -1113,10 +1115,7 @@ static int steam_register(struct steam_device *steam)
>  		if (steam->quirks & STEAM_QUIRK_WIRELESS)
>  			steam_battery_register(steam);
>  
> -		mutex_lock(&steam_devices_lock);
> -		if (list_empty(&steam->list))
> -			list_add(&steam->list, &steam_devices);
> -		mutex_unlock(&steam_devices_lock);
> +		do_add = true;
>  	}
>  
>  	spin_lock_irqsave(&steam->lock, flags);
> @@ -1132,6 +1131,13 @@ static int steam_register(struct steam_device *steam)
>  		if (ret != 0)
>  			goto steam_register_sensors_fail;
>  	}
> +
> +	if (do_add) {

[Severity: Medium]
Is it possible for do_add to be evaluated uninitialized here?

When steam_register() is invoked on an already-registered device,
steam->serial_no[0] is already populated. This causes the initialization block
for "if (!steam->serial_no[0])" to be skipped, meaning do_add is never
assigned a value. Could this undefined behavior lead to compiler
optimizations altering the control flow or causing redundant lock
acquisitions?

> +		mutex_lock(&steam_devices_lock);
> +		if (list_empty(&steam->list))
> +			list_add(&steam->list, &steam_devices);
> +		mutex_unlock(&steam_devices_lock);
> +	}
>  	return 0;

[ ... ]

> @@ -1470,7 +1452,6 @@ static void steam_remove(struct hid_device *hdev)
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	cancel_work_sync(&steam->unregister_work);

[Severity: High]
This is a pre-existing issue, but does the teardown order in steam_remove()
cause a Use-After-Free and double-free vulnerability?

Currently, work items like steam->work_connect are canceled before calling
hid_hw_stop(). Since the HID event stream is still active, an incoming
interrupt (such as a connection status report) can trigger steam_raw_event():

steam_raw_event()
  steam_do_connect_event()
    schedule_work(&steam->work_connect)

When steam_remove() completes, devres frees the steam_device struct, leaving
a dangling work item in the queue that triggers a UAF when executed.

Furthermore, if the rescheduled work executes concurrently with the end of
steam_remove(), both threads can invoke steam_unregister() simultaneously.
Because steam_unregister() lacks mutual exclusion, does this lead to
concurrent double-frees of the input and sensor devices?

>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {

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