Re: [PATCH v2 09/10] HID: steam: Reject short reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Out-of-bounds stack memory read in `steam_recv_report` when logging debug information.
- [Medium] Uninitialized kernel stack memory read on the error path for short reports.
--

--- Patch [9]: [PATCH v2 09/10] HID: steam: Reject short reads ---
commit ea3743d28300ad972f70af0939882012d7dbf763
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Reject short reads

Steam Controller FEATURE reports encode the size of the message in the
message itself. Previously we were trusting that the size reported matched
the size we actually read, leading to a potential issue with short reads.
Instead, we should actually verify the length of the read.

Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 1f2cf15d40831..49f6733d89359 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -380,8 +387,10 @@ static int steam_recv_report(struct steam_device *steam,
>  			buf, hid_report_len(r) + 1,
>  			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>  	if (ret > 0) {
> -		ret = min(size, ret - 1);
> -		memcpy(data, buf + 1, ret);
> +		/* Remove the report ID from the return buffer */
> +		ret--;
> +		size = min(size, ret);
> +		memcpy(data, buf + 1, size);
>  	}
>  	kfree(buf);
>  
> @@ -389,7 +398,15 @@ static int steam_recv_report(struct steam_device *steam,
>  		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
>  	else
>  		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);

[Severity: Medium]
Since the clamping of ret to size was removed above, ret can now be larger
than the caller-provided buffer size.

If a caller provides a small stack buffer (e.g. 25 bytes in
steam_get_serial()) and the device returns a larger report, will hid_dbg()
read out-of-bounds stack memory from the data buffer when dynamic debug is
enabled?

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

[Severity: Medium]
If the device returns a short read of 0 or 1 bytes, memcpy() copies less
than 2 bytes to data, leaving data[1] uninitialized.

Since hid_err() arguments are unconditionally evaluated before the function
is called, could this read uninitialized stack memory from data[1] if
ret < 2 evaluates to true?

> +		return -EPROTO;
> +	}
> +	return size;
>  }

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