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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Insufficient bounds checking allows uninitialized kernel stack memory to be parsed, exposed to userspace, and logged to dmesg.
--

commit 74668d65404cd2e20c8afd5929ec0da105fd2ec0
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Retry send/recv reports if stale

This commit introduces a retry mechanism for sending and receiving reports to
handle stale replies. It adds steam_exchange_report() to wrap the send and
receive logic with retries, avoiding failures from firmware bugs.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index e97431bc2828c..8d036b3412539 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 (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)

[Severity: High]
This is a pre-existing issue, but does this code evaluate uninitialized
stack memory if steam_recv_report() returns a short read?

steam_recv_report() has a mathematically flawed bounds check that can fail
to reject truncated packets or bypass the check entirely for 0-1 byte replies:

	if (ret >= 2 && data[1] > ret + 2) {
		...
		return -EPROTO;
	}

Because reply is uninitialized on the stack in callers like steam_get_serial(),
can a short read from a malfunctioning or malicious USB device cause us to
read uninitialized memory here?

Could it also potentially return success if the stack happens to match the
expected values?

> +			break;
> +		if (retries > 0)
> +			continue;
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				rsize, reply);

[Severity: High]
This is a pre-existing issue, but if steam_recv_report() returned a short read,
does this log uninitialized stack memory to dmesg since it dumps the full
rsize bytes of the reply buffer?

> +		return -EPROTO;
> +	} while (retries--);
> +
> +	return ret;
> +}
> +
>  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) {

[Severity: High]
This is a pre-existing issue, but since the length from reply[1] is not
validated against the actual bytes received, does the later call to strscpy()
leak up to 31 bytes of uninitialized kernel stack memory to userspace?

drivers/hid/hid-steam.c:steam_get_serial() {
	...
	reply[3 + STEAM_SERIAL_LEN] = 0;
	strscpy(steam->serial_no, reply + 3, reply[1]);
	return ret;
}

Can a remote or malfunctioning USB device returning short or empty HID payloads
trigger this information leak?

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