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

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

New issues:
- [Medium] The removal of the explicit query workaround in `steam_write_settings` can cause driver initialization to fail on subsequent driver binds or module reloads due to lingering reports.

Pre-existing issues:
- [High] Use-After-Free due to HID events triggering work re-queuing during teardown because `hid_hw_stop` is called after `cancel_work_sync`.
- [High] Concurrent execution of `steam_register` leads to resource leaks and Use-After-Free/Double-Free on error paths due to missing locking.
- [High] Race condition in teardown functions (`steam_battery_unregister`, `steam_input_unregister`, etc.) leads to double-free of device components.
--

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

HID: steam: Initial 2026 Steam Controller support

This brings support for the 2026 Steam Controller, comparably featureful to
the existing support for the Steam Deck.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 967f12c7a599a..6476fbdc58886 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -489,16 +615,7 @@ static int steam_write_settings(struct steam_device *steam,
>  	}
>  	va_end(args);
>  
> -	ret = steam_send_report(steam, cmd, 2 + cmd[1]);
> -	if (ret < 0)
> -		return ret;
> -
> -	/*
> -	 * Sometimes a lingering report for this command can
> -	 * get read back instead of the last set report if
> -	 * this isn't explicitly queried
> -	 */
> -	return steam_recv_report(steam, cmd, 2 + cmd[1]);
> +	return steam_send_report(steam, cmd, 2 + cmd[1]);

[Severity: Medium]
By removing the explicit steam_recv_report() call here, could a lingering
report be left in the device buffer?

If the module is later reloaded or the driver rebound, steam_probe() will
call steam_get_conn_status() which expects a wireless state report.

>  }

[ ... ]

> -/*
> - * This command requests the wireless adaptor to post an event
> - * with the connection status. Useful if this driver is loaded when
> - * the controller is already connected.
> - */
> -static inline int steam_request_conn_status(struct steam_device *steam)
> +static int steam_get_conn_status(struct steam_device *steam)
>  {
> +	int ret = 0;
> +	u8 cmd[] = {ID_DONGLE_GET_WIRELESS_STATE};
> +	u8 reply[3] = {};
> +	u8 report_id;
> +
> +	if (steam->quirks & STEAM_QUIRK_IBEX)
> +		report_id = REPORT_ID_FEATURES_DONGLE;
> +	else
> +		report_id = 0;
> +
>  	guard(mutex)(&steam->report_mutex);
> -	return steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE);
> +	ret = steam_send_report_id(steam, cmd, sizeof(cmd), report_id);
> +	if (ret < 0)
> +		return ret;
> +	ret = steam_recv_report_id(steam, reply, sizeof(reply), report_id);
> +	if (ret < 0)
> +		return ret;
> +	if (reply[0] != ID_DONGLE_GET_WIRELESS_STATE || reply[1] < 1) {
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				(int)sizeof(reply), reply);
> +		return -EIO;
> +	}
> +
> +	return reply[2];
>  }

[Severity: Medium]
Does this function read the lingering ID_SET_SETTINGS_VALUES report instead of
the expected status?

Because this lacks a retry mechanism, it appears this would fail the
reply[0] != ID_DONGLE_GET_WIRELESS_STATE check and return -EIO, breaking
device initialization.

[ ... ]

> @@ -1149,51 +1377,42 @@ static int steam_register(struct steam_device *steam)
>  	 * 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->serial_no[0])
> +		return 0;

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

If a wireless connect event triggers steam_work_connect_cb() simultaneously
with steam_probe() invoking steam_get_conn_status(), could both threads
evaluate steam->serial_no[0] as empty because there is no locking here?

If so, both could proceed to allocate and register input devices and
batteries.

Could this potentially overwrite the pointers and leak the first set, or
cause a use-after-free when one thread fails and runs the error cleanup
path on resources the other thread just allocated?

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does the teardown sequence in
steam_battery_unregister() and steam_input_unregister() have a race condition?

If these unregister functions are called concurrently (for example, if
steam_register() fails on one thread while steam_work_connect_cb()
processes a disconnect event on another), both threads could read the
same component pointer within the RCU read-side critical section before
either thread executes RCU_INIT_POINTER(..., NULL).

Would this cause both threads to call power_supply_unregister() and
devm_kfree() on the exact same pointer, leading to a double-free?

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the sequence in steam_remove() lead to
a use-after-free?

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

Because hid_hw_stop() is called after cancel_work_sync(), the HID transport can
still deliver events during this window.

If an event like REPORT_ID_WIRELESS_EVENT arrives, steam_raw_event() will call
steam_do_connect_event(), which calls schedule_work(&steam->work_connect),
re-queuing the work after it was cancelled.

Once steam_remove() returns and devres frees the steam_device structure,
will the workqueue run steam_work_connect_cb() on the freed memory?

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