Re: [PATCH v3 2/6] HID: steam: Initial 2026 Steam Controller support

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

Pre-existing issues:
- [High] Concurrent execution of `steam_register` between `steam_probe` and `steam_work_connect_cb` leads to data races, double registration, memory leaks, and Use-After-Free (UAF).
- [High] Teardown ordering in `steam_remove` incorrectly cancels works before stopping hardware events, leading to Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/6] HID: steam: Initial 2026 Steam Controller support ---
commit d704660582137e2d95967bceb5a602df38e73e5e
Author: Vicki Pfau <[email protected]>
Subject: HID: steam: Initial 2026 Steam Controller support

This commit brings support for the 2026 Steam Controller, adding new
capabilities comparable to the Steam Deck. It also includes cleanup of
the registration flow to make it cleaner and avoid potential pitfalls.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 967f12c7a599a..ec7ccbe5aba09 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1139,61 +1371,49 @@ static void steam_battery_unregister(struct steam_device *steam)
>  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
>  	 * wireless adaptor, without steam_unregister() between them, because
>  	 * another client send a get_connection_status command, for example.
> -	 * The battery and serial number are set just once per device.
>  	 */
> -	if (!steam->serial_no[0]) {
> -		/*
> -		 * Unlikely, but getting the serial could fail, and it is not so
> -		 * important, so make up a serial number and go on.
> -		 */
> -		if (steam_get_serial(steam) < 0)
> -			strscpy(steam->serial_no, "XXXXXXXXXX",
> -					sizeof(steam->serial_no));
> -
> -		ret = steam_get_attributes(steam);
> -		if (ret < 0)
> -			hid_err(steam->hdev,
> -				"%s:steam_get_attributes failed with error %d\n",
> -				__func__, ret);
> +	if (steam->registered)
> +		return 0;

[Severity: High]
This is a pre-existing issue, but does this lockless check allow concurrent
registration?

When hid_hw_open() is called in steam_probe(), the hardware can immediately
generate a wireless connect event. This triggers steam_do_connect_event(),
which schedules steam_work_connect_cb() to run on a workqueue concurrently
with the remainder of steam_probe().

Both threads call steam_register(), which performs a lockless check of this
registration state. Both threads can see it as false and concurrently execute
the registration sequence.

This results in multiple concurrency bugs: steam_battery_register()
concurrently allocates and frees steam->battery_desc.name using
devm_kasprintf()/devm_kfree(), causing the power supply subsystem to access
freed memory (UAF). Additionally, input_allocate_device() is called twice
with the first pointer leaked via RCU pointer overwrite.

Does this need a lock around the registration check to prevent double
registration and memory corruption?

[ ... ]
> @@ -1219,7 +1440,6 @@ static void steam_unregister(struct steam_device *steam)
>  	mutex_lock(&steam_devices_lock);
>  	list_del_init(&steam->list);
>  	mutex_unlock(&steam_devices_lock);
> -	steam->serial_no[0] = 0;
>  }

[Severity: High]
This is a pre-existing issue, but does the teardown ordering in steam_remove()
incorrectly cancel works before stopping hardware events, leading to a UAF?

During device removal, cancel_work_sync(&steam->work_connect) and
steam_unregister() are called before hid_hw_stop(hdev):

static void steam_remove(struct hid_device *hdev)
{
        ...
        cancel_work_sync(&steam->work_connect);
        ...
        steam_unregister(steam);
        hid_hw_stop(hdev);
}

While hid_hw_close(hdev) is called earlier, it only drops the logical open
count and does not reliably guarantee the immediate cessation of all incoming
hardware events across all possible low-level transport drivers.

Because hid_hw_stop() is the definitive function for stopping the underlying
transport and killing pending events, an incoming interrupt can trigger
steam_raw_event() in the window after the works are canceled but before the
hardware is stopped.

If a connect or disconnect event is received, steam_raw_event() calls
steam_do_connect_event(), which queues steam->work_connect. After
steam_remove() completes, devres frees the steam_device structure. When the
newly scheduled work executes, it accesses the freed steam_device, resulting
in a UAF memory corruption.

Should hid_hw_stop() be called before canceling the workqueues?

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