Re: [PATCH v4 08/11] HID: steam: Rearrange teardown sequence

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

New issues:
- [High] Calling `hid_hw_start()` before initializing driver data structures exposes the device to events early, leading to crashes from uninitialized locks and work_structs.
- [High] The modified error path in `steam_probe()` skips the cancellation of `work_connect` and `mode_switch`, leaving them dangling on probe failure.

Pre-existing issues:
- [High] Teardown ordering in `steam_remove()` allows `work_connect` to be re-scheduled by raw events after it has been explicitly cancelled.
- [High] `steam->client_hdev` is destroyed before the `client_opened` flag is cleared, allowing asynchronous events to dereference the freed device.
- [High] If `steam_register()` fails during initialization, the `steam` object is freed but remains linked in the global `steam_devices` list.
--

--- Patch [8]: [PATCH v4 08/11] HID: steam: Rearrange teardown sequence ---
commit 4778053b21e62ca367e7570ac85b4132006adade
Author: Vicki Pfau <[email protected]>

HID: steam: Rearrange teardown sequence

This fixes a narrow window during the teardown sequence where callbacks
could still be scheduled during cleanup that would then have a dangling
pointer to the now-freed steam struct. This also puts work canceling
for rumble and mode switch in steam_unregister, as that shouldn't persist
while the client hdev is open.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index ff112aa1b0e91..5ab4396a241e0 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1352,6 +1356,14 @@ static int steam_probe(struct hid_device *hdev,
>  	if (!steam)
>  		return -ENOMEM;
>  
> +	/*
> +	 * With the real steam controller interface, do not connect hidraw.
> +	 * Instead, create the client_hid and connect that.
> +	 */
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;
> +
>  	steam->hdev = hdev;
>  	hid_set_drvdata(hdev, steam);
>  	spin_lock_init(&steam->lock);

[Severity: High]
Does calling hid_hw_start() before initializing driver data structures expose
the device to events early?

Moving hid_hw_start() here makes the input devices visible to userspace.
If a userspace process opens the input node, hid_hw_open() is called, starting
URB polling. An incoming hardware interrupt can then invoke steam_raw_event().

Because hid_set_drvdata() is called right after hid_hw_start(),
steam_raw_event() could access the non-NULL steam pointer and call
steam_do_connect_event(), which might attempt to acquire the uninitialized
spinlock and schedule the uninitialized work_connect, potentially leading to
a crash.

[ ... ]
> @@ -1397,14 +1393,22 @@ static int steam_probe(struct hid_device *hdev,
>  			hid_err(hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -			goto err_hw_close;
> +			goto err_hw_stop;

[Severity: High]
This is a pre-existing issue, but if steam_register() fails during
initialization, does the steam object remain linked in the global
steam_devices list after being freed?

During steam_register(), the device is added to the global steam_devices list.
If a subsequent step like steam_input_register() fails, the function returns
an error without removing the device from the list.

When this failure jumps to err_hw_stop in steam_probe(), it bypasses
steam_unregister(), which is responsible for removing it from the list.
If devres then frees the object, could a dangling pointer be left in the
global list?

[ ... ]
>  err_destroy:
>  	hid_destroy_device(steam->client_hdev);
> -err_steam_unregister:
> -	if (steam->connected)
> -		steam_unregister(steam);
>  err_hw_close:
>  	hid_hw_close(hdev);
> -err_hw_stop:
> -	hid_hw_stop(hdev);
> -err_cancel_work:
> +err_steam_unregister:
> +	spin_lock_irqsave(&steam->lock, flags);
> +	steam->client_opened = 0;
> +	spin_unlock_irqrestore(&steam->lock, flags);
>  	cancel_work_sync(&steam->work_connect);
> +	if (steam->connected)
> +		steam_unregister(steam);
>  	cancel_delayed_work_sync(&steam->mode_switch);
> +err_hw_stop:

[Severity: High]
Does the modified error path in steam_probe() skip the cancellation of
work_connect and mode_switch when a probe failure jumps to err_hw_stop?

Because hid_hw_start() was moved earlier in the probe, raw events might already
queue work_connect. If the probe fails and jumps directly to err_hw_stop,
bypassing err_steam_unregister, could it leave these work items dangling when
devres frees the steam object?

>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> +	hid_hw_stop(hdev);
>  
>  	return ret;
>  }
> @@ -1435,25 +1441,24 @@ static int steam_probe(struct hid_device *hdev,
>  static void steam_remove(struct hid_device *hdev)
>  {
>  	struct steam_device *steam = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
>  	if (!steam || hdev->group == HID_GROUP_STEAM) {
>  		hid_hw_stop(hdev);
>  		return;
>  	}
>  
> +	hid_hw_close(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;
> +	spin_lock_irqsave(&steam->lock, flags);
>  	steam->client_opened = 0;
> +	spin_unlock_irqrestore(&steam->lock, flags);

[Severity: High]
This is a pre-existing issue, but is steam->client_hdev destroyed before
the client_opened flag is cleared?

Because steam_raw_event() runs in softirq context without taking a lock around
the client_opened check, there could be a race condition.

If an interrupt sees client_opened > 0 right before hid_destroy_device()
completes, could it pass the freed client_hdev pointer to hid_input_report()?

> +	cancel_work_sync(&steam->work_connect);
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);
> +	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but does the teardown ordering in steam_remove()
allow work_connect to be re-scheduled by raw events after it has been explicitly
cancelled?

cancel_work_sync(&steam->work_connect) is called above, but URB polling is only
guaranteed to stop at the end of the function when hid_hw_stop() is called.

If a hardware event arrives after the cancellation but before hid_hw_stop(),
could it re-queue work_connect via steam_raw_event(), leaving the work pending
after the driver finishes removal and frees the device?

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