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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] If `hid_hw_raw_request` returns 0, the function prints uninitialized caller stack memory to the kernel debug log.

Pre-existing issues:
- [High] The driver enables hardware events via `hid_hw_start()` before initializing locks and works, leading to TOCTOU and uninitialized lock usage.
- [Medium] The newly added length validation `ret < data[1] + 2` is insufficient for `steam_get_serial`, leaving an uninitialized stack byte read via `strscpy`.
--

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

This commit adds length validation to steam_recv_report() to ensure that the
reported size of a Steam Controller message matches the bytes actually read.
It also addresses stripping the report ID from the buffer more explicitly.

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

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 298a2c17e1123..a80d54ed9784b 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -380,16 +387,30 @@ 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);
>  
>  	if (ret < 0)
>  		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
>  	else
> -		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
> -	return ret;
> +		hid_dbg(steam->hdev, "Received report %*ph\n", size, data);

[Severity: Medium]
If hid_hw_raw_request() returns 0, the if (ret > 0) block is bypassed,
leaving size unmodified.  Will this cause hid_dbg() to print uninitialized
stack memory from the data array?

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

[Severity: Medium]
This is a pre-existing issue, but is this length validation sufficient for
steam_get_serial()?

If a device sends a response where the returned length exactly equals
data[1] + 2, steam_recv_report() accepts the packet.

However, steam_get_serial() calls strscpy(..., reply + 3, reply[1]).  This
instructs strscpy() to read up to reply[1] bytes starting from index 3, which
accesses indices 3 through 3 + reply[1] - 1 (which is reply[1] + 2).

Since only indices 0 through data[1] + 1 were validated and initialized, does
this result in an uninitialized stack byte read?

> +	return size;
>  }

[Severity: High]
This is a pre-existing issue, but does steam_probe() enable hardware events
before initializing locks and workqueues?

In drivers/hid/hid-steam.c:steam_probe():

	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
	if (ret)
		return ret;

	steam->hdev = hdev;
	hid_set_drvdata(hdev, steam);
	spin_lock_init(&steam->lock);
	mutex_init(&steam->report_mutex);
	steam->quirks = id->driver_data;
	INIT_WORK(&steam->work_connect, steam_work_connect_cb);

If a connection event arrives immediately from the hardware after
hid_set_drvdata() but before spin_lock_init() or INIT_WORK(), could
steam_raw_event() attempt to acquire an uninitialized spinlock or schedule
an uninitialized work struct?

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