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