Re: [PATCH 4/5] HID: steam: Clean up locking

[sashiko-bot@kernel.org]

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 <vi@endrift.com>

HID: steam: Clean up locking

This cleans up several issues with locking behavior, including RCU accesses
not being guarded behind a lock.

> 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

[ ... ]

> @@ -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?

[ ... ]

> @@ -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?

[ ... ]

> @@ -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.

[ ... ]

> @@ -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