Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Log spam in expected retry path due to unconditional `hid_err` in `steam_recv_report`.
- [Medium] Out-of-bounds stack read in `hid_dbg` when the device report exceeds the caller's buffer.
- [Medium] `steam_recv_report` reads uninitialized stack memory on short hardware reads.
--

--- Patch [10]: [PATCH v2 10/10] HID: steam: Retry send/recv reports if stale ---

commit 181ac85173536f44d1a324b3bc0042d43fa8238e
Author: Vicki Pfau [off-list ref]

HID: steam: Retry send/recv reports if stale

Sometimes recv report will reply with a stale result from a previous send
report. Instead of failing out, we should retry them, as they generally
reply correctly after three tries, give or take.

[Severity: Medium]
Does this call cause an out-of-bounds stack read in steam_recv_report()?

When a device sends a padded report that is larger than the caller's buffer
(like the 36-byte reply buffer in steam_get_serial()), steam_recv_report()
copies the correct size but logs using the unbounded ret length:

steam_recv_report() {
    ...
    size = min(size, ret);
    memcpy(data, buf + 1, size);
    ...
    hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
    ...
}

Since ret can be larger than size, won't hid_dbg() read past the end of the
reply buffer on the stack?

[Severity: Medium]
Can this also result in reading uninitialized stack memory on short reads?

If the hardware sends a short report where hid_hw_raw_request() returns
0 or 1, memcpy() copies 0 bytes, leaving the caller's reply buffer
uninitialized.

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

When ret < 2, data[1] has not been populated. Won't this read and log an
uninitialized byte from the stack?

+		/*
+		 * Sometimes this can fail on the first few tries on the Steam
+		 * Controller (2015). It appears to be a firmware bug, and Steam
+		 * itself just retries, so we should also retry a few times to
+		 * see if we get it.
+		 */
+		if (ret == -EPROTO)

[Severity: Medium]
Will this retry logic cause log spam during normal operation?

Because steam_recv_report() unconditionally calls hid_err() before returning
-EPROTO:

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

Won't every expected retry here flood the kernel log with error messages?

+			continue;
+		if (ret < 0) {
+			hid_err(steam->hdev, "%s: error reading reply (%*ph)\n",
+					__func__, csize, cmd);
+			return ret;
+		}
+		if (reply[0] == cmd[0] && reply[1] >= 1)
+			break;
+		if (retries > 0)
+			continue;
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				rsize, reply);
+		return -EPROTO;
+	} while (retries--);
+
+	return ret;
+}

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