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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Log spam in expected retry path due to unconditional `hid_err` in `steam_recv_report`.
- [Medium] Out-of-bounds stack read in `hid_dbg` when the device report exceeds the caller's buffer.
- [Medium] `steam_recv_report` reads uninitialized stack memory on short hardware reads.
--

--- Patch [10]: [PATCH v2 10/10] HID: steam: Retry send/recv reports if stale ---
commit 181ac85173536f44d1a324b3bc0042d43fa8238e
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 49f6733d89359..ba2dd40a79763 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -497,6 +497,43 @@ static int steam_write_settings(struct steam_device *steam,
>  	return steam_recv_report(steam, cmd, 2 + cmd[1]);
>  }
>  
> +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);

[Severity: Medium]
Does this call cause an out-of-bounds stack read in steam_recv_report()?

When a device sends a padded report that is larger than the caller's buffer
(like the 36-byte reply buffer in steam_get_serial()), steam_recv_report()
copies the correct size but logs using the unbounded ret length:

steam_recv_report() {
    ...
    size = min(size, ret);
    memcpy(data, buf + 1, size);
    ...
    hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
    ...
}

Since ret can be larger than size, won't hid_dbg() read past the end of the
reply buffer on the stack?

[Severity: Medium]
Can this also result in reading uninitialized stack memory on short reads?

If the hardware sends a short report where hid_hw_raw_request() returns
0 or 1, memcpy() copies 0 bytes, leaving the caller's reply buffer
uninitialized.

steam_recv_report() {
    ...
    if (ret < 2 || ret < data[1] + 2) {
        hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
                __func__, data[1] + 2, ret);
        return -EPROTO;
    }
}

When ret < 2, data[1] has not been populated. Won't this read and log an
uninitialized byte from the stack?

> +		/*
> +		 * Sometimes this can fail on the first few tries on the Steam
> +		 * Controller (2015). It appears to be a firmware bug, and Steam
> +		 * itself just retries, so we should also retry a few times to
> +		 * see if we get it.
> +		 */
> +		if (ret == -EPROTO)

[Severity: Medium]
Will this retry logic cause log spam during normal operation?

Because steam_recv_report() unconditionally calls hid_err() before returning
-EPROTO:

steam_recv_report() {
    ...
    if (ret < 2 || ret < data[1] + 2) {
        hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
                __func__, data[1] + 2, ret);
        return -EPROTO;
    }
}

Won't every expected retry here flood the kernel log with error messages?

> +			continue;
> +		if (ret < 0) {
> +			hid_err(steam->hdev, "%s: error reading reply (%*ph)\n",
> +					__func__, csize, cmd);
> +			return ret;
> +		}
> +		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);
> +		return -EPROTO;
> +	} while (retries--);
> +
> +	return ret;
> +}

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