spi_device: CSb-deassert race desyncs upload FIFO pointer from DPSRAM data — SW pops stale/phantom flash command during bootstrap
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
## Description
At the committing SCK edge of an upload command (8th opcode bit for CMDFIFO,
last address bit for ADDRFIFO), the push is committed by **two independent
buffered copies of the same CSb pad**:
- the **write-pointer increment** (`w_wptr_inc = wvalid & wready` inside
`prim_fifo_async_sram_adapter`), whose `wvalid` only dies when the **reset
copy** (`rst_csb_buf` → `rst_spi_in_n`) asserts — and `w_wptr_q` is
deliberately reset by `sys_rst_ni` only (`spid_upload.sv:510-527`, comment
"To maintain the pointer ...");
- the **DPSRAM write** (`mem_b_l2m` request), which is combinationally cut by
the **gate copy** (`sck_csb`, `spi_device.sv:932`).
The CSb pad is split into 4 independently-buffered copies
(`spi_device.sv:395-403`). In real silicon these copies have different delays
(the reset copy carries an extra `prim_clock_mux2` scan-mux stage plus a
high-fanout async reset tree; the gate copy is a single `prim_buf`). If the
gate copy rises **before** the committing SCK edge and the reset copy rises
**after** it, the pointer advances and the SW interrupt fires — but the SRAM
write is aborted. SW then pops a slot that was never written: a **stale or
phantom command** is delivered to software while the command the host actually
sent is silently lost, both in the same transaction.
This is invisible in zero-delay RTL simulation: the race only exists between
buffered copies of one pad, so standard DV structurally cannot catch it. It is
a silicon timing race (fault-injection class), not an RTL logic bug.
## Current behaviour
- `spid_upload.sv:510-527` — upload FIFO write pointers use `sys_rst_ni`, so
they survive CSb deassertion while the rest of the SCK domain resets.
- `spi_device.sv:880-939` — SCK-domain DPSRAM mux; `mem_b` request gated by
`!sck_csb` at `spi_device.sv:932`.
- `spid_csb_sync.sv` — CSb-as-clock tap through which `sck_cmdfifo_set`
reaches SW as the `upload_cmdfifo_not_empty` interrupt.
## The skew window (exact)
Let `D_GATE` = pad→`sck_csb` delay and `D_RST` = pad→`rst_csb_buf` delay, and
`T_commit` = the committing SCK rising edge. The desync occurs when the CSb
rise lands in:
```
( T_commit − D_RST , T_commit − D_GATE ) requires D_GATE < D_RST
```
- **Window width = `D_RST − D_GATE` exactly.** Measured by sweep across
skews 100–1200 ps: desync count grows linearly with skew, window width
matches to the sweep step, no saturation in the tested range.
- The window is **sub-cycle** (0 clock cycles; at 25 MHz SCK, a 700 ps window
is 1.75% of one SCK period) and sits inside the final SCK bit period — a
tSU-class violation region.
- **Direction matters**: only the gate-copy-faster direction desyncs. Negative
skew (reset copy faster) is benign: SRAM written, pointer not advanced
(measured 0 desyncs).
- The direction requirement is physically plausible: the reset copy has the
extra scan-mux stage and the high-fanout async reset tree; the gate copy is
a single buffer.
## Reproduction
Focused Verilator (`--timing`, 1 ps timescale) TB built from **production RTL
only** — `spi_s2p`, `spid_upload` (with the real `prim_fifo_async_sram_adapter`
×2, `prim_sram_arbiter`, `spid_fifo2sram_adapter`, `spid_csb_sync`),
`spid_dpram`, sys-side `prim_sram_arbiter`, `prim_clock_mux2`. The only
TB-authored logic replicates the `spi_device.sv` top-level glue the race lives
in: the 4-way CSb pad split with a parameterizable per-copy delay, and the
`!sck_csb`-gated SCK-domain SRAM mux. Host model: 25 MHz mode-0; SW model
drains CMDFIFO/ADDRFIFO in the sys domain exactly as the CSR interface does.
Racy opcodes are distinct per transaction so stale data can never alias the
sent value.
| condition | result |
|---|---|
| equal copy delays (skew = 0), offsets −1500..1500 ps | **0 desync** (clean commit/abort threshold) — race invisible in zero-delay RTL |
| `D_GATE`=300 ps, `D_RST`=1000 ps (skew 700 ps) | **70/70 in-window offsets desync**, offsets [−990, −300] ps == predicted `(−D_RST, −D_GATE)` |
| skew scan 100..1200 ps | desync count == skew linearly; window width == skew, exact |
| negative skew (reset copy faster) | **0 desync** — benign direction |
| ADDRFIFO committing edge (last of 3 address bytes) | identical race, same window, 36 desyncs |
Canonical run (skew 700 ps, offset −650 ps), memory peek + SW pop:
```
[before racy] cmdfifo w_wptr_q=14 slot word[78]=0x8040200d5 decoded16=0x00d5
[after racy 0xED @ off=-650ps] SW pops=1 popped16=0x00d5 <- stale old command
w_wptr_q now=15 (pointer advanced) slot word=0x8040200d5 (UNCHANGED -> write aborted)
cmdfifo intr fired for racy txn: YES <- SW told a command arrived
VERDICT: DESYNC (SW popped STALE entry; sent opcode never written to DPSRAM)
```
### Repro artifacts
The TB (single 593-line file, `tb_f91_csb_race.sv`), the build script
(Verilator against an OpenTitan checkout, no DV harness needed), the sweep CSV
(494 desync rows, deterministic across re-runs), and the sim log are ready to
publish as a gist, same as the #31187 harness:
**https://gist.github.com/john-kearney/273600250c4ed7fc363df3b9d7aa3a99-BEFORE-POSTING: create gist with `pocs/F91-SPI/confirm/{tb_f91_csb_race.sv, build.sh, f91_confirm_sim.log, f91_sweep.csv}` and insert URL here.**
## Where this matters
- **CMDFIFO**: the `upload_cmdfifo_not_empty` interrupt fires and SW pop
returns the **previous** 16-bit word at that slot — a stale opcode with
stale WEL/BUSY status bits (or `0x0000` for never-written slots). The sent
command is silently lost **and** a phantom/stale command is delivered in its
place, both at once.
- **ADDRFIFO**: same shape at the address committing edge — a stale/phantom
32-bit address is delivered while the sent address is lost.
- Upload is the FlashMode SW-processing channel for commands during
bootstrap/provisioning, so a phantom flash command (e.g., replay of a prior
opcode with stale WEL/BUSY bits) is delivered to bootstrap SW.
- Beyond adversarial timing, this is a plain reliability hazard: EMI or
ground bounce on CSb near the final edge can silently corrupt the command
stream on any physical part where the gate copy out-runs the reset copy.
Pointer/data desync in a FIFO is a classic silicon killer.
## Security impact — honest scoping
Grading this conservationally: exploitation requires sub-nanosecond CSb
placement control (fault-injection-class skill, though pure protocol timing —
no voltage or clock glitching; the host owns both pads and can sweep across
repeated commands). More importantly, **the attacker in the relevant phase is
the SPI host itself**: upload runs pre-validation mainly during
bootstrap/provisioning, and a host that can win the race could in most cases
have simply sent the stale command it is replaying. The direct gain is
therefore limited to **protocol-integrity corruption** — phantom command
delivery, SW/HW state desync, and audit/provisioning mislogging — not a
confidentiality or code-execution primitive. We'd categorize the security
severity as MEDIUM; the hardware-correctness severity is HIGH, and current DV
cannot see the class at all.
## Suggested fix
Make the pointer increment and the DPSRAM write abort together. Qualify the
write-side handshake (`w_sram_gnt` into `prim_fifo_async_sram_adapter`, hence
`w_wptr_inc`) with the **same buffered `!sck_csb` copy** that gates `mem_b_l2m`
— either route `sck_csb` into `spid_upload`, or gate the grant in the
`spi_device.sv` SRAM mux where `!sck_csb` already exists:
Conceptually, wherever the `!sck_csb` gate already cuts the SRAM request:
```systemverilog
// spi_device.sv, SCK-domain DPSRAM mux (near :932) — existing:
// mem_b_req gated by !sck_csb
// add: cut the upload FIFOs' write-side grant with the SAME copy:
// cmdfifo_wgrant = wgrant & ~sck_csb; // feeds w_sram_gnt -> w_wptr_inc
```
Exact port/placement is left to maintainers — the invariant is: **the grant
that advances `w_wptr_q` must be killed by the identical buffered copy that
kills the SRAM request**, so the two halves of the commit can never disagree.
Two complementary hardenings:
1. A timing constraint/SVA that `D_RST >= D_GATE` for these copies, enforced
with delay-matching buffers if synthesis cannot guarantee it.
2. A gate-level (SDF-annotated) DV test sweeping the CSb rise across the final
SCK edge of upload commands. Zero-delay RTL simulation of this block
structurally cannot catch this class of bug, so the current DV suite
provides no coverage here.
## Dedup
No existing issue found for CSb-deassert pointer/SRAM desync or skew between
CSb buffered copies (searched csb reset/race/recovery, upload FIFO; related
but different: #12096 DV bring-up, #15721 dummy-CSB interrupt).
Contributor guide
Research direction
Start with spi_device.sv:880-939 and spid_upload.sv:510-527, then trace w_sram_gnt and the !sck_csb-gated mem_b request through the upload FIFO path. Reproduce the race with tb_f91_csb_race.sv and its Verilator build script, then check CMDFIFO and ADDRFIFO behavior across CSb skew. Done means the pointer cannot advance when the corresponding DPSRAM write is aborted.
Written by the indexing model from the issue text.
Assessment
- Domain
- embedded-iot, security, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100