Re: [PATCH v4 05/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 3 potential issue(s) to consider:

New issues:
- [Medium] Enabling lizard mode via gamepad closure implicitly disables the IMU, breaking active sensor listeners.

Pre-existing issues:
- [High] If `steam_register()` fails for a wired controller in `steam_probe()`, the `steam` device is freed while remaining linked in the global `steam_devices` list.
- [High] Concurrent execution of workqueues causes double unregistration of the input and sensors devices due to a TOCTOU race condition.
--

--- Patch [5]: [PATCH v4 05/11] HID: steam: Add support for sensor events on the Steam Controller (2015) ---
commit bb1476f66b80150be67610fe42e5546b316d06ef
Author: Vicki Pfau <vi@endrift.com>

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 8bad79205b570..cabecd250072e 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -625,6 +689,42 @@ static void steam_input_close(struct input_dev *dev)
>  	}
>  }
>  
> +static int steam_sensor_open(struct input_dev *dev)
> +{
> +	struct steam_device *steam = input_get_drvdata(dev);
> +	unsigned long flags;
> +	bool client_opened;
> +
> +	spin_lock_irqsave(&steam->lock, flags);
> +	client_opened = steam->client_opened;
> +	spin_unlock_irqrestore(&steam->lock, flags);
> +	if (client_opened)
> +		return 0;
> +
> +	guard(mutex)(&steam->report_mutex);
> +	steam_write_settings(steam, SETTING_IMU_MODE,
> +			SETTING_IMU_MODE_SEND_RAW_ACCEL | SETTING_IMU_MODE_SEND_RAW_GYRO,
> +			0);

[Severity: Medium]
Does closing the gamepad evdev node implicitly disable the IMU, breaking
active sensor listeners?

When the gamepad evdev node is closed, the driver automatically re-enables
"lizard mode" by calling steam_set_lizard_mode(steam, true) in
steam_input_close(). This function sends the ID_LOAD_DEFAULT_SETTINGS
command to the controller, which performs a hardware state reset.

This reset wipes out the SETTING_IMU_MODE configuration previously requested
by steam_sensor_open() here. Since the driver does not track or coordinate
the IMU state across these logically independent evdev nodes, closing the
gamepad node while the sensor node is still open will cause the controller
to silently stop sending motion data.

> +
> +	return 0;
> +}
> +
> +static void steam_sensor_close(struct input_dev *dev)
> +{

[ ... ]

> @@ -917,9 +1029,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 can concurrent execution of
workqueues cause a double unregistration of the input and sensors devices
due to a TOCTOU race condition?

The workqueues work_connect and unregister_work cache the device state under
a spinlock, drop the lock, and then act on the cached state. Both can run
concurrently on different CPUs and call steam_sensors_unregister() or
steam_input_unregister() simultaneously.

In both unregister functions, the pointers (steam->sensors and steam->input)
are read under rcu_read_lock(), but the lock is dropped immediately before
checking if the pointer is NULL. Because there is no atomic swap or mutex
protecting the RCU_INIT_POINTER assignment, both threads can read the same
non-NULL pointer, wait for RCU, and call input_unregister_device() on the
same device, causing a double-free and list corruption in the input core.

> @@ -967,6 +1076,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);
> +

[Severity: High]
This is a pre-existing issue, but if steam_register() fails for a wired
controller in steam_probe(), is the steam device freed while remaining
linked in the global steam_devices list?

In steam_register(), the steam object is added to the global steam_devices
list under a lock. If registration subsequently fails (e.g., due to an
out-of-memory error in steam_input_register()), steam_register() returns an
error code without removing the object from the list.

For a wired controller, steam_probe() catches this error and jumps to
err_hw_close, bypassing err_steam_unregister and the steam_unregister()
function that would normally remove it from the list. When steam_probe()
returns the error, devres frees the steam object.

Any subsequent iteration over steam_devices (e.g., changing lizard mode via
sysfs) will dereference the freed memory, causing a use-after-free and
potential kernel panic.

>  		hid_info(steam->hdev, "Steam Controller '%s' connected",
>  				steam->serial_no);
>

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