[avgbob]

Summary

Add fail-closed bounds checks at the three sequencer call sites that read into fixed u8 hdr[16] stack buffers.

There is no current overflow in upstream: spi_hid_hdr_len() currently returns 9. This is defensive hardening so a future experimental/header-protocol change cannot accidentally turn a larger requested read into a kernel stack overwrite.

Changes

Before each spi_hid_seq_read(shid, hdr, hdr_len) using a fixed 16-byte header buffer:

• check hdr_len > sizeof(hdr);
• emit WARN_ON_ONCE;
• log the refused size;
• exit through the function's existing cleanup path;
• in the IRQ path, count the refusal consistently with other sequencer read failures.

No normal-path wire behavior changes while hdr_len <= 16.

Tests

Adds tests/header_read_bounds_test.py to make -C tests test.

The test pins:

• all three fixed-buffer guards;
• every numeric return in spi_hid_hdr_len() at <= 16;
• guard ordering before the corresponding SPI read.

This PR is independent of the functional touchscreen changes in #87/#88 and the tooling PRs #89/#90.

Project provenance

This defensive hardening came out of the same transport experimentation used to reproduce the Windows MSHW0231 behavior. Background and evidence for that work are here:

• Gate 2 Windows lifecycle capture
• Windows golden state machine
• Surface MSI mining report
• Windows source hashes

The current header length is safe; this PR simply makes future transport experiments fail closed if they ever request more bytes than the fixed sequencer header buffers can hold.

Review follow-up

The source test no longer guesses function boundaries using successor names. It brace-isolates each actual target function, requires the expected read call to exist, recognizes the descriptor poller's spi_hid_seq_read_reg(...) separately from the two spi_hid_seq_read(...) sites, and verifies the guard occurs before that concrete read. A negative in-memory mutation moves the guard after the read and must be rejected.

The driver guards/read lengths themselves were left unchanged.

[Syax89]

Thanks for the header-read bounds guards. Two independent review passes and direct reproduction found a deterministic failure in the new test, plus an ordering-coverage gap. I did not find a demonstrated introduced runtime defect in the guards themselves.

Reviewed head: 58356a790646e84e9e053ce81e23838836bdfe5d; base: 72b3492ba08fdee481b5dd1fa5781025264583f9.

Medium — new source test fails on the PR head

tests/header_read_bounds_test.py:28–37, invoked by tests/Makefile:218, searches forward for function delimiters that occur earlier in the file:

• spi_hid_stream_watchdog_work is at driver/spi-hid-core.c:2715, before spi_hid_poll_work:2824.
• static void spi_hid_create_device matches the _work definition at :1249, before spi_hid_seq_thread:3107. The main spi_hid_create_device at :1128 returns int, so the exact static void token does not identify it.

Both forward searches return -1. Reproduced output:

$ python3 tests/header_read_bounds_test.py
FAIL: cannot isolate static void spi_hid_poll_work
FAIL: cannot isolate static irqreturn_t spi_hid_seq_thread
# exit 1

make -C tests test exits 2 on this head; the same full suite passes on a separate base checkout. The descriptor-poller slice also spans unrelated functions, although that slice does not currently trigger the failure.

Suggested direction: isolate each actual function body rather than relying on guessed successor names, and rerun the full host gate. This is a local test-gate regression, not a claim that remote CI ran or that the device overflows at runtime.

Low — ordering assertion misses _read_reg

At tests/header_read_bounds_test.py:41–43, the test searches only for spi_hid_seq_read(shid, hdr, hdr_len), whereas the descriptor poller calls spi_hid_seq_read_reg(shid, reg, hdr, hdr_len) at driver/spi-hid-core.c:2200.

A missing call gives read == -1, and read < 0 or guard < read accepts it. After isolating the descriptor tuple to avoid the first failure, both the clean source and an in-memory mutant moving the existing guard after that real _read_reg call passed. Please recognize the actual read variant and fail if the expected read is absent. Keep a negative test that proves guard-after-read is rejected.

Guard behavior and concerns not accepted as bugs

• Current helper returns constant 9: base and head each attempt one 9-byte read on all three paths. The new >16 guards are inactive for current production lengths.
• Extracted actual helper/guard blocks compiled with -std=c11 -Wall -Wextra -Werror -fsanitize=undefined,address: boundary 16 accepted; artificial length 17 attempted a read on base and none on head, without sanitizer diagnostics. SPI was a recording stub, not a hardware transfer.
• The three out: epilogues release the sequencer mutex (:2295–2296, :2972–2974, :3247–3249): no demonstrated lock leak.
• Poller rescheduling/refusal/WARN-panic concerns are not reachable through oversized lengths at this pin. Do not change field-proven read lengths on those hypotheses.
• Neighboring read-failure handling already uses the same IRQ-storm increment; sizeof(hdr) is the 16-byte array size; > correctly allows 16; %u/%zu match the types.
• Older 9-vs-16 comments predate this change. Register selection, frame bytes, parser offsets and normal-path sequencing are unchanged.

Checks / limits

make -C tests real_frame_replay_test compiled successfully, as did the extracted guard probes; diff whitespace checks passed. No private frame corpus was replayed. The head aggregate stops at the new source-test failure, so later tests were not reached by that aggregate; the base full suite passed.

No kernel build, live IRQ/workqueue execution, real SPI, Surface hardware or cold boot/suspend qualification was performed. Remote checks were absent at review time. No source fixes were applied. These are nonblocking draft comments in GitHub review-state terms, but the demonstrated test failure should be resolved before relying on the host gate.

[avgbob]

Thanks — both test issues were valid. I changed the test, not the driver guards/read lengths.

Current head: 723b054d9513d7fa3edf9cfe86e51eb4292cc353.

header_read_bounds_test.py now brace-isolates each actual target function rather than guessing an end marker from another function name. For each of the three sites it requires the concrete expected read to exist and verifies the guard is before that read:

• descriptor poller: spi_hid_seq_read_reg(shid, reg, hdr, hdr_len);
• stream poller: spi_hid_seq_read(shid, hdr, hdr_len);
• threaded IRQ: spi_hid_seq_read(shid, hdr, hdr_len).

So a missing read can no longer produce a vacuous guard < -1 success.

I also kept an explicit negative mutation: the test moves the guard marker after the real read in each isolated function and requires the ordering predicate to reject it.

I separately audited the current source positions and all three guards precede their actual read call. The upstream PR CI is still action_required, so I’m not claiming the full host gate has rerun yet.