Re: [PATCH 4/5] HID: steam: Clean up locking
From: sashiko-bot@kernel.org
Date: 2026-08-05 02:16:03
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: New issues: - [High] Self-deadlock in `steam_unregister` due to `report_mutex` acquisition. - [High] AB-BA lock inversion between `steam_devices_lock` and `steam->report_mutex`. - [Medium] Inverted lizard mode logic in `steam_param_set_lizard_mode` breaks the parameter. - [Medium] Missing `report_mutex` in `steam_get_conn_status` creates a race condition with concurrent hardware access. Pre-existing issues: - [High] Use-after-free caused by cancelling workqueues before stopping the hardware. - [High] Resource leak and use-after-free of `power_supply` object on probe failure. --
--- Patch [4]: [PATCH 4/5] HID: steam: Clean up locking ---commit 4a13ea9d8650f6b804ba3ebe1f98c65bb697f0c4 Author: Vicki Pfau [off-list ref] HID: steam: Clean up locking This cleans up several issues with locking behavior, including RCU accesses not being guarded behind a lock.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index 2d60b79441b69..87af4bee9f5ff 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -728,7 +727,6 @@ static int steam_get_conn_status(struct steam_device *steam) else report_id = 0; - guard(mutex)(&steam->report_mutex); ret = steam_send_report_id(steam, cmd, sizeof(cmd), report_id);
[Severity: Medium] Will this removal create a race condition with concurrent hardware access? During steam_probe() in hid-steam.c, the hardware is opened and interrupts are enabled before steam_get_conn_status() is called. If the device immediately sends a connection event, the interrupt handler schedules steam_work_connect_cb(), which acquires report_mutex and sends feature reports. Without the mutex here, can the control transfers from steam_get_conn_status() interleave with the worker's transfers and confuse the controller firmware? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1728,7 +1728,9 @@ static int steam_probe(struct hid_device *hdev, steam->connected = true; } if (steam->connected) { + mutex_lock(&steam->report_mutex); ret = steam_register(steam); + mutex_unlock(&steam->report_mutex); if (ret) { hid_err(hdev, "%s:steam_register failed with error %d\n", __func__, ret); goto err_hw_close;
[Severity: High] This isn't a bug introduced by this patch, but does steam_probe() leak the power_supply object if steam_register() fails? If steam_input_register() or steam_sensors_register() fails inside steam_register(), it jumps to an error label and returns an error without unregistering the battery created via steam_battery_register(). The error propagates here, skipping steam_unregister() and jumping to err_hw_close. Doesn't this leave the power_supply device registered while devm frees the steam_device, leading to a use-after-free when userspace interacts with the stale power_supply? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1787,19 +1789,17 @@ static void steam_remove(struct hid_device *hdev) if (steam->quirks & STEAM_QUIRK_WIRELESS) { hid_info(hdev, "Steam wireless receiver disconnected"); } + guard(mutex)(&steam->report_mutex); steam_unregister(steam); hid_hw_stop(hdev); }
[Severity: High] Can holding report_mutex across steam_unregister() cause a self-deadlock? When a device is removed, steam_remove() acquires report_mutex here and calls steam_unregister(). steam_unregister() calls cancel_work_sync() on rumble_work, and steam_input_unregister(). If the rumble work is running in steam_haptic_rumble_cb(), it will attempt to acquire the same report_mutex and block forever. Additionally, unregistering the input device triggers the steam_input_close() callback, which also attempts to acquire this non-recursive report_mutex. [Severity: High] This is a pre-existing issue, but is there a use-after-free window here in steam_remove() due to the order of teardown? cancel_work_sync() for work_connect is called just before the quoted code, but hid_hw_stop() is called at the end. If an interrupt arrives between the cancellation and hid_hw_stop(), steam_raw_event() can process it and re-queue the work. When steam_remove() returns, devm frees the steam_device, but the queued work could later execute steam_work_connect_cb() on the freed memory. [ ... ]
quoted hunk ↗ jump to hunk
@@ -2631,6 +2624,8 @@ static int steam_param_set_lizard_mode(const char *val, { struct steam_device *steam; int ret; + bool client_opened; + unsigned long flags; ret = param_set_bool(val, kp); if (ret)@@ -2638,8 +2633,13 @@ static int steam_param_set_lizard_mode(const char *val, mutex_lock(&steam_devices_lock); list_for_each_entry(steam, &steam_devices, list) { - if (!steam->client_opened) + spin_lock_irqsave(&steam->lock, flags); + client_opened = steam->client_opened; + spin_unlock_irqrestore(&steam->lock, flags); + if (client_opened) { + guard(mutex)(&steam->report_mutex); steam_set_lizard_mode(steam, lizard_mode); + } } mutex_unlock(&steam_devices_lock); return 0;
[Severity: High] Does acquiring report_mutex while holding steam_devices_lock introduce an lock inversion? Here in steam_param_set_lizard_mode(), steam_devices_lock is acquired first, followed by report_mutex. However, concurrently in steam_probe() or steam_work_connect_cb(), report_mutex is acquired first, and then steam_register() is called, which attempts to acquire steam_devices_lock to add the device to the list. Could this lead to an indefinite deadlock? [Severity: Medium] Was the condition intentionally changed from !steam->client_opened to if (client_opened)? It seems like this applies the lizard mode settings to devices actively controlled by the Steam client, and ignores devices that actually need the fallback mappings. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260805015116.3884465-1-vi@endrift.com?part=4