Fenrir fixes 2026 08 26 - #874
Open
danielinux wants to merge 10 commits into
Open
Conversation
pci_enum_do() set mem_limit/mem_pf_limit to base + length - 1, i.e. the last usable byte, while every consumer of the limits compares them as exclusive ends: pci_enum_next_aligned32() rejects a start >= limit, the BAR end check rejects a region whose end is > limit, and pci_align_check_up() rejects an aligned start >= limit. The IO pool limit (PCI_IO32_LIMIT) is already the exclusive 16-bit ceiling. With the inclusive-style init the pool effectively lost its last byte and a BAR that exactly fills a configured pool (e.g. a 128 MB non-prefetchable MMIO BAR on the default 128 MB pool) was skipped instead of mapped. Initialize the MMIO and prefetch limits as base + length and reject a pool whose end would wrap the 32-bit address space (custom PCI_MMIO32_BASE/LENGTH definitions), computed in 64 bits so the check holds on every host word size. unit-pci gains test_enum_do_pool_fill, which drives the real pci_enum_do() over a 128 MB BAR that exactly fills the default pool; pre-fix the BAR was restored to its original value (never mapped). Verification: - Built: gcc (host) unit-pci with -DWOLFBOOT_USE_PCI: clean. - Tested: unit-pci 29/29; pre-fix the new test failed with the BAR restored to 0 instead of programmed at 0x80000000. - Pitfalls: single- and multi-BAR allocations under a partially filled pool are unaffected (region end <= base + length still fits); the overflow guard only rejects pools that cannot be represented in 32-bit address space. - Style: cstyle-check.sh flag output on src/pci.c identical to the pre-change file; the new test adds one C99-decl line in the suite registration, the class the existing registrations already trip. - Message: F-11031: prefix, no co-author trailers.
curr_bus_number is a uint8_t advanced once per bridge level. At 0xFF the increment wrapped to 0: pci_program_bridge() wrote SECONDARY_BUS 0 to the new bridge and then called pci_enum_bus(0), re-walking the already configured tree from the root. Every re-walk consumed the bus numbers again and reached the same wrap, so a bridge chain deep enough to exhaust the 256 bus numbers recursed without bound (stack exhaustion / boot hang) instead of degrading gracefully. Reject the bridge when curr_bus_number is already 0xFF, before the increment: the existing error path restores the saved allocator and bus state, disables the bridge window, and leaves enumeration of the remaining buses on the parent bus untouched. With the guard, nesting is bounded at 255 bridge levels, one per bus number. unit-pci gains test_program_bridge_bus_exhaustion with the two boundary cases: at 0xFE the last usable number 0xFF is assigned and the bridge is programmed; at 0xFF the call fails, the info state is restored, and the bridge registers are cleared. Pre-fix the 0xFF case returned success with the wrapped bus number. Verification: - Built: gcc (host) unit-pci with -DWOLFBOOT_USE_PCI: clean. - Tested: unit-pci 30/30; pre-fix the 0xFF case returned 0 (ret) with curr_bus_number wrapped to 1. - Pitfalls: the guard runs after the command register is read, so the error path restores a valid orig_cmd; bridges beyond the 255th level are disabled (their windows unmapped) rather than mis-programmed, which is the same outcome the OOM path already produces for a windowless bridge. - Style: cstyle-check.sh output on src/pci.c unchanged in class from the pre-change file; the new test adds one C99-decl line in the suite registration, the class the existing registrations already trip. - Message: F-11046: prefix, no co-author trailers.
hal_flash_erase() used the length decrement as the unbraced body of the NVMREADY wait loop. With the peripheral idle (NVMREADY set) the wait body never ran, the length never shrank, and the outer loop re-erased the first page of the range forever; whatever the wait duration, the number of decrements tracked wait-loop iterations instead of completed erases, and the address was never advanced, so later pages of the requested range were never erased. Brace the ready wait, and after a completed erase advance the address by FLASH_PAGESIZE and decrement the length once, as the sibling P1021 multi-block erase loop does (F-11034). unit-samr21-erase-advance extracts the real function and register macros and runs it against a host NVMCTRL window with NVMREADY preset (an idle peripheral): a 128-byte range must end with page 0x1040 programmed, a 256-byte range with page 0xC0, and a single 64-byte erase must complete. Pre-fix all three cases hang in the re-erase loop and fail on the tcase timeout. Verification: - Built: arm-none-eabi-gcc -fsyntax-only -Wall -Wextra hal/samr21.c: clean. - Tested: unit-samr21-erase-advance 3/3; pre-fix all three timed out (10 s tcase limit). - Pitfalls: single-page erases and page-aligned ranges behave as before; a non-page-multiple len erases the final partial page's page, unchanged from the pre-existing decrement semantics. - Style: cstyle-check.sh FMT diff on hal/samr21.c byte-identical to the pre-change file; the new test trips only the uncrustify START_TEST brace class the sibling unit tests trip. - Unverified: no SAMR21 board execution. - Message: F-11036: prefix, no co-author trailers.
sdcard_card_full_init() polled ACMD41 in an unbounded do/while until the card set OCR ready, so a card that answers every ACMD41 without ever setting the bit held the bootloader in the loop forever. F-7984 bounded the separate DATA0/CMD13 waits in sdhci_wait_busy(); this is the OCR readiness path, which still had no limit. Bound the poll with the same shape as sdhci_wait_busy(): a 30000 ms budget (SDCARD_ACMD41_TIMEOUT_MS, #ifndef-able) measured against hal_get_timer_us(), the watchdog serviced inside the loop, and -1 returned to fail the SD boot path. A healthy card reports ready in milliseconds, so the budget is far above any real initialization time. unit-sdhci-acmd41-timeout compiles the real driver (generated sdhci_host.c, as in the wait-busy test) against a scripted controller: commands complete without error, SRS12 is modeled write-1-to-clear, and the card model sets OCR ready after a configurable number of ACMD41 polls. A never-ready card must return -1 inside the shipped budget (and service the watchdog); a card ready after 5 polls must exit the loop promptly and proceed to the end of the init path. A command-write cap turns the pre-fix infinite loop into an abort instead of a hung build. Verification: - Built: gcc (host) unit-sdhci-acmd41-timeout with -DDISK_SDCARD: clean. - Tested: unit-sdhci-acmd41-timeout 2/2; pre-fix the never-ready case aborted at the 50001st command (the loop never terminates); post-fix it runs 3001 polls, reaches the 30 s budget, pets the watchdog every iteration and returns -1. - Pitfalls: the timeout returns -1 from the SD path, the same contract as a failed CMD0/CMD8; the budget is per init call, not shared with sdhci_wait_busy, and a card that becomes ready before the deadline is unaffected. - Style: cstyle-check.sh FMT diff on src/sdhci.c byte-identical to the pre-change file; the new test is flag-free. - Unverified: no SD card hardware execution. - Message: F-11032: prefix, no co-author trailers.
Under NVM_FLASH_WRITEONCE wolfBoot_update_trigger() stages a whole flash sector into the file-scope NVM_CACHE before rewriting the update partition flags. In EXT_ENCRYPTED builds that sector is where the firmware key/nonce live (ENCRYPT_CACHE aliases NVM_CACHE, and with FLAGS_HOME the update flags sit in the boot trailer), so after an update trigger the plaintext key material sat in the buffer at a fixed address. The partition-trailer helpers scrub the buffer with nvm_cache_scrub() on return (F-9765); the write-once update path copied the sector and never scrubbed it. Scrub the staged sector after the final erase, before the flash lock is released. unit-update-trigger-scrub extracts the real function together with nvm_cache_scrub() (Makefile, built with NVM_FLASH_WRITEONCE) and runs the write-once branch over a staged sector carrying a key/nonce pattern: one flags write and two sector erases are expected, and the buffer must be zero after the call. Pre-fix the staged pattern remained in NVM_CACHE. Verification: - Built: gcc (host) unit-update-trigger-scrub with -DNVM_FLASH_WRITEONCE: clean, no warnings. - Tested: unit-update-trigger-scrub 1/1 (pre-fix: key pattern remained); unit-nvm-cache-scrub 3/3 (the non-write-once extraction build is unaffected). - Pitfalls: the scrub runs unconditionally in the write-once branch, which has no early-return flash-error path; the non-write-once and external-flash branches stage nothing and are unchanged. - Style: cstyle-check.sh FMT diff on src/libwolfboot.c byte-identical to the pre-change file; the new test is warning-free. - Message: F-11037: prefix, no co-author trailers.
danielinux
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 26, 2026 15:49
wolfSSL-Fenrir-bot
previously requested changes
Aug 26, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #874
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.
NVMCMD_ERASE (0x02) is the NVMCTRL row erase: one command erases a 256-byte row (4 pages). The erase loop advanced the address by one page per iteration, so it issued four row-erase commands against the same row - the second through fourth with a non-row-aligned address - quadrupling erase time and wear on every flash erase. Stride the loop by FLASH_ROW_SIZE (4 * FLASH_PAGESIZE). The row containing a sub-row request is erased once, as the command granularity requires. Unit test updated to row semantics: a sub-row request ends on the containing row, a two-row range advances to the second row, and an exact row erases once with no extra row. All three fail on the page-stride loop.
A pool ending exactly at 0x100000000 (e.g. 0xC0000000 + 0x40000000, the classic top-half 32-bit MMIO layout) was a working configuration: the old base + length - 1 initialization wrapped to 0xFFFFFFFF in 32-bit arithmetic. The overflow guard from the exclusive-limit fix rejected such pools with base + length > 0xFFFFFFFF, aborting enumeration - and the FSP caller discards the return value, so the platform would boot with no PCI BARs programmed. The limit fields cannot hold the exclusive end 0x100000000 while 32-bit, so widen mem_limit and mem_pf_limit (and the limit parameters of pci_enum_next_aligned32 and pci_align_check_up, plus the local in pci_program_bar) to 64-bit, and reject only pools whose end is above the 32-bit space. The initialization now casts to 64-bit before the addition so the sum cannot wrap. The T10xx PCIe setup initializes the same struct with the old inclusive base + length - 1 form; align it to the exclusive semantics the allocator enforces, or the last byte of the configured pool is unusable. New unit-pci-4gib build of the existing test file with the MMIO pool [0xC0000000, 0x100000000): pci_enum_do() must accept the pool and map a 1 MB BAR at the pool base. Fails on the old guard.
wolfBoot_update_trigger() derives the staged sector by rounding the update flags address down to a sector boundary and copies a full sector from it. The g_sector fixture carried no sector alignment, so the boundary landed inside the array and the copy read up to 4095 bytes past its end - the test passed only because the over-read happened to fall in a neighboring global. Align g_sector to WOLFBOOT_SECTOR_SIZE so the staged sector is the array itself; the copy stays in bounds by construction. Also ignore the generated unit-test extraction headers, as the other generated sources in that list are.
wolfSSL-Fenrir-bot
previously requested changes
Aug 26, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #874
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 3
2 finding(s) posted as inline comments (see file-level comments below)
Required changes (1)
MMIO allocator cursor wraps to 0 for pools ending at 4 GiB
File: src/pci.c:527
Function: pci_program_bar
Category: Integer overflows
The widened uint64_t limit now accepts a region whose exclusive end is 0x100000000, but the cursor info->mem/info->mem_pf stays 32-bit, so *base wraps to 0. Subsequent BARs and bridge windows are then programmed at physical address 0, aliasing device MMIO over DRAM instead of being skipped. Adjacent to known finding #11031, which only concerns the inclusive-vs-exclusive limit init.
Related known finding #11031 (similar but distinct): Both affect PCI MMIO pool boundary handling, but this finding is in pci_program_bar's 32-bit cursor update, where adding a BAR length truncates at 4 GiB. #11031 is in pci_enum_do's initialization of an exclusive limit as inclusive. Widening the cursor fixes this finding; correcting limit initialization fixes #11031, so separate patches are required.
Recommendation: Widen the mem/mem_pf cursors in struct pci_enum_info to 64-bit as was done for the limits, so the end-of-pool update cannot truncate.
Referenced code: src/pci.c:527-531 (5 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
The cursors mem, mem_pf and io advanced as 32-bit values: after a BAR allocation whose end is exactly the pool end 0x100000000 (now reachable), *base = bar_value + length wrapped to 0. Every later allocation then passed the start and end checks and programmed its BAR at address 0, over the legacy IO range and DRAM. Widen the three cursors to 64-bit and advance with a 64-bit sum, so an exhausted pool leaves the cursor at the end. The address parameters of pci_enum_next_aligned32 and pci_align_check_up widen with them; pci_enum_next_aligned32 computes in uint64_t rather than uintptr_t, which on 32-bit targets would truncate the exhausted cursor back to 0 and defeat the addr > 0xffffffff rejection. The programmed BAR value stays 32-bit. The 4 GiB pool unit test now also adds a second device with a preset (previously programmed) BAR: exactly filling the pool must leave that BAR untouched instead of re-allocating it from a wrapped cursor. The mock learns to seed a BAR preset from the bar info, and the loop variable shadowing in test_pci_commit that it exposed is fixed (the inner preset loop clobbered the outer node loop counter, so only the first node was ever committed). Verified: unit-pci 30/30, unit-pci-4gib green, full nxp_t1024 powerpc build green.
The hal_flash_write stub discarded the written data, so the test only checked call counts and the final NVM_CACHE state. An implementation that scrubbed before the write - programming an all-zero flags sector and destroying the firmware key that persists in the trailer - would have satisfied every assertion. Capture the written sector in the stub at call time and assert the staged payload: the sector fill and key pattern intact, the fresh IMG_STATE_UPDATING byte and magic in place, with NVM_CACHE still required to be zero afterwards. Mutation-checked: a scrub-before- write variant that programs the zeroed sector fails the payload assertion.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #874
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.
wolfSSL-Fenrir-bot
dismissed stale reviews from themself
August 26, 2026 18:46
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
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.
9b89b5e F-11037: scrub NVM_CACHE after the write-once update trigger
c471357 F-11032: bound the ACMD41 OCR readiness poll in SD card init
300e5db F-11036: advance the page address in the SAMR21 erase loop
7290d87 F-11046: reject bridge programming when the bus number is exhausted
7db653b F-11031: initialize PCI MMIO pool limits as exclusive ends