Re: [PATCH v4 07/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] Unifying connect and unregister logic causes log spam and unnecessary I/O on hidraw open/close.

Pre-existing issues:
- [High] Dangling pointer in the global `steam_devices` list on probe failure.
--

--- Patch [7]: [PATCH v4 07/11] HID: steam: Fully unregister controller when hidraw is opened ---
commit d6b711a20faba4cc20306f07e01853b79872c545
Author: Vicki Pfau <[email protected]>

HID: steam: Fully unregister controller when hidraw is opened

To avoid conflicts between anything touching the hidraw and the driver, this
patch fully unregisters the controller internally when hidraw is opened,
leaving only the hidraw active until it's closed. It also unifies the
unregister and connect callbacks.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a200878db8ac7..ff112aa1b0e91 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1141,38 +1142,41 @@ static int steam_register(struct steam_device *steam)
[ ... ]
>  static void steam_work_connect_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							work_connect);
> +
>  	unsigned long flags;
>  	bool connected;
> +	bool opened;
>  	int ret;
>  
>  	spin_lock_irqsave(&steam->lock, flags);
> +	opened = steam->client_opened;
>  	connected = steam->connected;
>  	spin_unlock_irqrestore(&steam->lock, flags);
>  
> -	if (connected) {
> +	if (connected && !opened) {
>  		ret = steam_register(steam);

[Severity: High]
This is a pre-existing issue, but does steam_register() leak the list
entry if it fails?

If steam_register() fails (e.g., if input_allocate_device() fails inside
steam_input_register()), the device is added to the global steam_devices
list but not removed.

Also, in steam_probe(), if a wired Steam controller is probed but an error
occurs later, the error path jumps to err_hw_stop, bypassing
err_steam_unregister. Devres will free the steam_device structure, leaving a
dangling pointer in steam_devices. Could this lead to a use-after-free if the
list is later traversed (e.g., when a user modifies lizard_mode)?

> -		if (ret) {
> +		if (ret)
>  			hid_err(steam->hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -		}
>  	} else {
>  		steam_unregister(steam);
>  	}

[Severity: Medium]
By unifying the connect and unregister logic here, will this cause log spam
and unnecessary synchronous USB I/O when hidraw is opened and closed?

steam_unregister() clears the serial number and logs a disconnect message.
When hidraw is closed, steam_register() logs a connect message and performs
synchronous USB I/O to re-fetch the serial number and device attributes.

Could an unprivileged userspace application rapidly open and close the
hidraw node (which is typical for gamepads managed by udev), flooding the
kernel log and triggering localized DoS via USB I/O?

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