Re: [PATCH v3 08/10] HID: steam: Improve logging and other cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a kernel stack information leak by logging uninitialized memory in the error path of `steam_get_serial`.

Pre-existing issues:
- [High] `steam_get_serial()` does not verify the length of the received report before reading from the uninitialized `reply` buffer, leaking kernel stack memory into the device serial number.
- [High] Infinite workqueue reschedule loop if the controller disconnects while force feedback is active.
--

--- Patch [8]: [PATCH v3 08/10] HID: steam: Improve logging and other cleanup ---
commit 077388dea9c00bbb5cfcdddffe77daaebbb2a614
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Improve logging and other cleanup

This commit adds more logging as appropriate, reindents an enum to match 
surrounding style, and replaces manual lock/unlock sequences with scope-based 
guard() constructs.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -481,22 +490,21 @@ static int steam_get_serial(struct steam_device *steam)
>  	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
>  	u8 reply[3 + STEAM_SERIAL_LEN + 1];
>  
> -	mutex_lock(&steam->report_mutex);
> +	guard(mutex)(&steam->report_mutex);
>  	ret = steam_send_report(steam, cmd, sizeof(cmd));
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  	ret = steam_recv_report(steam, reply, sizeof(reply));
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
>  	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {

[Severity: High]
This is a pre-existing issue, but can this validation block read out-of-bounds
uninitialized memory?

If a device returns a short report (e.g., 1 byte), steam_recv_report()
returns 1, leaving reply[1] and reply[2] uninitialized.

If these uninitialized bytes happen to match the validation criteria, could
strscpy() copy up to 21 bytes of uninitialized stack memory into
steam->serial_no, which is then exposed to userspace via the input subsystem?

> -		ret = -EIO;
> -		goto out;
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				(int)sizeof(reply), reply);

[Severity: High]
Could this leak uninitialized stack memory to dmesg?

If steam_recv_report() returns a short report (e.g., fewer bytes than
sizeof(reply)), the reply buffer won't be fully populated.

The newly added hid_err() unconditionally prints the entire sizeof(reply)
array. Does this expose uninitialized kernel stack data to the kernel ring
buffer?

> +		return -EIO;
>  	}
>  	reply[3 + STEAM_SERIAL_LEN] = 0;
>  	strscpy(steam->serial_no, reply + 3, reply[1]);
> -out:
> -	mutex_unlock(&steam->report_mutex);
>  	return ret;
>  }

[ ... ]

> @@ -1193,6 +1201,7 @@ static void steam_unregister(struct steam_device *steam)
>  {
>  	if (!steam->serial_no[0])
>  		return;
>  
>  	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
>  			steam->serial_no);
>  	steam_battery_unregister(steam);
>  	steam_sensors_unregister(steam);
>  	steam_input_unregister(steam);

[Severity: High]
This is a pre-existing issue, but is there a risk of an infinite loop if the
controller disconnects while force feedback is active?

In steam_coalesce_rumble_cb(), the work continuously reschedules itself based
on the non-zero state of rumble_left or rumble_right:

steam_coalesce_rumble_cb()
    if (steam->rumble_left || steam->rumble_right)
        schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);

When steam_unregister() is called on device disconnect, it unregisters the
input devices but doesn't appear to cancel the active force feedback works or
clear their persistent magnitude state.

Could this cause the workqueue to infinitely wake up and send USB reports to
the dongle until it's physically unplugged?

>  	mutex_lock(&steam_devices_lock);

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