[PATCH] HID: steam: reject short serial number reports

[PATCH] HID: steam: reject short serial number reports

[Yousef Alhouseen]

steam_recv_report() may return a short positive response and copies
only the bytes actually received. steam_get_serial() nevertheless reads
the full three-byte header and trusts its length without checking that
the serial payload was returned.

A malformed USB device can therefore make the driver read uninitialized
stack bytes. With a complete-looking short header, those bytes can also
be copied into steam->serial_no and printed.

Account for the stripped report ID in the return value and reject replies
that do not contain both the header and its declared payload.

Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-steam.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 197126d6e081..8c8bfb10e8b8 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -454,11 +454,20 @@ static int steam_get_serial(struct steam_device *steam)
 	ret = steam_recv_report(steam, reply, sizeof(reply));
 	if (ret < 0)
 		goto out;
+	/* hid_hw_raw_request() counts the stripped report ID byte. */
+	if (ret < 4) {
+		ret = -EIO;
+		goto out;
+	}
 	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
 	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
 		ret = -EIO;
 		goto out;
 	}
+	if (ret - 1 < 3 + reply[1]) {
+		ret = -EIO;
+		goto out;
+	}
 	reply[3 + STEAM_SERIAL_LEN] = 0;
 	strscpy(steam->serial_no, reply + 3, reply[1]);
 out:
-- 
2.54.0

Re: [PATCH] HID: steam: reject short serial number reports

[Vicki Pfau]

Hi Yousef,

On 6/27/26 5:41 PM, Yousef Alhouseen wrote:
> steam_recv_report() may return a short positive response and copies
> only the bytes actually received. steam_get_serial() nevertheless reads
> the full three-byte header and trusts its length without checking that
> the serial payload was returned.
> 
> A malformed USB device can therefore make the driver read uninitialized
> stack bytes. With a complete-looking short header, those bytes can also
> be copied into steam->serial_no and printed.
> 
> Account for the stripped report ID in the return value and reject replies
> that do not contain both the header and its declared payload.
> 
> Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
> Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
> Cc: stable@vger.kernel.org
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>  drivers/hid/hid-steam.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 197126d6e081..8c8bfb10e8b8 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -454,11 +454,20 @@ static int steam_get_serial(struct steam_device *steam)
>  	ret = steam_recv_report(steam, reply, sizeof(reply));
>  	if (ret < 0)
>  		goto out;
> +	/* hid_hw_raw_request() counts the stripped report ID byte. */
> +	if (ret < 4) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
>  	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
>  		ret = -EIO;
>  		goto out;
>  	}
> +	if (ret - 1 < 3 + reply[1]) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  	reply[3 + STEAM_SERIAL_LEN] = 0;
>  	strscpy(steam->serial_no, reply + 3, reply[1]);
>  out:

I already have locally a patch that fixes this as part of my pending Steam Controller 2 support. However, it chooses to fix it in a different way that would affect all uses of steam_recv_report instead of per-callsite (with only one callsite fixed). I am hoping to get this patchset submitted soon, once more widescale testing is done, but if you want in the meantime I can pull out that single fix and submit it separately; it's a bit more sprawling and involves adding a new function for combined send/recv.

Vicki

Re: [PATCH] HID: steam: reject short serial number reports

[sashiko-bot]

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

Pre-existing issues:
- [High] The serial number payload is systematically truncated and its buffer is improperly NUL-terminated. `strscpy(steam->serial_no, reply + 3, reply[1])` incorrectly uses the dynamic payload length (`reply[1]`) as the destination buffer size limit. Because `strscpy` guarantees NUL-termination by copying at most `size - 1` bytes, it unconditionally truncates the last byte of the serial number. Furthermore, `reply[3 + STEAM_SERIAL_LEN] = 0` hardcodes the NUL terminator at index 24 (the maximum possible payload boundary). If the device returns the maximum 22-byte payload, this overwrites the 22nd payload byte before copying. If the payload is shorter, it leaves uninitialized stack memory between the end of the payload and the NUL terminator, which would be exposed if `strscpy` used the correct buffer size.
- [High] Works and delayed works are cancelled before the underlying hardware is stopped, creating a race condition that leads to a Use-After-Free (UAF) on the `steam_device` structure.
--

commit 9b12928f5d4ac893e4176a52f291e99aaa7d63e9
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: steam: reject short serial number reports

This patch modifies the steam driver to correctly validate the length of
the serial number response. It accounts for the stripped report ID in the
return value and ensures that both the header and its declared payload are
fully received before parsing.

Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 197126d6e0810..8c8bfb10e8b82 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -454,11 +454,20 @@ static int steam_get_serial(struct steam_device *steam)
>  	ret = steam_recv_report(steam, reply, sizeof(reply));
>  	if (ret < 0)
>  		goto out;
> +	/* hid_hw_raw_request() counts the stripped report ID byte. */
> +	if (ret < 4) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
>  	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
>  		ret = -EIO;
>  		goto out;
>  	}
> +	if (ret - 1 < 3 + reply[1]) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  	reply[3 + STEAM_SERIAL_LEN] = 0;
>  	strscpy(steam->serial_no, reply + 3, reply[1]);

[Severity: High]
This is a pre-existing issue, but does this logic truncate the serial number?

Because strscpy() guarantees NUL-termination by copying at most size - 1
bytes, passing reply[1] as the destination size limit will unconditionally
truncate the last byte of the serial number payload.

Additionally, setting reply[3 + STEAM_SERIAL_LEN] = 0 hardcodes the NUL
terminator at the maximum boundary.

If the device returns the maximum 22-byte payload, this overwrites the 22nd
payload byte. If the payload is shorter, could this leave uninitialized
stack memory between the end of the payload and the NUL terminator?


I also noticed another potential problem in steam_remove() which is not
touched by this patch:

> static void steam_remove(struct hid_device *hdev)
> {
[ ... ]
> 	cancel_delayed_work_sync(&steam->mode_switch);
> 	cancel_work_sync(&steam->work_connect);
> 	cancel_work_sync(&steam->rumble_work);
> 	cancel_work_sync(&steam->unregister_work);
> 	steam->client_hdev = NULL;
> 	steam->client_opened = 0;
> 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> 		hid_info(hdev, "Steam wireless receiver disconnected");
> 	}
> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);

[Severity: High]
This isn't a bug introduced by this patch, but could this cancellation
sequence cause a use-after-free on the steam struct?

Since the driver explicitly cancels works before stopping the underlying
hardware with hid_hw_close() and hid_hw_stop(), an incoming interrupt could
trigger steam_raw_event() and reschedule the work.

If the work is rescheduled right before steam_remove() returns, could the
workqueue execute on the freed steam struct after the driver core cleans
it up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628004106.26920-1-alhouseenyousef@gmail.com?part=1