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

New issues:
- [Medium] `steam_get_attributes()` incorrectly parses the device attributes payload due to a wrong buffer offset and incorrect loop bounds, misinterpreting the report header as payload data.

Pre-existing issues:
- [High] Concurrent execution of device registration due to an unprotected race between `steam_probe` and `steam_work_connect_cb` leads to double allocation and a memory leak of input devices.
- [High] If `steam_register` fails, the device remains in the global `steam_devices` list, leading to a Use-After-Free when the memory is freed by devres.
--

commit 73cdd9e51b7293cba488d76f23eb3d6d1d4ca204
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..34653ad383ac0 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));
> +	if (ret < 0)
> +		return ret;
> +	ret = steam_recv_report(steam, reply, sizeof(reply));
> +	if (ret < 0)
> +		return ret;
> +	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
> +		return -EIO;
> +
> +	size = min(reply[1], sizeof(reply) - 2);
> +	for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
> +		attr = (struct steam_controller_attribute *)&reply[i];

[Severity: Medium]
Does this code incorrectly parse the device attributes payload?

The iteration starts at index 0, which is the message ID reply[0], instead
of index 2 where the payload actually begins.

Also, the size check relies on reply[1] but misses the +2 offset for the
header.

Will this misread the header as payload data and fail to extract attributes
like ATTRIB_CONNECTION_INTERVAL_IN_US?

> +		if (attr->tag == ATTRIB_CONNECTION_INTERVAL_IN_US) {
> +			steam->sensor_update_rate_us = get_unaligned_le32(&attr->value);
> +			hid_dbg(steam->hdev, "Sensor update rate: %uus\n",
> +				steam->sensor_update_rate_us);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> @@ -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();

[Severity: High]
This is a pre-existing issue, but does steam_input_register() have a race
condition that leaks memory?

Looking at steam_input_register() (and steam_sensors_register() as well), the
check for an existing input device is not protected by a lock:

	rcu_read_lock();
	input = rcu_dereference(steam->input);
	rcu_read_unlock();
	if (input) {
		dbg_hid("%s: already connected\n", __func__);
		return 0;
	}

	input = input_allocate_device();

If a wired Steam controller sends a wireless connection event
(ID_CONTROLLER_WIRELESS) immediately after its hardware is started in
steam_probe(), steam_work_connect_cb() is queued and runs concurrently with
the rest of steam_probe().

Can both threads enter steam_register() and steam_input_register(), see a
NULL pointer, and allocate/register the input device twice?

[ ... ]
> @@ -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);
> +

[Severity: High]
This is a pre-existing issue, but does the error handling in steam_probe()
leak the device in the global steam_devices list?

When steam_register() is called, it adds the device to the global list:

	mutex_lock(&steam_devices_lock);
	if (list_empty(&steam->list))
		list_add(&steam->list, &steam_devices);
	mutex_unlock(&steam_devices_lock);

If a subsequent step in steam_register() fails (like input_allocate_device()
failing due to memory pressure), it returns an error but leaves the device in
the list. 

Then in 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;
	}

The error handling path skips the err_steam_unregister label. The driver core
frees the devres-managed steam_device, leaving a dangling pointer in the
steam_devices list. 

Could subsequent iterations of this list (like when writing to the lizard_mode
module parameter) dereference the freed pointer and cause a Use-After-Free?

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

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