Re: [PATCH v5 04/11] HID: steam: Add support for sensor events on the Steam Controller (2015)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Global list dangling pointer and Use-After-Free when steam_register fails during probe.
- [High] Race condition leading to double-free of input devices in workqueue callbacks.
--

--- Patch [4]: [PATCH v5 04/11] HID: steam: Add support for sensor events on the Steam Controller (2015) ---
commit c819d5d2af295f0bbbbb6b163562daebd8e1a93c
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Add support for sensor events on the Steam Controller (2015)

Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 983d18d1de4fe..1ceb044e170b4 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -918,9 +1030,6 @@ static void steam_sensors_unregister(struct steam_device *steam)
>  {
>  	struct input_dev *sensors;
>  
> -	if (!(steam->quirks & STEAM_QUIRK_DECK))
> -		return;
> -
>  	rcu_read_lock();
>  	sensors = rcu_dereference(steam->sensors);
>  	rcu_read_unlock();

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
leading to a double-free of the input devices in the workqueue callbacks?

If a device disconnects at the exact time a user opens the raw HID node,
steam->work_connect and steam->unregister_work can execute concurrently
on different CPUs. Both callbacks invoke steam_sensors_unregister() and
steam_input_unregister().

In steam_input_unregister(), the check-and-clear logic is not protected by a
mutex or lock:

drivers/hid/hid-steam.c:steam_input_unregister() {
	rcu_read_lock();
	input = rcu_dereference(steam->input);
	rcu_read_unlock();
	if (!input)
		return;
	RCU_INIT_POINTER(steam->input, NULL);
	synchronize_rcu();
	input_unregister_device(input);
}

Could both concurrent threads read a non-null pointer, drop the RCU lock,
and then both call input_unregister_device() on the same device?

[ ... ]

> @@ -968,6 +1077,12 @@ static int steam_register(struct steam_device *steam)
>  			strscpy(steam->serial_no, "XXXXXXXXXX",
>  					sizeof(steam->serial_no));
>  
> +		ret = steam_get_attributes(steam);
> +		if (ret < 0)
> +			hid_err(steam->hdev,
> +				"%s:steam_get_attributes failed with error %d\n",
> +				__func__, ret);
> +
>  		hid_info(steam->hdev, "Steam Controller '%s' connected",
>  				steam->serial_no);

[Severity: High]
This is a pre-existing issue, but could the error path in steam_register()
leave a dangling pointer in the global steam_devices list?

If steam_register() encounters an error during a wired device probe, such as
steam_input_register() failing to allocate memory, it returns an error code
but leaves the steam object on the global list:

drivers/hid/hid-steam.c:steam_register() {
...
steam_register_sensors_fail:
	steam_input_unregister(steam);
steam_register_input_fail:
	return ret;
}

The caller, steam_probe(), then jumps to the err_hw_close label, skipping
the err_steam_unregister label where the list state would be cleaned up:

drivers/hid/hid-steam.c:steam_probe() {
...
		ret = steam_register(steam);
		if (ret) {
			hid_err(hdev,
				"%s:steam_register failed with error %d\n",
				__func__, ret);
			goto err_hw_close;
		}
...
}

Because the steam object is managed by devm_kzalloc(), the kernel
automatically frees the memory when probe fails. A subsequent write to the
lizard_mode module parameter will trigger steam_param_set_lizard_mode(),
which iterates over the list and accesses the freed memory, leading to a
use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730041251.1659029-1-vi@endrift.com?part=4