Re: [PATCH v3 04/10] 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 6 potential issue(s) to consider:

New issues:
- [Medium] The newly added `sensors` IMU configuration is silently destroyed when userspace closes the concurrent `input` evdev node or briefly toggles the hidraw client.
- [Medium] The IMU gyroscope Y-axis mapping for the Steam Controller has an inverted sign, breaking the right-handed coordinate system.

Pre-existing issues:
- [High] Use-After-Free when `steam_register()` fails during `steam_probe()`.
- [High] Concurrent execution of connection and unregistration work queues causes double-registration and permanent memory leaks of input devices.
- [High] Information Leak: `steam_send_report()` sends uninitialized kernel heap memory to the USB device.
- [High] HID I/O requests are issued after the hardware transport is torn down, violating hardware lifecycle rules.
--

--- Patch [4]: [PATCH v3 04/10] HID: steam: Add support for sensor events on the Steam Controller (2015) ---
commit 00dc87454b13954b455cab385838fde427082fff
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 add9f4f27fff9..6029c055a6cf3 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -468,6 +500,38 @@ static int steam_get_serial(struct steam_device *steam)
>  	return ret;
>  }
>  
> +static int steam_get_attributes(struct steam_device *steam)
> +{
> +	int ret = 0;
> +	u8 cmd[] = {ID_GET_ATTRIBUTES_VALUES, 0};
> +	u8 reply[64] = {};
> +	u8 size;
> +	int i;
> +	struct steam_controller_attribute *attr;
> +
> +	guard(mutex)(&steam->report_mutex);
> +	ret = steam_send_report(steam, cmd, sizeof(cmd));

[Severity: High]
This is a pre-existing issue, but does steam_send_report() send uninitialized
kernel heap memory to the USB device?

Looking at steam_send_report(), it allocates a buffer using
hid_alloc_report_buf(), which internally calls kmalloc() instead of
kzalloc(). It then copies the payload and sends max(size, 64) + 1 bytes via
hid_hw_raw_request().

When steam_get_attributes() calls this with a 2-byte payload, does this
result in up to 63 bytes of uninitialized kernel memory being transmitted to
the USB device?

> +	if (ret < 0)
> +		return ret;
[ ... ]
> @@ -626,6 +690,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]
Is the newly added sensors IMU configuration silently destroyed when userspace
closes the concurrent input evdev node?

If userspace has both the sensors node and the input node open, and then
closes the input node, steam_input_close() executes and evaluates
!steam->client_opened && lizard_mode. This calls
steam_set_lizard_mode(steam, true), which sends ID_LOAD_DEFAULT_SETTINGS.

Since ID_LOAD_DEFAULT_SETTINGS entirely resets the controller's hardware
configuration and turns off the IMU, does the sensors node remain open while
the physical IMU silently stops reporting data? Could this same state clobbering
occur if the raw HID client is quickly opened and closed?

> +
> +	return 0;
> +}
> +
> +static void steam_sensor_close(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;
> +
> +	guard(mutex)(&steam->report_mutex);
> +	steam_write_settings(steam, SETTING_IMU_MODE, 0, 0);

[Severity: High]
This is a pre-existing issue, but does calling steam_write_settings() here
violate hardware lifecycle rules during device removal?

In steam_remove(), the driver calls hid_hw_close(hdev) and
hid_hw_stop(hdev), which tears down the USB transport layer. Following this,
steam_unregister() invokes steam_sensors_unregister(), which calls
input_unregister_device().

If a userspace application holds the evdev node open, this triggers the
steam_sensor_close() callback. Since steam_write_settings() issues I/O
requests to a stopped HID device, can this access torn-down transport structures
and cause use-after-free conditions or kernel panics during device unplug?

> +}
[ ... ]
> @@ -839,9 +939,6 @@ static int steam_sensors_register(struct steam_device *steam)
>  	struct input_dev *sensors;
>  	int ret;
>  
> -	if (!(steam->quirks & STEAM_QUIRK_DECK))
> -		return 0;
> -
>  	rcu_read_lock();
>  	sensors = rcu_dereference(steam->sensors);
>  	rcu_read_unlock();
>  	if (sensors) {
>  		dbg_hid("%s: already connected\n", __func__);
>  		return 0;
>  	}
>  
>  	sensors = input_allocate_device();

[Severity: High]
This is a pre-existing issue, but can concurrent execution of the connection and
unregistration work queues cause double-registration and permanent memory leaks?

steam_work_connect_cb() and steam_work_unregister_cb() run on different
workqueues. If they execute concurrently, they can both read steam->sensors
as NULL here because the check only holds rcu_read_lock() and provides no
mutual exclusion.

Could both threads allocate a new device, call input_register_device(), and
publish their pointer via rcu_assign_pointer(), leaving the first device
permanently orphaned and leaked?

>  	if (!sensors)
[ ... ]
> @@ -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 does a failure in steam_register() lead to
a Use-After-Free during steam_probe()?

Early in steam_register(), the steam object is added to the global
steam_devices list. If steam_register() fails, it returns the error code
without removing steam from the list.

Back in steam_probe(), the error is caught, and the code jumps to
goto err_hw_close;, bypassing the err_steam_unregister: label.
steam_probe() exits with an error, and the driver core automatically frees
the steam object allocated with devm_kzalloc().

Does the freed object remain in the global steam_devices list, where any
subsequent read or write to the lizard_mode sysfs parameter would dereference
the freed pointer?

>  
[ ... ]
> @@ -1491,6 +1610,16 @@ static const struct steam_axis_mapping steam_controller_axis_mappings[] = {
>  	{ /* sentinel */ },
>  };
>  
> +static const struct steam_axis_mapping steam_controller_imu_mappings[] = {
> +	{ ABS_X,   1, 28 },
> +	{ ABS_Z,  -1, 30 },
> +	{ ABS_Y,   1, 32 },
> +	{ ABS_RX,  1, 34 },
> +	{ ABS_RZ,  1, 36 },

[Severity: Medium]
Does the ABS_RZ mapping have an inverted sign here, breaking the right-handed
coordinate system?

The Accel Y-axis is mapped to ABS_Z with a sign of -1, which matches the
Steam Deck mapping, indicating the physical IMU orientation is identical. On the
Steam Deck, the Gyro Y-axis is mapped to ABS_RZ with a sign of -1 to
maintain the correct coordinate system.

Since the Steam Controller mapping uses 1 for Gyro Y (ABS_RZ) instead of
-1, will this report mathematically inverted yaw rotation to applications?

> +	{ ABS_RY,  1, 38 },
> +	{ /* sentinel */ },
> +};
> +

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