Batch of Fenrir fixes: 2026 09 24 - #910
Open
danielinux wants to merge 50 commits into
Open
danielinux wants to merge 50 commits into
danielinux wants to merge 50 commits into
Conversation
An undersized e_phentsize made the last program-header loop iteration read past the validated table (and past the image buffer when the table ends at the image end). Reject such images with -3. Verification: tools/unit-tests/unit-elf-phentsize fails pre-fix (undersized ELF accepted, ret=0), passes post-fix (2/2); full make -C tools/unit-tests run green.
With an unaligned start address the byte-wise prefix leaves i % 4 != 0, so dst[i >> 2] programmed the double word below the intended address and left the tail bytes of the unit unprogrammed. Derive src/dst from address + i, matching the already-fixed else branch. Verification: new unit-stm32g4-write test_write_unaligned_start fails pre-fix (memcmp mismatch), 5/5 post-fix; full make -C tools/unit-tests run green.
With matched-but-unaligned address/data residues the byte-wise prefix leaves i % 4 != 0, so dst[i >> 2] stored the word below the intended address, dropped the tail bytes of the unit, and issued an unaligned NVM store (HardFault on M0+). Derive src/dst from address + i, matching the already-correct else branch. Verification: new matched-alignment test fails pre-fix in both unit-flash-write-samr21 and unit-flash-write-same51 (tail bytes stay 0xFF), 3/3 post-fix; full make -C tools/unit-tests run green.
Input longer than the 128-byte conversion buffer was silently truncated: the loop stopped at the buffer edge and one raw write dropped the rest. Flush on full (and before a newline that no longer fits) and keep converting. Verification: new unit-nrf54l-uart-write fails pre-fix 3/4 (output capped at 128 bytes), 4/4 post-fix; full make -C tools/unit-tests run green.
A second -r replaced the ranges array without freeing the first (leak) and silently discarded its ranges. Reject the repeat with a pointer to the comma-separated form. Verification: new tools/squashelf/test-repeat-range.py fails pre-fix (repeated -r accepted), passes post-fix; full make -C tools/squashelf test green (8/8).
hal_flash_write() picked the bank from the start address and programmed the whole request on that bank, so a request crossing into bank 2 wrote its tail to the wrong bank. The exported writer now splits at FLASH_BANK2_BASE_REL into two per-bank passes; a failure in the first pass stops the split. Unit test extracts the dispatcher and mocks the per-bank pass: cross-boundary split, single-bank pass-through, and first-pass failure all covered; fails pre-fix.
hal_flash_write() cleared errors before programming and waited for completion but never read the post-program status register, so OPERR/PROGERR/WRPERR/PGAERR/SIZERR/PGSERR faults were swallowed and a failed program was reported as success. Check FLASH_NS_SR after the wait, clear the flags, and return -1 on fault; reject addresses not aligned to the 8-byte double-word programming unit. Unit test extracts the writer with stubbed registers: success, PROGERR, WRPERR and misaligned-address cases; fails pre-fix.
The not-found sentinel is -1, so the early-exit test must be start >= 0 && end >= 0; with '> 0' any range involving sector zero scanned the whole table. Behavior-preserving: each address falls in exactly one bucket, so the full scan produced the same result.
A corrupted node-table pos was used to compute the object address without a range check, unlike bitmap_put() which rejects positions outside KEYVAULT_MAX_ITEMS. Both storage implementations now delete the node and return NULL when pos is out of range, matching the existing "cannot recover object payload" path. Verification: new test seeds the node table with pos = KEYVAULT_MAX_ITEMS and matching id's in the backup sector, so the unfixed restore path reaches the out-of-range slot (mock flash fails on the erase); fails pre-fix in unit-psa_store and unit-pkcs11_store, 10/10 and 13/13 post-fix; full make -C tools/unit-tests run green.
The update_store_size()/store_live_size() guards only bounded the first byte of the 32-byte struct obj_hdr (and used '>'), so a header offset in (SECTOR_SIZE-20, SECTOR_SIZE] would write/read the 'size' field into the adjacent cache slot. Compute the offset first and reject anything past SECTOR_SIZE - sizeof(struct obj_hdr), in all three copies (pkcs11 update_store_size, pkcs11 store_live_size, psa update_store_size). New tests forge a header at SECTOR_SIZE-16 and assert the write is rejected without touching the adjacent slot (pkcs11) / without any flash traffic (psa). Full unit suite green.
The UART formatter recognized 'l' and 'z' length modifiers and %p, but still consumed the argument as int, truncating long, size_t and pointers on 64-bit targets (undefined behavior). Track the 'z' modifier and, where PRINTF_LONG_LONG is enabled, consume long / unsigned long / void * and format through the 64-bit uart_writenum_ll() path; the 32-bit path is unchanged (long == int width there). New test test_uart_printf_64bit_args covers %ld, %lu, %zu, %p, %lx with values past 32 bits plus %lld and %d regressions; fails pre-fix (truncated output), green post-fix. 32-bit compile of string.c verified with -m32 -Wall -Wextra.
A negative '*' width passed the upper-bound-only clamp and reached the zero-pad memset as a huge size_t (stack overflow). Clamp to zero at the parser, the single source of dynamic widths; the existing zero-to-8 default then applies. New test test_uart_printf_negative_star_width covers %0*x and %0*llu; SIGSEGV pre-fix, green post-fix. Full unit suite green.
uart_init() initialized a local to zero and tested that local, so the LCR stop-bit field was always zero (one stop bit) and stop values other than 1/2 were silently accepted. Validate stop against 1/2 and derive the field from the parameter. Add unit-x86-uart (linked real driver, io_write8/io_read8 mocked) covering 8N1/8N2/5O2 LCR encodings plus stop-count and existing argument rejection; 3 of 5 tests fail pre-fix.
- ext_flash_read() backends return the number of bytes read; the ramboot payload copy only rejected negative returns, so a positive short read left a truncated image in the RAM load region (executed when WOLFBOOT_SKIP_BOOT_VERIFY is set) - require ret == img_size, mirroring the header read in the same function and the wolfBoot_start() copy (F-12065 fix) - unit test: mock short read on the payload copy aborts ramboot
wolfBoot_get_blob_version() returns uint32_t, but the value was assigned to 'int ret' and rejected when 'ret <= 0'. Versions with bit 31 set (e.g. 0x80000001) are valid unsigned versions and were silently rejected with 'No valid image found'. - check the version as uint32_t, rejecting only version 0 - add test_ramboot_high_bit_version (version 0x80000001 ramboots)
…743) HFROSCCFG_DIV_SHIFT(d) masked the shifted value with HFROSCCFG_TRIM (bits 16-23) instead of HFROSCCFG_DIV (bits 0-3), so the divider always programmed zero. - mask with HFROSCCFG_DIV - add unit-hifive1-hfrosccfg pinning the field macros (value, overlap, and the composed init register value)
SIGN=NONE, ALLOW_DOWNGRADE and DISABLE_BACKUP were applied silently in the CMake build path while options.mk prints a prominent warning for each. Surface the same wording via message(WARNING) so both supported build systems alarm identically.
- ext_flash_read/ext_flash_write forwarded unvalidated (address, len) to the SDK flash_stream_* primitives; a request past the 16 MB device end reached the driver unchecked, unlike the overflow guard already in ext_flash_erase. - Add ext_flash_in_layout(): reject negative len, base at/past the 16 MB device end, and any end offset beyond it (the base check keeps the size subtraction from wrapping). - Verified: host -fsyntax-only of the bare backend compiles clean; make -C tools/unit-tests run passes.
- The seven RAM start-table scratch slots sit at hard-coded 256-byte strides (last one 512 bytes); nothing checked the 2048-byte buffer still covers them if the layout or the buffer is edited. - Add a _Static_assert on the slot budget plus a comment recording the per-slot contract and the sizeof() re-derivation owed when SPIC adaptor reuse lands (SDK struct sizes are only visible in the SDK backend, so full sizeof-based slots are deferred to that toolchain). - Verified: host -fsyntax-only clean at 2048; shrinking the buffer to 1024 fails with the assertion message; make -C tools/unit-tests run passes.
mmap_file() left the file descriptor open when mmap() failed; the current callers exit(-1) so the leak was invisible, but close it on the error path.
hal_flash_erase() selected the bank geometry from the start address only; a range extending past the end of that bank was partially erased and reported as success. Derive the bank end from the sector geometry and reject cross-bank requests before erasing anything. New unit test extracts the real hal_flash_erase() and runs it against an emulated two-bank FAPI.
A mismatch at index zero returned -0 (success-looking), and a successful test returned the ext_flash_read() byte count. Return a fixed -1 on mismatch and 0 on success, and have the hal_init diagnostic caller log a failure.
For len <= 0 the inclusive end (address + len - 1) underflows and the sector loop runs through address wraparound. Treat a non-positive length as a no-op before computing the end.
The compression property is only known to be NUL-terminated for the recognized values; the unsupported and no-destination diagnostics formatted the raw property with %s, letting a malformed value read past the property in uart_vprintf's strlen. Copy at most 23 bytes into a local buffer and log that.
wolfBoot_check_flash_image_elf() discarded the results of every read_flash_fwimage() and update_hash_flash_fwimg() call, so a rejected range left ph_buf/ph_next_buf stale or uninitialized and the parser consumed indeterminate values. Check each result and return -1 on failure, matching the pattern wolfBoot_load_flash_image_elf already uses.
A token longer than the name buffer left its remainder in the stream, which the next call read as a second, spurious slot name. env_next_name now consumes the whole token and returns -1; the select_slot gate treats <= 0 as malformed. Adds unit tests driving the static directly (70-char token with and without a trailing valid name).
The inner dec_block declaration shadowed the function-scope buffer; the trailer now decrypts into the outer buffer.
Per-file entries, matching the existing convention; covers the artifacts added by recent unit-test campaigns that were never registered.
The dispatcher split cross-bank requests at FLASH_BANK2_BASE_REL (0x00100000), but callers pass absolute flash addresses in FLASHMEM_ADDRESS_SPACE, so the split never fired and the whole request was programmed on the bank of the start address. Compare against FLASH_BANK2_BASE (0x08100000); the test now drives absolute addresses so it exercises the real call convention.
The MCXA ROM API (fsl_romapi.h) takes lengthInBytes, not a sector count, so the original pass-through was already correct and the sector-count conversion erased almost nothing. It also called FLASH_GetFlashConfig, which does not exist in the MCXA ROM API and only compiled against the unit-test stub. Restore the byte length, keep the len guard, and drop the invented call from the stub.
A failed hash update was silently ignored, so a corrupt read of a loadable segment could still pass image verification.
Erase was the only ext_flash op without the layout check, so an out-of-range request reached the SDK. Also replace the lone _Static_assert with the tree's typedef char idiom.
Returning -1 for a truncated token contradicted the documented contract (token length, 0 at end of list) and skipped the caller's existing skip path for unmatchable names. Return the truncated length: env_leftkey fails to match it and the slot is skipped.
Only the final read result reached the return value; a failed erase or write was reported as success.
The write loop cast len to uint32_t, so a negative length became a huge iteration count and read past the source buffer; a zero length is a no-op success, matching the erase side.
Truncating the header offset to uint32_t left the upper bits of a 64-bit difference in the arithmetic; compare the header pointer against the vault bounds directly and keep the offset width-correct.
The Check fixture runs once per case, so under CK_FORK=no the rejection tests saw LCR writes from the earlier tests and failed.
The G0 and G4 files carry the same double-word fast path and were fixed in the same commit, but only the G4 copy had a regression test. Clone the G4 harness with the G0 register set (BSY1, 4-bit W1C clear).
wolfSSL-Fenrir-bot
previously requested changes
Sep 24, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #910
Scan targets checked: wolfboot-src, wolfboot-bugs
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.
The direct (unsigned long long) cast of the va_arg pointer warns pointer-to-int-cast on 32-bit targets (void* is 32 bits), which the nxp-t1024 parse-tools build rejects with -Werror. Route the cast through uintptr_t so it is size-exact on every word size.
The version check moved to 'version' in F-13606, leaving 'ret' used only in the EXT_FLASH && NO_XIP branches. EXT_FLASH/XIP configs (e.g. raspi3-encrypted) then fail -Werror=unused-variable.
- hal/nxp_ls1028a.c: ext_flash_erase/write return len on success, not 0, so the F-12067 follow-up guards (ret != 0) always tripped and test_flash could never pass; check ret < 0 - src/x86/qemu_fsp.c: the F-12885 stop-bit guard rejects the in-tree uart_init(115200, 8, 'N', 0) call, leaving the FSP QEMU debug UART unprogrammed; pass 1 stop bit
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #910
Scan targets checked: wolfboot-src, wolfboot-bugs
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
their stale review
September 24, 2026 11:00
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.
815c461 F-11023: add the STM32G0 copy of the fast-path write test
2f73f50 F-12885: reset the LCR counter in the no-fork x86 uart tests
6632776 F-12884: drop the unused variable in the nrf54l write test
7a87f67 F-13607: use pointer comparisons in the store size guards
c74ed95 F-12886: reject non-positive length in HiFive1 flash write
d07d981 F-12067: check erase/write results in test_flash
839a1d2 F-13609: return the truncated length for over-long tokens
7570738 F-6871: bound ext_flash_erase to the device size
c236414 F-13608: check update_hash_flash_addr in ELF hashing
f528cab F-14131: pass the byte length to FLASH_EraseSector
641c9cf F-12871: compare against the absolute bank 2 base
07b573e unit-tests: gitignore the generated extract headers and test binaries
455e535 F-13611: drop the shadowed dec_block in ext_flash_decrypt_read
13448ab F-13609: consume over-long BOOT_ORDER tokens whole in env_next_name
ef122f6 F-13608: check flash read/hash results in ELF image verification
3d988c7 F-12919: log FIT compression from a bounded NUL-terminated copy
c7bf895 F-12886: reject non-positive length in HiFive1 flash erase
f219d09 F-12067: test_flash returns -1 on mismatch, 0 on success
93da376 F-9742: reject cross-bank flash erase on TI Hercules
6da35c8 F-9741: close fd on mmap failure in sim HAL
a5e5d37 F-6872: assert hal_bl_scratch covers the bootloader slot budget
772f640 F-6871: bound ext_flash read/write to the SPI NOR device size
83d1e25 cmake: warn on security-weakening flags like the Makefile (F-6760)
d6464f9 hifive1: mask the HFROSCCFG divider shift with the divider field (F-9743)
87b89a0 ramboot: accept image versions with the high bit set (F-13606)
e6824ae ramboot: require full payload size from ext_flash_read (F-13605)
647022b x86: derive UART stop-bit field from requested stop count (F-12885)
0702a61 F-11048: clamp negative '' field widths in uart_vprintf
d6926a7 F-11030: consume long/size_t/pointer args at full width in uart_vprintf
a2ad3c5 F-13607: bound the whole object header in the store guards
9e9cdbe F-12070: validate the persisted slot position in find_object_buffer
7860fa6 F-12071: early-exit sector scan for sector-zero ranges
66b2205 F-12106: report flash programming errors on STM32U3
5806172 F-12871: split cross-bank flash writes at the bank boundary
519552f F-12872: reject a repeated squashelf -r option
65e3b40 F-12884: flush the nRF54L UART conversion buffer when full
1d1977c F-13654: index the NVM word fast path from the current offset
6025f35 F-13655: index the flash double-word fast path from the current offset
f0e2c49 F-13656: bound e_phentsize to the program-header struct size
d17ea7a F-14129: derive the C0 erase page from the flash-relative offset
5e065c6 F-14130: guard zero-length erase in the kinetis HAL
0c8a9ae F-14131: pass a sector count, not a byte length, to FLASH_EraseSector
6296f24 F-14132: stage1 loader requires the full partition read before the jump
1a8378e F-14133: drop the dead entry_out_set guard in the ELF check
b6f842c F-14138: fix the DTCV base comment (2^13, not 213)
a1bb766 F-14149: compute the ARMORED witness triangular number in 64 bits