Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: steam: Race in steam_unregister() causes UAF on probe error path
- [Medium] hid: steam: TOCTOU race in lizard mode management due to cached client_opened state
--
commit 0277b6436a42d5feb84d587b977aa32f278d2fe5
Author: Vicki Pfau [off-list ref]
HID: steam: Clean up locking
This patch 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 aa001a293a361..bbc150167a7f1 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -294,22 +295,30 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
static int steam_input_open(struct input_dev *dev)
{
struct steam_device *steam = input_get_drvdata(dev);
+ unsigned long flags;
+ bool client_opened;
- mutex_lock(&steam->mutex);
- if (!steam->client_opened && lizard_mode)
+ spin_lock_irqsave(&steam->lock, flags);
+ client_opened = steam->client_opened;
+ spin_unlock_irqrestore(&steam->lock, flags);
+ guard(mutex)(&steam->mutex);
+ if (!client_opened && lizard_mode)
steam_set_lizard_mode(steam, false);
- mutex_unlock(&steam->mutex);
return 0;
}
[Severity: Medium]
Could this split locking pattern introduce a race condition?
The client_opened state is cached under the spinlock but acted upon later
under the mutex. A concurrent hidraw client could open or close the device
(modifying steam->client_opened via steam_client_ll_open or close) in the
window between dropping the spinlock and acquiring the mutex.
This time-of-check to time-of-use race could result in the driver making
incorrect lizard mode state transitions based on a stale client connection
status.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -565,9 +578,11 @@ static void steam_unregister(struct steam_device *steam)
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
steam->serial_no);
+ mutex_lock(&steam->registration_mutex);
steam->registered = false;
steam_battery_unregister(steam);
steam_input_unregister(steam);
[Severity: High]
Does this lockless check of steam->registered at the top of steam_unregister()
create a race condition with the asynchronous registration work?
In steam_probe() for a wireless device, a connect event can schedule
steam_work_connect_cb(), which begins executing steam_register(). If
steam_probe() fails (for example, when allocating client_hdev), it jumps to
err_stream_unregister and calls steam_unregister():
steam_probe()
err_stream_unregister:
if (steam->connected)
steam_unregister(steam);
Because steam_unregister() checks if (!steam->registered) locklessly right
before the quoted section above, it returns immediately if steam_register()
hasn't set steam->registered = true yet.
After returning, steam_probe() calls cancel_work_sync(), which blocks until
steam_register() finishes registering the input and battery devices. Then
steam_probe() returns an error and devres frees the steam struct. Could this
leave the registered input and battery devices with dangling pointers,
leading to a use-after-free when userspace interacts with them?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1108,6 +1118,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)@@ -1115,10 +1127,13 @@ static int steam_param_set_lizard_mode(const char *val,
mutex_lock(&steam_devices_lock);
list_for_each_entry(steam, &steam_devices, list) {
- mutex_lock(&steam->mutex);
- 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->mutex);
steam_set_lizard_mode(steam, lizard_mode);
- mutex_unlock(&steam->mutex);
+ }
}
mutex_unlock(&steam_devices_lock);
return 0;
[Severity: Medium]
Is this another instance of the same time-of-check to time-of-use race?
This pattern seems to allow the driver to apply a lizard mode change even if
a client just opened the device, which might improperly interfere with the
raw client's control over the controller.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091204.529049-1-lee@kernel.org?part=6