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

[sashiko-bot@kernel.org]

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