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.
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.
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.
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.
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.
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.
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.
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.
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.
Four review items on this PR, all valid:
- unit-hifive1-flash-write: the over-read test assumed the canary
array landed right after the data array on the stack, which C does
not guarantee (and the canary was filled but never read). Use a
single contiguous buffer split into data and canary regions so an
out-of-range read lands on known bytes, and assert the canary stays
intact after the call.
- unit-fwtpm-rsp-overrun: rsp_fitting_capacity_gets_response checked
the guard bytes with a loop bounded by rspSz, which the call had
already overwritten to the produced size (10), so the loop never
ran. Snapshot the offered capacity before the call and bound the
guard check with that.
- unit-fdt-memrsv-wrap: put32() encoded header fields by calling
fdt32_to_cpu(), which reads as the inverse operation. Call
cpu_to_fdt32() directly, matching how production code writes FDT
fields.
- Makefile: the five new extraction headers (fdt_memrsv, hifive1
flash write, t10xx flash status, p1021 erase x2) were not in
GENERATED_SRC, so make clean left them behind. Listed now.
Verification:
- Built: the three affected unit tests compile clean on the rebased
branch.
- Tested: unit-hifive1-flash-write 3/3, unit-fwtpm-rsp-overrun 3/3,
unit-fdt-memrsv-wrap 3/3; make -n clean now removes all five
extraction headers.
- Pitfalls: the hifive1 test now passes an explicit request length
(data is a pointer into the shared buffer, so sizeof would be
wrong).
- Style: cstyle-check.sh flag count unchanged per file (1 pre-existing
FMT pointer-alignment class each).
- Message: no co-author trailers.
fdt_add_mem_rsv() added the 32-bit string-block offset and size
without overflow checks. A wrapped data_end bypassed the capacity
check, and the same wrapped expression derived the memmove length, so
a malformed (or attacker-supplied) DTB produced a huge memmove -
broad boot-time memory corruption. fdt_check_header validates only
magic and version, so raw-DTB callers reach this code with
inconsistent layout fields.
Compute the block end in 64-bit, validate the layout before touching
it (structure block starts after the reserve map terminator, string
block after the structure block, shifted layout fits in totalsize),
and derive the move length only from the validated 64-bit end. The
reserve-map scan bound uses 64-bit arithmetic as well, so a wrapped
32-bit sum cannot pass it.
unit-fdt-memrsv-wrap extracts the real fdt_add_mem_rsv (plus the
byte-order helpers) and feeds it crafted DTB headers:
- a wrapped end below off_dt: pre-fix the memmove length wraps to
~2^32 and the process segfaults; post-fix rejected (-FDT_ERR_NOSPACE)
- a wrapped end inside [off_dt, total): pre-fix the layout was
accepted (ret == 0) with a silently corrupted FDT; post-fix rejected
- a consistent layout: the entry is inserted, the terminator moves
down one, structure and string blocks shift by 16 bytes, and the
header offsets follow (regression guard, passed pre-fix as well)
Verification:
- Built: unit test compiles the extracted real function (host).
- Tested: unit-fdt-memrsv-wrap 3/3 post-fix; pre-fix (fix stashed)
1 segfault + 1 assertion failure on the wrap cases, valid case
passing - red demonstrated on both corruption modes.
- Pitfalls: the helper that builds the crafted DTB only writes block
contents where the offsets fit the buffer, so the malformed cases
cannot corrupt memory in the test itself before reaching the code
under test; validation runs before any block access.
- Style: cstyle-check.sh flag count on src/fdt.c unchanged (1
pre-existing FMT class); the new test trips only the uncrustify
class the sibling unit tests trip.
- Message: F-11045: prefix, no co-author trailers.
- Unverified: no target build needed (pure C, host-compiled from the
real source); fdt.c compiles as part of the normal wolfBoot build
paths unchanged.
wcs_fwtpm_transmit() verified that cmd identifies non-secure memory
and then passed that mutable buffer directly to FWTPM_ProcessCommand.
The processor parses the packet more than once (authentication, then
handler execution), so a DMA-capable non-secure attacker who rewrites
the command buffer in the window between the two parses can make the
authenticated command differ from the executed command.
Copy exactly cmdSz bytes into a secure staging buffer after the range
validation and invoke the processor only on that copy: an NS DMA
master cannot rewrite secure memory, so both parses see the same
bytes. The command and response staging are zeroed before returning
(the response may carry auth tags or unsealed data).
unit-fwtpm-cmd-toctou includes the real fwtpm_callable.c and mocks
FWTPM_ProcessCommand with two parse points (authentication,
execution). The test plays the attacker, rewriting the NS command
buffer at the window between the parses - the mock may only touch the
caller's buffer, never secure staging, mirroring the hardware
boundary. Pre-fix the processor authenticated the original bytes and
executed the rewritten ones (test fails); post-fix both parses see
the original command.
Verification:
- Built: unit tests compile the real veneer (host, poisoned fwtpm
headers, same pattern as unit-fwtpm-nv-oob).
- Tested: unit-fwtpm-cmd-toctou 2/2 (red demonstrated against the
pre-fix veneer via git stash of the fix); sibling
unit-fwtpm-rsp-overrun 3/3 and unit-fwtpm-nv-oob 4/4 after the
change.
- Pitfalls: the staging copy happens after all NS range checks and
before any processor access; zeroing covers the full staging
buffers regardless of the produced length.
- Style: cstyle-check.sh flag count on src/fwtpm_callable.c unchanged
(1 pre-existing); the new test trips only the uncrustify
pointer-alignment class the sibling unit tests trip.
- Message: F-11044: prefix, no co-author trailers.
- Unverified: no CMSE/armclang build and no m33mu emulator run here
(lib/wolftpm is not checked out in this tree); the trustzone-emulator
workflow covers the full build on push.
wcs_fwtpm_transmit() validated the caller-supplied response capacity
and then passed that buffer directly to FWTPM_ProcessCommand with
rspLen initialized to the capacity. The fwTPM processor emits a
10-byte TPM error response even for a malformed short command,
regardless of the offered capacity, so a non-secure caller offering
less than 10 bytes got an out-of-range write into its response buffer
before the wrapper compared rspLen against the capacity.
Process into a max-sized staging buffer inside the veneer and copy to
the caller's buffer only after verifying the produced length fits the
snapshotted capacity; when it does not fit, return TPM_RC_FAILURE
without touching the buffer. The staging buffer is file-scope static:
the CMSE secure callable is not preemptible, so no locking is needed.
unit-fwtpm-rsp-overrun includes the real fwtpm_callable.c and mocks
FWTPM_ProcessCommand to emulate the processor's behavior of writing
the full 10-byte error response no matter the offered capacity. A
6-byte capacity must leave the guard bytes past the capacity intact
(fails pre-fix: bytes 6-9 were overwritten); 16-byte and exact-10
capacities must receive the full response.
Verification:
- Built: unit tests compile the real veneer (host, poisoned fwtpm
headers, same pattern as unit-fwtpm-nv-oob).
- Tested: unit-fwtpm-rsp-overrun 3/3 (pre-fix the short-capacity test
caught the overrun at offset 6); sibling unit-fwtpm-nv-oob 4/4
after the change.
- Pitfalls: the copy uses the snapshotted capacity taken before the
call, so a concurrent NS write to *rspSz cannot widen the range;
the NS_R/NS_RW checks are unchanged.
- Style: cstyle-check.sh flag count on src/fwtpm_callable.c unchanged
from the pre-change file; the new test trips only the uncrustify
pointer-alignment class the sibling unit tests trip.
- Message: F-11043: prefix, no co-author trailers.
- Unverified: no CMSE/armclang build and no m33mu emulator run here
(lib/wolftpm is not checked out in this tree); the trustzone-emulator
workflow covers the full build on push.
hal_flash_write() in hal/hifive1.c selected the page path and clamped
the partial-page length from the original total len instead of the
bytes still remaining (len - j). A page-aligned multi-page write that
ended in a partial page therefore took the full-page branch on the
last iteration: it read past the end of the caller's buffer and
programmed a full 256-byte page where only the remaining bytes were
requested, clobbering flash past the update range.
Compute remaining = len - j at the top of the loop and use it for both
the branch test and the rel_len clamp; j still advances only by the
bytes actually consumed (256 on the full-page path, rel_len on the
partial path).
unit-hifive1-flash-write runs the real extracted function against a
mock fespi model (FLASH_BASE points at a flash image buffer,
fespi_write_address/fespi_sw_tx program into it, and the RMW path
reads the image back through FLASH_BASE as on hardware). The
regression case is a 356-byte aligned write: the last page's tail must
stay erased, which fails pre-fix (the over-read bytes are programmed
instead).
Verification:
- Built: riscv-none-elf-gcc 15.2 -fsyntax-only -Wall with ARCH_RISCV:
clean.
- Tested: unit-hifive1-flash-write 3/3; pre-fix the 356-byte case
wrote non-erased bytes past offset 356 of the flash image.
Unaligned single-page RMW and exact-full-page cases unchanged.
- Pitfalls: relative (sub-FLASH_BASE) addresses are accepted as-is by
the function, which is what the test passes so the 32-bit address
parameter never carries a 64-bit host pointer.
- Style: cstyle-check.sh flag count on hal/hifive1.c unchanged from
the pre-change file; the new test trips only the uncrustify
pointer-alignment class the sibling unit tests trip.
- Message: F-11035: prefix, no co-author trailers.
- Unverified: no HiFive1 board execution.
ext_flash_erase() decremented the remaining length each iteration but
never advanced the address, so the derived page (address / page_size) was identical
every pass: the first block of the range was re-erased for the whole
loop and every later block was left intact (the caller then programmed
an update image into un-erased NAND). The sibling ext_flash_write()
loop already advanced address/pos/data and was not affected.
Advance address by block_size after each successful erase.
unit-p1021-erase-advance runs the real extracted function against
mocked ELBC register access and records the page programmed per erase
command: two blocks must hit page 0 then page 32 (16 KiB block / 512
page), and a failing command must stop the loop after one attempt.
Verification:
- Built: powerpc-linux-gnu-gcc -fsyntax-only -Wall with
TARGET_nxp_p1021: clean.
- Tested: unit-p1021-erase-advance 2/2; pre-fix the second erase
re-targeted page 0.
- Pitfalls: single-block erases (len <= block_size) behave exactly as
before; the error path is unchanged (break on hal_flash_command
failure).
- Style: cstyle-check.sh flag count on hal/nxp_p1021.c unchanged from
the pre-change file; the new test trips only the uncrustify
pointer-alignment class the sibling unit tests trip.
- Message: F-11034: prefix, no co-author trailers.
- Unverified: no P1021 board execution.
hal_flash_status_wait() returns -1 when the NOR does not settle its
status bits within the poll budget, but hal_flash_write() and
hal_flash_erase() discarded the result and returned 0 unconditionally:
a stuck program or erase reported success, and the update flow
continued as if the flash held the new image.
Capture every wait result and return it to the caller on the first
failure. No state-restore command is needed: this driver works through
the memory-mapped QPI window, where reads are plain loads and the
controller issues the read command per access, so there is no device
command state to recover.
unit-t10xx-flash-status runs the real extracted functions against a
mock QPI status model (offset 0 reports the DQ status byte: toggling
while busy, 0x44 after a program, 0x4C after an erase). A stuck
device burns the full 200 ms / 1.1 s poll budget with a no-op udelay,
so the timeout path runs in milliseconds.
Verification:
- Built: powerpc-linux-gnu-gcc -fsyntax-only -Wall with
TARGET_nxp_t1024: clean.
- Tested: unit-t10xx-flash-status 4/4; pre-fix both timeout tests got
ret == 0 from a stuck device, now -1. Success paths (write lands in
the model, erase completes) unchanged.
- Pitfalls: the first failing page/sector now aborts the rest of the
operation, which is the desired behavior (the caller aborts the
update); no callers depended on the unconditional 0.
- Style: cstyle-check.sh flag count on hal/nxp_t10xx.c unchanged from
the pre-change file; the new test trips only the uncrustify
pointer-alignment class the sibling unit tests trip.
- Message: F-11033: prefix, no co-author trailers.
- Unverified: no T10xx board execution.
sbi_ipi_irq() read-and-cleared the per-hart op word before executing
the requested fence.i/sfence.vma, and sbi_wait_ipi_done() treated a
zero op word as completion. The SBI remote-fence ecalls are
synchronous, so a requester could return while the target had not yet
run its fence (e.g. resume relying on a new page table before the
target flushed its TLB).
Split the protocol into pending work and completion state, per hart:
ipi_done[h] is incremented by the target only after it has executed
the fence ops it consumed; the requester snapshots it into
ipi_wait_gen[h] before posting (so a concurrent coalesced consume of
two requesters' ops still increments past both snapshots) and waits
until ipi_done[h] passes the snapshot. SSIP posts are fire-and-forget
and do not wait, as before.
Verification:
- Built: riscv-none-elf-gcc 15.2 -fsyntax-only -Wall with
WOLFBOOT_RISCV_MMODE + WOLFBOOT_MMODE_SMODE_BOOT: clean.
- Tested: none (race condition; the contract skips failing-first tests
for races, and the file is MPFS S-mode monitor code with no host or
CI build target).
- Pitfalls: the shared DTIM struct gains two per-hart arrays; the
struct is self-initialized under init_magic by this same code on
every hart, so no cross-version ABI is broken. A target that never
runs M-soft still hits the bounded spin timeout as before. The
completion increment covers only fence ops, matching the only
waiting call sites (both RFENCE paths).
- Style: cstyle-check.sh flag count unchanged from the pre-change
file (3 pre-existing).
- Message: F-11029: prefix, no co-author trailers.
- Unverified: no multi-hart runtime execution (MPFS board only).
The RAM boot loop switched to the other partition on a verification
failure (active ^= 1, continue) with a comment claiming the failing
image was invalidated, but nothing was invalidated: wolfBoot_fallback_is_possible()
only sees that both partitions carry nonzero versions. Two present
but invalid images therefore alternated indefinitely, re-verifying
forever. (The flash path does not have this hole because it erases
the failing partition, which zeroes its version and makes the second
fallback check fail.)
Track the candidates attempted this boot: once both partitions have
failed, panic with a clear message. Same-version images are used in
the regression test so the anti-rollback guard cannot mask the
alternation.
unit-update-ram gains test_both_images_corrupted_panics: both
partitions carry valid-version images with corrupted digests; the
boot must panic after exactly two attempts. Pre-fix the loop ran
unbounded (process had to be killed; ~60k partition switches in 30 s).
Verification:
- Built: unit-update-ram, unit-update-ram-enc, unit-update-ram-nofixed,
unit-update-ram-uboot all compile.
- Tested: unit-update-ram 20/20, unit-update-ram-nofixed 3/3,
unit-update-ram-uboot 5/5; the new test shows Boot fail, Update
fail, panic.
- Pitfalls: attempt tracking is per boot session (local), no new
persistent state; the flash path is untouched.
- Style: cstyle-check.sh flag count on both changed files is
unchanged from the pre-change versions (pre-existing FMT/R1).
- Message: F-11028: prefix, no co-author trailers.
- Note: unit-update-ram.c defines 21 tests but wires 20 into the
suite (test_forward_update_samesize_notrigger was never added);
pre-existing, left as-is.
elf_load_image_mmu() skipped a segment (continue) when its mmu_cb
mapping failed, then kept loading the rest, published the ELF entry
point and returned success for a partially loaded image. The x86 FSP
payload path (boot_x86_fsp_payload.c) passes a real mmu_cb and only
panics on a non-zero return, so the buggy continue booted a payload
with a missing segment.
Return -6 with a fail-loud message, matching the program-header
clobber guard that aborts for the same reason: never silently drop a
a PT_LOAD segment.
unit-elf-mmu-fail fails the first segment's mapping and checks the
load is rejected with no entry point published; a second test checks
successful mappings still load the segments and publish the entry.
Verification:
- Built: unit test compiles elf.c (WOLFBOOT_ELF config); elf.c
syntax-clean under the WOLFBOOT_FSP config (gcc -fsyntax-only).
- Tested: unit-elf-mmu-fail 2/2; pre-fix the failure-path test got
ret == 0 (entry published for a partially loaded image).
- Pitfalls: no caller switches on the exact code (all check != 0);
-6 is new and dedicated to the mapping failure.
- Style: cstyle-check.sh on src/elf.c flags pre-existing FMT/R1
issues also present on the pre-change file; the new test trips the
uncrustify pointer-alignment class the sibling unit tests trip and
matches their local style.
- Message: F-11027: prefix, no co-author trailers.
wolfBoot_start() in hal/library.c ended its exit: path with an
unconditional 'return 0;', so a rejected image (bad header, hash or
signature) still made the test-lib process exit 0. main() propagates
wolfBoot_start()'s return value, so the failure was only visible in
the printed "Failure" message, which the test-library workflow had to
grep for (TODO referencing PR #625).
Return ret, which carries the wolfBoot_verify_*() error on every path
that reaches exit: with a failure. The success path never returns:
do_boot() jumps to the firmware.
The test-library workflow drops the status-rewriting workaround and
asserts the non-zero exit code directly, keeping the "Failure"
message check as a diagnostic.
Verification:
- Built: make test-lib (host, library.config, ED25519/SHA256).
- Tested: local repro of the workflow flow: corrupt the last byte of
a signed image; before the fix the process exited 0 while printing
"Failure -1", after the fix it exits 255; a valid image still
exits 0 with "Firmware Valid".
- Pitfalls: 'return ret' only changes the error paths; do_boot() does
not return on success.
- Style: the cstyle-check.sh FMT flag on hal/library.c is present on
the pre-change file as well (not introduced here).
- Message: F-9749: prefix, no co-author trailers.
The wolfHSM verify path of wolfBoot_verify_signature_ecc() converts
the fixed-width raw R||S signature to DER with wc_ecc_rs_raw_to_sig().
It passed the minimal field sizes (mp_unsigned_bin_size) while leaving
the pointers at the start of each fixed-width field, so whenever R or
S had a leading zero byte the conversion encoded a zero-padded integer
with the low bytes truncated, and wc_ecc_verify_hash() rejected an
otherwise valid signature (each component has roughly a 1 in 256
chance of a leading zero).
Pass the full-width fields (point_sz for both): the raw signature is
fixed-width and left-zero-padded, and the conversion strips the
padding itself. The multiprecision sizing only existed to compute the
minimal lengths and is dropped with the fix.
unit-ecc-raw-der signs until a leading-zero signature shows up, then
checks that the minimal-size pattern rejects it while the full-width
pattern accepts it, and that both patterns agree for leading-zero-free
signatures.
Verification:
- Built: src/image.c syntax-clean with WOLFBOOT_ENABLE_WOLFHSM_CLIENT
(gcc -fsyntax-only, __WOLFBOOT, ECC256/SHA256 config, partition
stubs); normal library build via make test-lib.
- Tested: unit-ecc-raw-der 3/3: the minimal pattern rejects the
leading-zero signature the full-width pattern accepts; both agree
on leading-zero-free signatures.
- Pitfalls: no key material touched; the dropped mp values had no
matching mp_clear before the fix either (stack variables).
- Style: cstyle-check.sh on src/image.c flags two pre-existing
violations (L1869 anonymous union, L2079 C99 declaration) outside
this change; the new test trips the same uncrustify
pointer-alignment class the unit-stm32l5/u5-write twins trip and
matches their local style.
- Message: F-11024: prefix, no co-author trailers.
- Unverified: no wolfHSM target builds in CI; the HSM branch was
checked by syntax-only compile, not a full target build.
The double-word fast path in hal_flash_write() on STM32G4, STM32C0
and STM32G0 was selected on 'len - i > 3' but programs an 8-byte
unit, so an aligned 4-7 byte tail read up to 4 bytes past the
caller's buffer and programmed those bytes into flash.
Require len - i >= 8 before taking the fast path. Shorter tails
fall through to the existing RMW branch, which rewrites the unit
with the out-of-range bytes read back from flash, so nothing past
len is read or programmed.
Add unit-stm32g4-write (same harness as the STM32L5/STM32U5 twins),
which fails on the 60-byte tail before the fix.
The page walk recomputed the block from the address each iteration,
losing the running cursor.
Carry the cursor again, checking for a bad block only when it rolls
over. unit-sama5d3-ext-read now emulates bad blocks.
12 of the 22 test-size-all configs grew 4B (ECC384 NO_ASM 16B), all
within the 32B-per-config ratchet. Re-measured in the CI footprint
container (ghcr.io/wolfssl/wolfboot-ci-arm:v1.0) with the exact CI
sequence (stm32f407-discovery config, keytools, per-signature rebuilds)
and ratcheted each grown limit to the measured size; test-size-all
passes 22/22 with the new limits.
RSAPSS2048/3072/4096 (asm) shrank 4B; their limits are left as-is.
unit-sama5d3-ext-read joined the ENABLE_32BIT_TESTS gate but the info
line still only named the linux-loader tests, which misleads anyone
debugging a skipped suite.
Skoll review finding 5, 2026-08-21 wolfboot review.
Since F-9750 the head and tail read-modify-write paths decrypt the
stored neighbour block into block/enc_block before splicing the
caller's bytes, so the two stack buffers transiently hold plaintext the
caller never supplied, and several exits returned without scrubbing
them (stale head plaintext also outlived into the tail path). Funnel
every exit after the partition switch through a single cleanup that
ForceZero()s both buffers, matching the zeroization posture of the rest
of the campaign (F-7396 header cache, F-7966/F-7971 keys, aes_set_iv
IV). Defense-in-depth: the buffers are stack-local, but this is the most
long-lived plaintext in the write path.
Skoll review finding 4, 2026-08-21 wolfboot review.
The WOLFBOOT_DTS_MAX_SIZE/WOLFBOOT_DTS_MIN_SIZE pair was defined
separately in update_disk.c (F-7066) and update_ram.c (pre-existing), so
the two copies could drift. Move it to include/fdt.h, the FDT dialect
header both translation units already pull in via image.h; the hal
override (nxp_ppc.h, included before fdt.h in boot_ppc.c) keeps its
precedence. Also replace the 'bounded by the staging region' comment,
which claimed more than the code guarantees: the copy is clamped to
WOLFBOOT_DTS_MAX_SIZE, so the staging window at
WOLFBOOT_LOAD_DTS_ADDRESS must be at least that large (or the bound
overridden for the target), and the header comment now says so.
Skoll review finding 3, 2026-08-21 wolfboot review.
The F-9750 E2E roundtrip writes sector-aligned chunks, so it never enters
the head/tail read-modify-write paths where the iv_offset_at_entry
re-syncs live, and the RMW neighbour tests only run with the standard IV.
The combination the fix protects - a fallback-IV write whose head and
tail partial blocks land on already-encrypted blocks - was untested: a
dropped re-sync offset re-anchors exactly one block at the standard-IV
position and no current test would catch it.
New test primes two blocks under the fallback IV, patches them with one
unaligned write (head RMW + tail RMW, block-size independent so it runs
on the 16-byte AES and 64-byte ChaCha builds), and reads back with the
fallback IV forced the way the update flow does. Verified to fail when
the head RMW re-sync offset restore is removed.
Skoll review finding 2, 2026-08-21 wolfboot review.
The F-9756 validation rejected seg_start > UINT64_MAX - filesz, but the
very next line truncates: load_addr = (uintptr_t)seg_start. On 32-bit
targets a paddr that fits in 64 bits but not in the 32-bit address space
(e.g. 0x1_0000_0000) passed every check and the flash hash walk read the
wrapped (possibly unmapped) address - the same fault class the check was
written to prevent. Bound the range by UINTPTR_MAX, the width the cast
actually uses, and pin the 32-bit-only case with a guard test.
Skoll review finding 1, 2026-08-21 wolfboot review.
Daniele Lacamera
David Garske