|
| 1 | +# Final Review: `doc/tpm-gpio-vuln` branch — heads repo |
| 2 | + |
| 3 | +**Date**: 2026-07-21 |
| 4 | +**Branch**: `doc/tpm-gpio-vuln` (55 files, +2333/-65 lines) |
| 5 | +**Reviewer**: Oracle |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Overall Verdict: CHANGES REQUESTED (2 blocking findings) |
| 10 | + |
| 11 | +The branch is close to ready. Two staleness issues remain in the Approaches doc |
| 12 | +(appendix and testing sections that weren't updated when the script's register |
| 13 | +computation was corrected). All other criteria pass or have minor notes. |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## Blocking Findings |
| 18 | + |
| 19 | +### B1: Approaches doc Appendix A — stale SPT register computation (lines 663-666) |
| 20 | + |
| 21 | +**File**: `doc/TPM_GPIO_Reset_Approaches.md` |
| 22 | +**Lines**: 663-666 |
| 23 | + |
| 24 | +Current text: |
| 25 | +``` |
| 26 | +uses 4 DWORDS, starting at PAD_CFG_BASE. GPP_B13 is global pad 37, which is |
| 27 | +local index 13 within GROUP_B. DW0 = PAD_CFG_BASE + (37 * 16) = 0x400 + 0x250 = 0x650. |
| 28 | +Total address = 0xFD00_0650. |
| 29 | +``` |
| 30 | + |
| 31 | +**Problems**: |
| 32 | +1. "4 DWORDS" is wrong — SPT uses 2 DWORDS per `skylake/gpio_defs.h:12` |
| 33 | +2. DW0 offset should be `0x400 + 37 * 8 = 0x528`, not `0x400 + 37 * 16 = 0x650` |
| 34 | +3. Address should be `0xFDAF0528` (with port shift), not `0xFD00_0650` |
| 35 | + |
| 36 | +The script itself was corrected (NUM_PAD_CFG_REGS=2, offset 0x528) but this |
| 37 | +appendix section was missed. |
| 38 | + |
| 39 | +**Correction**: |
| 40 | +``` |
| 41 | +uses 2 DWORDS (8 bytes per `skylake/gpio_defs.h` line 12: |
| 42 | +`GPIO_NUM_PAD_CFG_REGS 2`). DW0 = PAD_CFG_BASE + (37 * 8) |
| 43 | += 0x400 + 0x128 = 0x528. Total address = 0xFDAF0528. |
| 44 | +``` |
| 45 | + |
| 46 | +### B2: Approaches doc SPT community testing section — stale address (line 553) |
| 47 | + |
| 48 | +**File**: `doc/TPM_GPIO_Reset_Approaches.md` |
| 49 | +**Line**: 553 |
| 50 | + |
| 51 | +Current text: |
| 52 | +``` |
| 53 | +- Register address: 0xFD00_0474 (with PCR_BASE = 0xFD000000). |
| 54 | +``` |
| 55 | + |
| 56 | +**Problem**: `0xFD00_0474` was computed as `0x400 + (13 * 4 * 4) = 0x400 + 0xD0 = 0x4D0` |
| 57 | +with port 0xAF applied incorrectly (missing 0xAF << 16). The correct address is |
| 58 | +`0xFDAF0528` per verified offset 0x528. Also the pad is global pad 37, not 13. |
| 59 | + |
| 60 | +**Correction**: |
| 61 | +``` |
| 62 | +- Register address: 0xFDAF0528 (offset 0x528 = 0x400 + 37*8, port 0xAF << 16, PCR_BASE 0xFD000000). |
| 63 | +``` |
| 64 | + |
| 65 | +--- |
| 66 | + |
| 67 | +## Non-Blocking Findings |
| 68 | + |
| 69 | +### N1: `blobs/README.md` files only partially updated |
| 70 | + |
| 71 | +**Files**: `blobs/m900/README.md`, `blobs/xx80/README.md` |
| 72 | +**Lines**: Pre-existing caveat sections |
| 73 | + |
| 74 | +The blobs/README.md files had "TPM reset" changed to "TPM GPIO reset" but the |
| 75 | +body text still says "extract FDE keys from a TPM." The board configs were |
| 76 | +updated to the new language ("TOTP/HOTP secret extractable, DUK with passphrase |
| 77 | +safe") but the blobs/README.md retained the old language. Recommend updating |
| 78 | +to match the board configs. |
| 79 | + |
| 80 | +### N2: Script header says ADL-P "TESTED" but mechanism "NOT confirmed working" |
| 81 | + |
| 82 | +**File**: `initrd/bin/tpm-gpio-reset-demo.sh` |
| 83 | +**Line**: 74-75 |
| 84 | + |
| 85 | +``` |
| 86 | +# This method IS IMPLEMENTED here. TESTED on NV4x ADL-P: write verified, |
| 87 | +# PCRs do not clear -- mechanism NOT confirmed working. |
| 88 | +``` |
| 89 | + |
| 90 | +Slightly confusing — "IS IMPLEMENTED" suggests it works, but the rest of the |
| 91 | +sentence says it doesn't. The script correctly reports failure when run with |
| 92 | +--execute. Consider rephrasing: |
| 93 | + |
| 94 | +``` |
| 95 | +# Register address verified (0xFD6E07D0). Write confirmed by readback. |
| 96 | +# However, PCRs do not clear after PLTRST# assertion on this PCH die. |
| 97 | +# Execute mode will report this honestly. |
| 98 | +``` |
| 99 | + |
| 100 | +### N3: Approaches doc line 553 uses "local pad index 13" for SPT |
| 101 | + |
| 102 | +**Line**: 553 |
| 103 | + |
| 104 | +Says `local pad index 13` but GPP_B13 on SPT is global pad 37. The local index |
| 105 | +within GPP_B group IS 13 (GPP_B0=24, GPP_B13=37, 37-24=13), but the terminology |
| 106 | +is ambiguous. The script correctly uses `FIRST_PAD=0` and `PLTRST_PAD=37`. |
| 107 | + |
| 108 | +### N4: Vulnerability doc summary still uses "reprogrammed to GPIO mode" language |
| 109 | + |
| 110 | +**File**: `doc/TPM_GPIO_Reset_Vulnerability.md` |
| 111 | +**Lines**: 6-8 |
| 112 | + |
| 113 | +The summary section describes the attack as "reprogrammed to GPIO mode" which |
| 114 | +is accurate for CNP-H but not for SPT/ADL (where the mechanism is writing to a |
| 115 | +PLTRST# assertion register at a fixed offset). The rest of the document correctly |
| 116 | +distinguishes mechanisms. Consider adding a brief qualification in the summary. |
| 117 | + |
| 118 | +### N5: per-platform feasibility says ADL-P Dasharo "Yes" |
| 119 | + |
| 120 | +**File**: `doc/TPM_GPIO_Reset_Vulnerability.md` |
| 121 | +**Line**: 227 |
| 122 | + |
| 123 | +The "Yes" for ADL/RPL Dasharo covers NV4x/NS50 (ADL-P) which was tested and |
| 124 | +confirmed NOT working (PCRs don't clear despite unlocked pad). Should be split |
| 125 | +into ADL-P "No" and ADL-S/RPL-S "Unknown." |
| 126 | + |
| 127 | +--- |
| 128 | + |
| 129 | +## Per-Criterion Assessment |
| 130 | + |
| 131 | +### 1. Factual accuracy: PASS (with B1-B2 fixes) |
| 132 | +- TCG spec sections: Sections 4, 12.2.3.2, 37 used correctly throughout ✅ |
| 133 | +- Section 27.2 parent reference removed ✅ |
| 134 | +- Section 27.2.6 retained for Unseal command (correct) ✅ |
| 135 | +- Register addresses: corrected to verified values in script and Approaches doc table ✅ |
| 136 | +- Platform status: verified against Dasharo/Purism forks ✅ |
| 137 | +- SPT NUM_PAD_CFG_REGS=2 verified against coreboot ✅ |
| 138 | +- CNP-H port 0x6B, first_pad=HDA_BCLK=107 verified ✅ |
| 139 | +- ADL-P tested: write verified, PCRs don't clear — honestly reported ✅ |
| 140 | + |
| 141 | +### 2. Per-platform consistency: PASS |
| 142 | +- All board configs use consistent single-line caveat format ✅ |
| 143 | +- BOARDS_AND_TESTERS table matches board config caveats ✅ |
| 144 | +- TPM doc per-platform table matches BOARDS_AND_TESTERS ✅ |
| 145 | +- Script detect_platform() statuses match documentation ✅ |
| 146 | + |
| 147 | +### 3. Attack mechanism clarity: PASS |
| 148 | +- "TPM GPIO reset" used consistently throughout (no bare "TPM reset" in new content) ✅ |
| 149 | +- Old "TPM reset attack" language removed from board configs ✅ |
| 150 | +- Board configs updated from verbose to concise `TPM_GPIO_RESET=VULNERABLE` format ✅ |
| 151 | +- Legacy language only in blobs/README.md (partial update) ⚠️ N1 |
| 152 | + |
| 153 | +### 4. TCG spec references: PASS |
| 154 | +- Sections 4, 12.2.3.2, 37 used for volatile/non-volatile guarantees ✅ |
| 155 | +- Section 27.2.6 used only for Unseal command (correct) ✅ |
| 156 | +- No PLTRST# attributed to TCG spec ✅ |
| 157 | +- Attacks doc aligns with Vulnerability doc references ✅ |
| 158 | + |
| 159 | +### 5. BusyBox compliance: PASS |
| 160 | +- No `od` usage (xxd used; comment correctly notes od is unavailable) ✅ |
| 161 | +- `set -eo pipefail` present ✅ |
| 162 | +- `TRACE_FUNC` used for function entry ✅ |
| 163 | +- No `echo >/dev/console` ✅ |
| 164 | +- `command -v` used directly (no custom has_cmd) ✅ |
| 165 | +- `xxd -p | tr -d '\n '` correctly handles BusyBox padding ✅ |
| 166 | + |
| 167 | +### 6. User-facing vs developer docs: PASS |
| 168 | +- TPM_GPIO_Reset_Vulnerability.md: technical, precise, cites source files ✅ |
| 169 | +- TPM_GPIO_Reset_Approaches.md: chronicle of development, register-level detail ✅ |
| 170 | +- BOARDS_AND_TESTERS.md: summary table with technical statuses ✅ |
| 171 | + |
| 172 | +### 7. TL;DR clarity: ⚠️ MINOR |
| 173 | +- **T480/480s wiki pages**: First paragraph is neutral description, doesn't immediately |
| 174 | + tell user they're affected. The "this platform" marker is in a table row further down. |
| 175 | + Consider adding bold: **This platform is affected.** |
| 176 | +- **T420/T430/x230**: Heading says "Not Affected", first line says "not affected" ✅ |
| 177 | + |
| 178 | +### 8. Mitigation guidance: PASS |
| 179 | +- "DUK with passphrase is not affected" present in all docs ✅ |
| 180 | +- Recovery shell GPG auth mentioned as mitigation ✅ |
| 181 | +- Per-board status table tells users which boards are safe ✅ |
| 182 | +- Fix noted as coreboot upstream responsibility ✅ |
| 183 | + |
| 184 | +--- |
| 185 | + |
| 186 | +## Follow-Up (deferrable) |
| 187 | + |
| 188 | +1. Test SPT/KBP on actual hardware (T480 KBL) to confirm mechanism works |
| 189 | +2. Test CNP-H on actual hardware (T480s) to confirm mechanism works |
| 190 | +3. Investigate why PLTRST# assertion doesn't reset TPM on ADL-P (PCH routing?) |
| 191 | +4. Update `blobs/README.md` to match board config language |
| 192 | +5. Review wiki T480/T480s first paragraphs for TL;DR clarity |
0 commit comments