Fenrir fixes 2026 09 01 - #880
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR addresses multiple platform-specific regressions and hardening fixes across boot/update paths (PKCS#11 credential lifetime, flash write correctness, PCI bridge error rollback, Multiboot2 compliance), and adds targeted unit tests to prevent regressions.
Changes:
- Enforce full-size external-flash reads when loading images to RAM and add a unit test for short-read rejection.
- Add/extend platform fixes (STM32L4/WB flash write tails, RP2350 partial-page writes, NXP P1021 bad-block handling, Kontron TGL SPI lock, PCI bridge error rollback).
- Add comprehensive unit tests for each fixed regression and wire them into the unit-test build.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-update-ram-noramboot.c | Adds regression test ensuring short positive ext_flash_read aborts RAM-load boot. |
| tools/unit-tests/unit-stm32wb-write.c | New regression test for STM32WB double-word fast-path tail overread/overprogram. |
| tools/unit-tests/unit-stm32l4-write.c | New regression test for STM32L4 double-word fast-path tail overread/overprogram. |
| tools/unit-tests/unit-rp2350-flash-write.c | New regression test enforcing pico-sdk flash_range_program page contract and RMW behavior. |
| tools/unit-tests/unit-pkcs11-pin-zeroize.c | New unit test verifying PKCS#11 login PIN is wiped during crypto deinit. |
| tools/unit-tests/unit-pci.c | Adds test ensuring bridge programming error path restores COMMAND and disables windows. |
| tools/unit-tests/unit-p1021-read-badblock.c | New regression test for bad-block marker handling and buffer-overflow avoidance. |
| tools/unit-tests/unit-multiboot.c | Extends tests to require Multiboot2 end tag and validates a consumer-style tag walk. |
| tools/unit-tests/unit-mock-flash.c | Adds mock support for short positive reads to exercise update_ram short-read handling. |
| tools/unit-tests/unit-kontron-tgl-spi.c | New regression test for TGL SPI BIOS-region lock via BAR MMIO and readback verification. |
| tools/unit-tests/Makefile | Registers new unit tests and adds extraction rules for platform HAL functions. |
| src/update_ram.c | Rejects positive short reads for RAM image load; deinitializes PKCS#11 before handoff. |
| src/update_flash_hwswap.c | Deinitializes PKCS#11 before handoff. |
| src/update_disk.c | Deinitializes PKCS#11 before handoff. |
| src/pci.c | Restores original COMMAND on bridge-program failure and clears all windows on error. |
| src/multiboot.c | Appends Multiboot2 end tag to boot-info tag list and accounts for its size. |
| src/libwolfboot.c | Adds explicit PKCS#11 PIN wiping helper and wires it into pkcs11_crypto_deinit. |
| hal/stm32wb.c | Requires 8 bytes remaining before taking STM32WB double-word fast path. |
| hal/stm32l4.c | Requires 8 bytes remaining before taking STM32L4 double-word fast path. |
| hal/rp2350.c | Implements partial-page RMW to satisfy flash_range_program page alignment/size contract. |
| hal/nxp_p1021.c | Resets marker-page counter per erase block and fixes skip logic to avoid buffer overflow. |
| hal/kontron_vx3060_s2.c | Locks BIOS region through SPI BAR MMIO using FREG0, adds readback checks, and provides hal_flash_protect override. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #880
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
PR review (wolfSSL#880, Fenrir bot) flagged that the register offsets introduced by the F-12104 fix are wrong, and the Linux kernel's Intel PCH SPI driver (drivers/spi/spi-intel.c) confirms it: FDATA(n) = 0x10 + 4n -> 0x48 is FDATA14, a scratch data register FRACC = 0x50 -> not FREG0 FREG(n) = 0x54 + 4n -> FREG0 = 0x54, FREG1 = 0x58 FPR0-4 = 0x84-0x9C (BXT/CNL protection-range base) Two consequences. First, the BIOS range source: Intel flash region numbering is region 0 = flash descriptor, region 1 = BIOS, and the kernel driver's partition code reflects that ("start from the mandatory descriptor region", then iterate FREG(1..)). The original pre-F-12104 code read FREG1 (0x58) for the BIOS range and was right on that point; the F-12104 fix regressed it to FREG0 (0x50), which is the FRACC register. Restore FREG1. Second, FPR0: the original 0x48 came from the buggy PCI-config-space write path and is a FDATA scratch register in the SPIBAR map, so the readback check passed on a register that never programs protection. FPR0 is 0x84 (BXT/CNL PR base; JSL is not in the kernel's platform table but follows the same-generation layout). HSFSTS_CTL at 0x04 and the RPE (bit 15) / WPE (bit 31) / base / limit fields match the kernel's PR_ definitions and are unchanged. unit-kontron-tgl-spi.c: mirror the corrected offsets (FREG0 decoy at 0x54, FREG1 BIOS source at 0x58, FPR0 at 0x84) and assert FPR0 carries the FREG1 range; the test runs the real extracted tgl_lock_bios_region(), so it fails if the HAL offsets drift. Verification: unit-kontron-tgl-spi 4/4, full unit suite green, sim build green, cstyle clean on changed hunks.
PR review (wolfSSL#880, Fenrir bot) flagged that test_aligned_page_multiple_write_xip filled its 512-byte source at offset 3 * FLASH_PAGE_SIZE (768) of the 1024-byte flash model, so both the fixture and the HAL's XIP staging read 256 bytes past the modeled region; it only passed because mmap rounds the mapping up to a full page. Move the source to pages 2-3 (offset 2 * FLASH_PAGE_SIZE) with the destination at pages 0-1: non-overlapping, fully inside the model. Verification: unit-rp2350-flash-write 6/6, full unit suite green.
flash_range_program() requires a page-aligned address and a page-multiple length (pico-sdk ROM, invalid_params_if on both). The partition-state path without NVM_FLASH_WRITEONCE issues 1-byte (trailer) and 4-byte (magic) writes that violated the contract on every state transition. Keep the direct-program fast path for page-aligned page-multiple writes; otherwise read the page back from XIP, merge the write, and program the full page. The AND program keeps the trailer flag accumulation intact. Add unit-rp2350-flash-write: runs the extracted hal_flash_write against a mock flash_range_program() that enforces the ROM contract (4/6 checks fail pre-fix, 6/6 pass post-fix).
The double-word fast path of hal_flash_write() was selected on "len - i > 3" but always reads and programs two 32-bit words, so an aligned 4-7 byte tail read up to four bytes past the caller's buffer and programmed them into flash. Require at least eight remaining bytes before taking the fast path; shorter tails fall to the RMW branch, which rewrites the unit with the out-of-range bytes read back from flash. Same fix as the STM32G4 twin (F-11023). Add unit-stm32l4-write: runs the extracted hal_flash_write() against a host register/flash model with a canary after the source (3/5 checks fail pre-fix, 5/5 pass post-fix).
The double-word fast path was selected on 'len - i > 3' but always reads and programs two 32-bit words, so an aligned 4-7 byte tail read up to four bytes past the caller's buffer and programmed them into flash. Require at least eight remaining bytes; shorter tails fall to the RMW branch, which rewrites the unit with the out-of-range bytes read back from flash. Add unit-stm32wb-write: runs the extracted hal_flash_write() against a host register file with stale destination flash and a source canary (3/5 checks fail pre-fix, 5/5 pass post-fix).
ext_flash_read() initialized its bad-block page counter once per request, so the marker was inspected only on the first two pages read and a bad erase block later in the request was delivered as valid data. Restart the counter at the start of each erase block. The skip path also rewound the logical position to a block boundary without rewinding the output pointer, so a marker found after some pages had been delivered continued the read past the end of the caller's buffer. pos and data already agree (data = original + pos) after any delivered pages, so the skip only advances the source address. Add unit-p1021-read-badblock: runs the extracted ext_flash_read() against a mocked ELBC on a simulated NAND with three 16 KiB blocks (2/5 checks fail pre-fix, 5/5 pass post-fix).
tgl_lock_bios_region() wrote the protected range and the FLOCKDN value through PCI configuration space (offsets 0x48 and 0x04, the status/command dword) instead of the SPI controller's memory-mapped registers at the BAR0 base, and took the range from FREG1 (non-BIOS) instead of FREG0 (BIOS). The lock now writes FPR0 and BIOS/H SFSTS/CTL through mmio_write32(), verifies both by readback, and returns an error if the bits do not stick. The helper had no callers: no hal_flash_protect() override existed, so the weak no-op default ran before handoff and the BIOS region stayed writable. Add the override routing to tgl_lock_bios_region(). Including <hal.h> for the hook signature also exposes the hal_flash_write/hal_flash_erase stubs as mismatching the HAL contract; fix their address parameter to haladdr_t. Add unit-kontron-tgl-spi: runs the extracted tgl_lock_bios_region() and hal_flash_protect() against mocked PCI config space and an MMIO array at the BAR address (build fails pre-fix - hal_flash_protect undefined - 4/4 pass post-fix).
The non-RAMBOOT copy-to-RAM path only rejected negative ext_flash_read() results. Backends return the number of bytes read on success (filesystem.c forwards XFREAD's count), so a positive short read was accepted and boot continued with a truncated RAM image. Require the read to return exactly os_image.fw_size; any other result aborts the boot, matching the header-copy check already in the same file (ret != IMAGE_HEADER_SIZE). Note: the RAMBOOT image-load path (WOLFBOOT_USE_RAMBOOT) has the same `ret < 0` pattern on its img_size read; outside this finding's scope, left as-is. Test: unit-update-ram-noramboot gains a short-read case (mock ext_flash_read withholds 1 byte from the full-image copy only). Pre-fix the truncated image staged for boot (staged_ok == 1); post-fix the boot is aborted with "Error loading image ... (ret 5299)".
mb2_build_boot_info_header() emitted the requested tags and set
total_size without appending the Multiboot2 end tag (type 0,
size 8). A strict consumer walking the output section sees no
terminator inside total_size (or a zero-sized pseudo-terminator
only if the destination buffer happens to be zero-filled), so
the handoff is structurally invalid.
Reserve eight bytes for the end tag after the requested tags,
write {type 0, flags 0, size 8}, and include it in total_size.
The write is bounds-checked against the caller's max_size like
the other tag builders, and idx is already 8-byte aligned since
every tag size is a multiple of 8.
Tests: the existing layout assertions now expect the end tag
inside total_size (basic mem info: 24 -> 32, mem map with one
entry: 48 -> 56), and a new consumer-style test walks the
generated output section the way a strict Multiboot2 consumer
would: both requested tags found, well-formed end tag (type 0,
size 8), and the end tag is the last structure in total_size.
Pre-fix: 3 failures (total_size short by 8, walk finds no end
tag); post-fix: 36/36.
pci_program_bridge() used orig_cmd both as the saved COMMAND register value and as the accumulator for the decode bits enabled while programming. Error paths after a window was programmed (post-enum MMIO or IO alignment failures) restored that mutated value and left the programmed bridge windows active, so the bridge kept decoding address ranges the allocator rollback had just returned. Keep the two values separate: saved_cmd holds the original register content for the error path, new_cmd accumulates the decode bits and is written on success (seeded from saved_cmd, so the success path preserves the bits it did not manage, exactly as before). The error path now disables every bridge window (prefetch, MMIO, IO) before restoring saved_cmd. Test: test_program_bridge_oom_late_restore programs a prefetch window behind the bridge, then exhausts the MMIO pool so the post-enum MMIO alignment fails. Pre-fix the restored COMMAND was 0x0006 (original 0x0004 plus the MEM_SPACE bit for the discarded window) and the prefetch window stayed programmed (0x9000-0x900F); post-fix the original COMMAND is restored and all windows are disabled.
pkcs11_pin is a file-scope copy of the compile-time credential passed to C_Login() for the token holding the firmware-decryption key. pkcs11_crypto_deinit() runs on the pre-handoff path but only closed the session, leaving the credential in retained bootloader memory where a post-handoff attacker could recover it and authenticate to the token. Wipe the copy (volatile zeroize) after the final C_CloseSession(). No re-init path exists after deinit in the product flow (init is only called from the verification paths), so wiping inside the deinit is safe. Add unit-pkcs11-pin-zeroize: a full init/deinit cycle with a stubbed PKCS#11 backend that asserts the pin copy is all zero after deinit and the session was closed, plus a no-session deinit safety case. Verification: unit-pkcs11-pin-zeroize 1/2 pre-fix (pin byte 0 not wiped), 2/2 post-fix; full unit suite green; sim build green; kontron_vx3060_s2 CI build green.
pkcs11_crypto_deinit() - the only caller of pkcs11_pin_wipe() - was invoked from the update_flash path alone (src/update_flash.c:1715). On the RAMBOOT, hwswap and disk pre-handoff paths the PKCS#11 login credential stayed in retained bootloader memory after handoff. Add the same #ifdef ENCRYPT_PKCS11 deinit block after the WOLFHSM cleanup in src/update_ram.c, src/update_flash_hwswap.c and src/update_disk.c, in the same position as the existing update_flash call (before hal_flash_protect/hal_prepare_boot). update_ram.c and update_flash_hwswap.c did not include encrypt.h, where pkcs11_crypto_deinit() is declared - add the include (update_flash.c and update_disk.c already had it). The deinit is a no-op when crypto was never initialized, so the calls are safe on every build. Verification: full build with PKCS11 enabled (sim config + CFLAGS_EXTRA: ENCRYPT_PKCS11, EXT_ENCRYPTED, EXT_FLASH, WOLFCRYPT_SECURE_MODE, SECURE_PKCS11, WOLFPKCS11_USER_SETTINGS + mechanism/sizes/PIN) compiles all sources cleanly; the link stops on pre-existing externals (token library + secure-mode wolfssl objects that a real target's link config supplies) - a control build of the unpatched tree fails identically with the same undefined-symbol set. sim and kontron_vx3060_s2 builds green (PKCS11 disabled, hunks inactive). cstyle clean on the changed hunks.
pkcs11_pin is pre-populated from the compile-time credential (ENCRYPT_PKCS11_PIN), so the RAM copy exists from image load, not from a successful C_Login. pkcs11_crypto_deinit() only wiped it inside the encrypt_initialized branch, so on a target where init never completed the credential stayed in retained memory after the pre-handoff path ran. Move pkcs11_pin_wipe() out of the branch: the token interaction (C_CloseSession) stays conditional on an established session, the credential wipe is unconditional. deinit only runs on the terminal pre-handoff paths, so this cannot break the init retry in wolfBoot_initialize_encryption, which runs at decryption time, well before handoff. test_pkcs11_deinit_no_session now re-populates the pin and asserts every byte is zero after deinit without init (plus no C_CloseSession and repeat-call safety). Pre-fix it failed with "pkcs11_pin byte 0 not wiped" (1/2); post-fix 2/2. Verification: unit-pkcs11-pin-zeroize 2/2, full unit suite green, sim build green, cstyle clean on changed hunks.
The Kontron TGL SPI regression test kept the MMIO shadow in a uint8_t array and reached into it through uint32_t * casts. The accesses are all 32-bit at 4-aligned register offsets (0x04, 0x48, 0x50, 0x58), so the model is now a uint32_t array indexed by offset/4: no casts, no alignment or strict-aliasing doubt, and the redundant byte-clear before the 32-bit FREG0 store is gone. Verification: unit-kontron-tgl-spi 4/4, full unit suite green, cstyle clean.
PR review (wolfSSL#880, Fenrir bot) flagged that the register offsets introduced by the F-12104 fix are wrong, and the Linux kernel's Intel PCH SPI driver (drivers/spi/spi-intel.c) confirms it: FDATA(n) = 0x10 + 4n -> 0x48 is FDATA14, a scratch data register FRACC = 0x50 -> not FREG0 FREG(n) = 0x54 + 4n -> FREG0 = 0x54, FREG1 = 0x58 FPR0-4 = 0x84-0x9C (BXT/CNL protection-range base) Two consequences. First, the BIOS range source: Intel flash region numbering is region 0 = flash descriptor, region 1 = BIOS, and the kernel driver's partition code reflects that ("start from the mandatory descriptor region", then iterate FREG(1..)). The original pre-F-12104 code read FREG1 (0x58) for the BIOS range and was right on that point; the F-12104 fix regressed it to FREG0 (0x50), which is the FRACC register. Restore FREG1. Second, FPR0: the original 0x48 came from the buggy PCI-config-space write path and is a FDATA scratch register in the SPIBAR map, so the readback check passed on a register that never programs protection. FPR0 is 0x84 (BXT/CNL PR base; JSL is not in the kernel's platform table but follows the same-generation layout). HSFSTS_CTL at 0x04 and the RPE (bit 15) / WPE (bit 31) / base / limit fields match the kernel's PR_ definitions and are unchanged. unit-kontron-tgl-spi.c: mirror the corrected offsets (FREG0 decoy at 0x54, FREG1 BIOS source at 0x58, FPR0 at 0x84) and assert FPR0 carries the FREG1 range; the test runs the real extracted tgl_lock_bios_region(), so it fails if the HAL offsets drift. Verification: unit-kontron-tgl-spi 4/4, full unit suite green, sim build green, cstyle clean on changed hunks.
PR review (wolfSSL#880, Fenrir bot) flagged that test_aligned_page_multiple_write_xip filled its 512-byte source at offset 3 * FLASH_PAGE_SIZE (768) of the 1024-byte flash model, so both the fixture and the HAL's XIP staging read 256 bytes past the modeled region; it only passed because mmap rounds the mapping up to a full page. Move the source to pages 2-3 (offset 2 * FLASH_PAGE_SIZE) with the destination at pages 0-1: non-overlapping, fully inside the model. Verification: unit-rp2350-flash-write 6/6, full unit suite green.
95227f8 (fdt: rewrite device tree parser with capacity bound and full validation) changed wolfBoot_get_dts_size() to take a capacity argument but left the mock in unit-update-disk-fs.c with the old 1-arg signature, so the test no longer compiles (conflicting types). Update the mock; behavior is unchanged (always -1, no DTS in this test).
13b70de to
a504552
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #880
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
When the bad-block marker is found on the block's second page, the first page has already been copied to the caller's buffer and counted in pos; the skip advanced only the source address, so the bad block's page stayed in the output and the read returned len with the bad block's content mixed into the image. Record the output position at the start of each erase block and, on a bad block, rewind both pos and the data pointer to it before advancing the source address. This preserves the data = original + pos invariant (no out-of-bounds write) and discards the bad block's delivered pages. test_bad_marker_second_page_dropped now asserts the bad block is fully skipped (output starts at the next block's first page); it fails against the previous code.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #880
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
The F-12065 fix compared the int return of ext_flash_read() directly against the uint32_t os_image.fw_size. That int-vs-unsigned comparison triggers -Wsign-compare, which is a hard error under the default -Werror -Wextra for every EXT_FLASH+NO_XIP update_ram target (e.g. zynqmp) at -O0 and on host x86_64 gcc. Check the error range explicitly and cast for the size comparison, matching the established pattern in src/disk_fs.c (ret < 0 check followed by (uint32_t)ret != len). Semantics are unchanged: negative returns and positive short reads are both rejected. Verified: host gcc -Werror -Wextra -fsyntax-only warns on the old line and is clean on this one; unit-update-ram-noramboot 3/3.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #880
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
PR review (wolfSSL#880, Fenrir bot) flagged that test_noramboot_ext_flash_short_read_rejected set the shared mock globals mock_ext_flash_short_len/mock_ext_flash_short_bytes and only cleared them after the ck_assert. The suite runs CK_NOFORK, so a failing assertion longjmps out of the test and leaves every full-size ext_flash_read truncated for test_noramboot_highversion_rollback_denied, which then fails for an unrelated reason. Clear the mock globals immediately after wolfBoot_start() returns, before the assertion: the assert only checks wolfBoot_staged_ok, which is set during wolfBoot_start(), so the ordering changes nothing about what is tested and the mock state can no longer leak into the next test on a failure path. Verification: full unit suite in wolfboot-ci-sim (make -C tools/unit-tests; make run) exit 0; unit-update-ram-noramboot 3/3.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #880
Scan targets checked: wolfboot-bugs, wolfboot-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
634d679 F-12061: unit test: keep the XIP fast-path source in the flash model
5e80903 F-12104: kontron tgl: correct the SPIBAR FREG/FPR register offsets
65cab0a F-12104: unit test: model the TGL SPI MMIO as 32-bit registers
1c57f8c F-12114: pkcs11: wipe the PIN even when no session was established
b548341 F-12114: pkcs11: wipe the login PIN on all pre-handoff paths
6dabdad F-12114: pkcs11: wipe the login PIN in crypto deinit
0da4114 F-12066: pci: restore original COMMAND and clear windows on bridge error
7736d97 F-12060: multiboot2: terminate the boot info tag list with an end tag
c55795b F-12065: update_ram: reject short ext flash reads on the RAM load
4c447f4 F-12104: Kontron TGL: apply the SPI BIOS-region lock through the BAR
20c9031 F-12064: P1021: check bad-block markers per erase block
6792b5d F-12063: STM32WB: require a full double word in the fast write path
40d448b F-12062: STM32L4: require a full double word in the fast write path
5c22c27 F-12061: RP2350: RMW partial pages in hal_flash_write