Fenrir fixes 2026 09 07 - #883
Open
danielinux wants to merge 14 commits into
Open
Conversation
The PKCS#11 and PSA store Remove paths invalidated the metadata and freed the bitmap slot but left the payload in flash, so removed keys stayed recoverable by a physical reader. Both Remove paths now call erase_object_payload() before invalidating the metadata; the existing sector read-modify-write preserves neighboring slots. Raw-flash deletion tests added to both unit suites.
The final image-size check re-derived the low-memory limit with a uint32 subtraction and no ordering check, so an inverted tolum wrapped into a near-2^32 limit and accepted any image. Compare the tolum/load_address ordering in 32-bit (low-memory) form when computing slot_max, and reuse that validated value in the check. Add a unit test for the inverted-tolum case (fails closed, both slots rejected).
…g PLL The MSIRDY wait read RCC_CFGR (bit 1 is SW status) instead of RCC_CR, so it never gated, and the MSI selection was cleared only in a local variable, never written back to RCC_CFGR. The PLL was disabled while still the SYSCLK source, dropping the system clock. Now: wait MSIRDY, commit the MSI selection, wait for SWS to confirm, then turn off PLL.
Sector 11 was at 0x818C000, inside sector 10's 256 KB range (0x8180000-0x81C0000), so hal_flash_erase mapped the upper part of sector 10 to sector 11 and erased the wrong sector. Sector 11 is 0x81C0000, contiguous after sector 10 and ending at FLASH_TOP.
sbi_wait_ipi_done() returned void, so a target hart that never completed its fence within the bounded wait was invisible and both the standard and legacy remote-fence interfaces reported success while the caller kept relying on a fence that may not have run. Return SBI_ERR_FAILED when any target does not complete within the bound, and propagate it through both SBI interfaces (standard RFENCE via err, legacy v0.1 remote fence via a0).
The CM4 firmware DTB is unverified (unsigned FAT partition) and this path never attaches an authenticated ramdisk, so a non-zero linux,initrd-start/end in it would direct the signed kernel to an unauthenticated initramfs in RAM. Zero both properties on the relocated DTB (fail closed on fixup error) and update the SECURITY comment to state the new behavior.
The HAL contract is absolute (0x08000000-based) addresses, as the erase path and every NVM caller use, but the double-word fast path added the flash base on top and targeted an address space past the flash. The read-modify-write path located its unit from the request base indexed by i/4, landing in the wrong 8-byte unit once a request started inside one. Both paths now program the 8-byte unit at (address + i), and the now-dead FLASHMEM_ADDRESS_SPACE define is gone. Add unit-stm32c0-write: runs the extracted hal_flash_write() against a host register/flash model with a second mapping standing in for the wrong address space (4/5 checks fail pre-fix, 5/5 pass post-fix).
hal_flash_write() stored both words of the 64-bit program unit relative to the caller's address, so a write starting inside a unit split the two stores across two units: the flash has no 32-bit program mode, so nothing is programmed and the second store faults on alignment. Align the destination down to the unit, take the bytes outside the requested span from the unit itself, and store through the aligned pointer, as hal/stm32h5.c does. The TrustZone claim is unchanged: it is page-granular and already covers every non-secure byte the aligned program touches. Extend unit-stm32l5-write with unaligned-start cases. The host data model cannot observe the program-unit split (pre-fix the bytes land identically), so these pin the fixed layout: bytes before the request preserved, nothing past it touched.
An unaligned starting address split the four word stores across two 16-byte program units, leaving partial quad-words that set FLASH_SR_WDW and hang the wait for completion. Align the destination down to the unit, read-modify-write the whole unit, and store through the aligned pointer. The unit test gains unaligned-start cases.
Contributor
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 bundles multiple embedded/bootloader fixes plus regression tests, focusing on flash write correctness (alignment and addressing), safer boot-time configuration handling, and secure deletion behavior.
Changes:
- Fix STM32 flash write implementations (U5/L5/C0) to program correct units/addresses and add/extend unit tests for unaligned writes.
- Harden boot flows: FSP low-memory limit fails closed on inverted
tolum, CM4 firmware DTB initrd pointers are zeroed, and RISC-V SBI remote-fence timeouts return an error. - Enforce keyvault anti-remanence by erasing payloads on PSA/PKCS11 object removal, with new unit tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-update-disk-fsp.c | Adds a regression test that inverted tolum fails closed (panics, no boot). |
| tools/unit-tests/unit-stm32u5-write.c | Expands STM32U5 flash-write regression tests to cover unaligned starts. |
| tools/unit-tests/unit-stm32l5-write.c | Expands STM32L5 flash-write regression tests to cover unaligned starts. |
| tools/unit-tests/unit-stm32c0-write.c | Adds new STM32C0 flash-write regression test harness (absolute addressing + RMW unit selection). |
| tools/unit-tests/unit-psa_store.c | Adds a test ensuring removal erases payload bytes from the raw keyvault flash. |
| tools/unit-tests/unit-pkcs11_store.c | Adds an equivalent anti-remanence test for the PKCS#11 store. |
| tools/unit-tests/Makefile | Registers/builds the new STM32C0 unit test and extraction header generation. |
| src/update_disk.c | Makes FSP tolum cap computation fail closed and reuses validated slot_max. |
| src/riscv_sbi.c | Propagates remote-fence timeouts as SBI errors. |
| src/psa_store.c | Erases keyvault payload before invalidating metadata on remove. |
| src/pkcs11_store.c | Erases keyvault payload before invalidating metadata on remove. |
| hal/stm32wb.c | Switches SYSCLK to MSI and waits for switch confirmation before disabling PLL. |
| hal/stm32u5.c | Reworks STM32U5 writes to RMW via aligned program-unit pointer. |
| hal/stm32l5.c | Reworks STM32L5 writes to RMW via aligned program-unit pointer. |
| hal/stm32f7.c | Fixes sector 11 base address for non-dual-bank configuration. |
| hal/stm32c0.c | Fixes STM32C0 fast-path absolute addressing and corrects RMW unit computation. |
| hal/cm4.c | Zeroes firmware DTB initrd pointers to avoid booting unauthenticated initramfs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
requested changes
Sep 7, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #883
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| /* A write of 20 bytes starting 4 bytes into an 8-byte unit: the | ||
| * first unit is only half requested, the rest of it keeps its stale | ||
| * value, and the request runs on through the following units. */ | ||
| START_TEST(test_write_20_unaligned4) |
There was a problem hiding this comment.
New STM32L5 unaligned-start tests pass against the unfixed HAL · Weak or missing assertions
Both new tests assert only final byte contents. The pre-fix hal_flash_write stored dst[i>>2]/dst[(i>>2)+1] relative to the caller address, landing the same bytes at the same offsets and rewriting the trailing unit bytes with their own values, so both tests pass unmodified against the unfixed code. The F-12877 alignment fix has no regression coverage.
Fix: Make the program-unit boundary observable, e.g. end the flash mapping at a page boundary so an unaligned unit store faults, and assert the aligned case succeeds.
| /* A write of 20 bytes starting 4 bytes into a 16-byte unit: the | ||
| * first unit is only partly requested, the rest of it keeps its stale | ||
| * value, and the request runs on through the following units. */ | ||
| START_TEST(test_write_20_unaligned4) |
There was a problem hiding this comment.
New STM32U5 unaligned-start tests pass against the unfixed HAL · Weak or missing assertions
Both new tests assert only final byte contents. The pre-fix hal_flash_write wrote dst[(i>>2)+j] relative to the caller address, producing identical bytes at offsets 4..23 and rewriting bytes 24..35 with their existing values, so both tests pass unmodified against the unfixed code. The F-12878 alignment fix has no regression coverage.
Fix: Make the 16-byte program-unit boundary observable, e.g. end the flash mapping at a page boundary so an unaligned unit store faults.
hal/stm32wb.c: dedicated RCC_CFGR_SWS_{MSI,MASK} macros for the
clock-switch confirmation wait; SW/SWS encodings verified identical
in RM0434 6.4.3 and the STM32WB55 SVD.
unit-stm32u5-write.c: START_TEST brace on the next line, matching
the file and the unit-suite convention.
…te tests The new unaligned tests passed against the pre-fix HAL (identical final bytes), so the alignment fix had no regression coverage. Mock hal_flash_wait_complete now diffs the flash per program window and asserts the changed bytes fit in one aligned unit; the 20-byte unaligned test goes red on the pre-fix HAL (l5: bytes 4-11 across two 8-byte units, u5: bytes 4-19 across two 16-byte units).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cfd44bc F-12878: STM32U5: program the 16-byte unit through an aligned pointer
ca06782 F-12877: STM32L5: program the 8-byte unit through an aligned pointer
8e86847 F-12870: STM32C0: program flash writes through absolute addresses
376f9e1 F-12920: zero firmware-DTB initrd pointers in hal_get_boot_dts
49c1fff F-12876: riscv_sbi: report remote-fence timeout as SBI error
9475169 F-12875: stm32f7 fix non-dual-bank sector 11 start address
22356e6 F-12874: stm32wb switch SYSCLK to MSI and confirm SWS before disabling PLL
394f160 F-12873: update_disk: FSP low-mem check reuses validated slot_max
194322e F-12921: erase keyvault payload on object removal