Re: [STABLE v6.1 6/6] HID: steam: Clean up locking

[sashiko-bot@kernel.org]

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

HID: steam: Clean up locking

This patch 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 aa001a293a361..bbc150167a7f1 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

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

[ ... ]

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

[ ... ]

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