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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `steam_register()` fails during `steam_probe()`, the device is left in the global `steam_devices` list, leading to a Use-After-Free.
- [High] In `steam_remove()`, workqueues are cancelled before stopping the HID hardware and unregistering input devices, allowing works to be re-queued and executed after the device is freed.
--

--- Patch [6]: [PATCH v3 06/10] HID: steam: Fully unregister controller when hidraw is opened ---
commit b7eebc07a61af5a54e2737e2f58726a8f773d095
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Fully unregister controller when hidraw is opened

To avoid conflicts between anything touching the hidraw and the driver we
had previously detached the evdev nodes when the hidraw is opened. However,
this isn't sufficient to avoid FEATURE reports from conflicting, so we
change to fully unregistering the controller internally, leaving only the
hidraw active until it's closed.

This also unifies the unregister and connect callbacks, as now the logic
between these two callbacks is identical.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 5537488139011..0771ad4f5f2c2 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -1451,7 +1429,6 @@ static int steam_probe(struct hid_device *hdev,
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	cancel_work_sync(&steam->unregister_work);
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a failed
probe to leak the device pointer in the global steam_devices list?

If steam_register() fails during a wired controller's steam_probe(), the
failure causes steam_probe() to jump to an error label that bypasses
err_steam_unregister, which would have cleanly removed it via
steam_unregister().

The devres subsystem then frees the steam structure. Any subsequent iteration
of steam_devices—such as modifying the lizard_mode module parameter via sysfs
or plugging in another controller—will dereference this freed memory.

> @@ -1470,7 +1447,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);
>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;

[Severity: High]
This is a pre-existing issue, but does cancelling the workqueues here create
a race condition?

In steam_remove(), workqueues (like rumble_work and work_connect) are
cancelled early in the teardown sequence. However, hid_hw_stop() is called
after these cancellations, meaning the HID transport is still active and
incoming reports can trigger steam_do_connect_event(), which re-queues
work_connect.

Furthermore, steam_unregister() is called at the very end of steam_remove().
Because the input device is still active, userspace can send force-feedback
events that call steam_play_effect(), re-queuing rumble_work and
coalesce_rumble_work.

Since these works are re-queued after the cancel_work_sync() calls, could
they execute after the steam structure is freed by devres, causing a
Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710020048.2337655-1-vi@endrift.com?part=6