Re: [PATCH 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:

New issues:
- [High] The newly added short-read check in `steam_recv_report` contains multiple logic errors that render it ineffective, allowing short reads to bypass validation and leak uninitialized kernel stack memory, while also erroneously rejecting valid reports.

Pre-existing issues:
- [High] Self-deadlock in haptic rumble callbacks due to recursive acquisition of `steam->report_mutex`.
--

commit 27b5eb27907342525c15b39fe076131c6198f9b3
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.

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

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 593151709cf1d..e97431bc2828c 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -389,6 +389,12 @@ 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);
> +
> +	if (ret >= 2 && data[1] > ret + 2) {

[Severity: High]
Does this logic effectively catch short reads, or does it contain an algebraic
error? 

It appears data[1] > ret + 2 should be data[1] + 2 > ret. With the current
check, a device returning a short payload would bypass validation.

Additionally, since this check happens after ret is clamped to size earlier
in the function (ret = min(size, ret - 1);), won't this reject perfectly valid
large reports if the caller only requested a smaller buffer?

Also, the ret >= 2 gate allows 0 or 1 byte reads to return successfully. If
this occurs, callers like steam_get_serial() will read from an uninitialized
stack buffer. 

For instance, in steam_get_serial():

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

If steam_recv_report() returns 0 or 1, reply[1] is uninitialized,
potentially causing out-of-bounds reads or leaking kernel stack memory into
steam->serial_no.

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

[Severity: High]
This is a pre-existing issue, but does steam_haptic_rumble_cb() cause a
self-deadlock when triggering rumble?

This was introduced by the commit "HID: steam: Coalesce rumble packets"
earlier in the same series and persists through the end of the series.

In steam_haptic_rumble_cb():

drivers/hid/hid-steam.c:steam_haptic_rumble_cb() {
	guard(mutex)(&steam->report_mutex);
	steam_haptic_rumble(steam, 0, steam->rumble_left,
		steam->rumble_right, 2, 0);
}

And steam_haptic_rumble() unconditionally locks the same mutex:

drivers/hid/hid-steam.c:steam_haptic_rumble() {
	mutex_lock(&steam->report_mutex);
	ret = steam_send_report(steam, report, sizeof(report));
	mutex_unlock(&steam->report_mutex);
}

Since Linux mutexes are not recursive, won't this permanently hang the system
workqueue worker executing the rumble callbacks?

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