Re: [PATCH v3 10/10] HID: steam: Retry send/recv reports if stale

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized kernel stack memory leaked to dmesg on invalid USB replies or short reads.
- [High] Missing cancellation of workqueues in the `steam_probe` error path leads to a Use-After-Free if probe fails.
--

--- Patch [10]: [PATCH v3 10/10] HID: steam: Retry send/recv reports if stale ---
commit 2a2fc2d99039c81756476abdde17aa7642c354be
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Retry send/recv reports if stale

Sometimes recv report will reply with a stale result from a previous send
report. Instead of failing out, we should retry them, as they generally
reply correctly after three tries, give or take.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a80d54ed9784b..3f5d4a88b8649 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> +static int steam_exchange_report(struct steam_device *steam, u8 *cmd, int csize,
> +		u8 *reply, int rsize)
> +{
> +	unsigned int retries = 5;
> +	int ret;
> +
> +	guard(mutex)(&steam->report_mutex);
> +	do {
> +		ret = steam_send_report(steam, cmd, csize);
> +		if (ret < 0)
> +			return ret;
> +		ret = steam_recv_report(steam, reply, rsize);
[ ... ]
> +		if (reply[0] == cmd[0] && reply[1] >= 1)
> +			break;
> +		if (retries > 0)
> +			continue;
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				rsize, reply);

[Severity: High]
Does this code leak uninitialized kernel stack memory to dmesg?

If the USB device returns a short reply, steam_recv_report() will only
copy the received bytes into the reply buffer. The remaining bytes in
the buffer remain uninitialized.

Since reply is allocated on the stack in callers like steam_get_serial(),
this hid_err() call will log the full rsize bytes, potentially leaking
sensitive kernel stack memory to dmesg on an invalid or short reply.

> +		return -EPROTO;
> +	} while (retries--);
> +
> +	return ret;
> +}
> +
>  static int steam_get_serial(struct steam_device *steam)
>  {
>  	/*
> @@ -511,15 +548,10 @@ 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];
>  
> -	guard(mutex)(&steam->report_mutex);
> -	ret = steam_send_report(steam, cmd, sizeof(cmd));
> +	ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
>  	if (ret < 0)
>  		return ret;
> -	ret = steam_recv_report(steam, reply, sizeof(reply));
> -	if (ret < 0)
> -		return ret;
> -	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
> -	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
> +	if (reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
>  		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
>  				(int)sizeof(reply), reply);

[Severity: High]
Similarly, can this leak uninitialized stack memory to dmesg?

If steam_exchange_report() successfully returns but the reply is invalid or
too short, this error path will log the full sizeof(reply), dumping any
uninitialized stack bytes from the reply buffer into dmesg.

>  		return -EIO;
>  	}

[Severity: High]
Does the probe error path cause a Use-After-Free?

Looking at steam_probe() in the same file, if steam_register() fails, the 
probe error path jumps to err_hw_stop:

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_stop;
		}
    ...
}

However, this bypasses the cancellation of steam->mode_switch which occurs
under err_steam_unregister:

drivers/hid/hid-steam.c:steam_probe() {
    ...
err_steam_unregister:
	spin_lock_irqsave(&steam->lock, flags);
	steam->client_opened = 0;
	spin_unlock_irqrestore(&steam->lock, flags);
	cancel_work_sync(&steam->work_connect);
	if (steam->connected)
		steam_unregister(steam);
	cancel_delayed_work_sync(&steam->mode_switch);
err_hw_stop:
	cancel_work_sync(&steam->rumble_work);
	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
	hid_hw_stop(hdev);
    ...
}

If a malicious USB device sends an input report that triggers 
steam_do_deck_input_event() before steam_register() fails, the mode_switch 
work is scheduled. When steam_probe() returns the error, the steam struct 
will be freed, but the un-canceled mode_switch delayed work could still 
execute, causing a UAF on the freed struct.

This teardown issue appears to have been introduced by earlier patches in
this series rearranging the teardown paths without properly covering all
workqueues on the error path.

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