Fenrir fixes 2026 07 02#814
Merged
Merged
Conversation
…tore cache_commit() is the single choke point through which every psa_store helper (create_object, delete_object, wolfPSA_Store_Write's sector loop, update_store_size, erase_object_payload, check_vault) flushes the static cached_sector staging buffer to flash. The buffer was never cleared, so key-object plaintext staged there could remain resident in static SRAM after the store operation returned. Wipe cached_sector with wc_ForceZero right after the flash write completes, mirroring the existing pattern in src/wolfhsm_flash_hal.c.
Add tests asserting rejection on a mismatched pubkey hint and on a short digestSz from wolfTPM2_NVReadAuth, so a weakened comparison (e.g. == 0 -> >= 0, or a dropped clause) is caught.
… sata_unlock_disk sata_unlock_disk() calls panic() directly when the post-unlock ATA security state doesn't match the expected SEC5/SEC6, bypassing the cleanup: label that zeroizes the plaintext secret[] stack buffer. On x86, panic() is an infinite hlt() loop, so the plaintext disk-unlock secret (TPM-unsealed key or password) remains readable in DRAM for as long as the device stays halted. Zeroize secret[] before panic() on this path, mirroring the existing cleanup zeroization.
security_command_passphrase() copies the plaintext disk-unlock secret into the file-static DMA buffer `buffer` at ATA_SECURITY_PASSWORD_OFFSET but never wipes it. On the slot<0 early return no ATA command is even dispatched, and on failures of ata_security_set_password()/ ata_security_unlock_device() (which sata_unlock_disk() turns into a panic()) no later IDENTIFY DMA ever overwrites it, so the secret is left resident in BSS for as long as the device stays powered. Zeroize the password field on the slot<0 path and after synchronous command completion. The async path (used only by the currently unreachable ata_security_erase_unit()) is left untouched because the HBA may still be DMAing out of the buffer when exec_cmd_slot_ex() returns ATA_ERR_BUSY; wiping it there would race the transfer.
A matched block's source offset is encoded as off[0..2] right after the ESC (0x7f) header marker. When the match offset falls in [0x7f0000, 0x7fffff], off[0] is 0x7f, so the header begins with ESC ESC -- the same two bytes wb_patch uses to decode an escaped literal 0x7f. The decoder then emits a single literal byte, consumes only 2 of the 6 header bytes, and desyncs the rest of the stream, breaking the wb_patch(wb_diff(A,B)) == B roundtrip for base images >= ~8MB (a supported MMU/Linux delta-update configuration). Make wb_diff skip any candidate match whose offset's most-significant byte equals ESC, in both the forward (base-image) and backward (previously-patched-image) search paths, so the ambiguous header is never produced; the position falls back to literal encoding instead.
…(kinetis.c, mcxa.c, mcxw.c) In the unaligned/partial-word path, address/len were advanced by the full flash-word-relative loop index "i" (which starts at start_off), instead of by the number of data bytes actually consumed (i - start_off). On a write spanning more than one flash word this drops start_off bytes of input data and misdirects the following word write. Mirrors the already-correct form in hal/kinetis_kl26.c.
otp-keystore-gen.c reads the device root UDS into the stack buffer `uds` and copies it into the heap buffer `otp_buf` at OTP_UDS_OFFSET, then on every exit path calls free(otp_buf) without wiping it first, and never clears `uds`. Both copies of the highest-value device secret remain in the host process's freed heap chunk and stack frame. Add a local secure_zero() helper (no wolfSSL dependency, matching this standalone host tool's existing bare-gcc build) and call it on the success path and on the write-failure/short-UDS-read error paths, before free()/exit(), mirroring the zeroize-before-release pattern already used elsewhere in this tree (src/x86/ata.c, src/x86/ahci.c). Paths that exit before `uds` is populated are left untouched since there is no secret to wipe yet.
test_hwswap_highversion_rollback_denied was the only test for update_flash_hwswap.c's wolfBoot_start and always ends in boot_panic, so the entire successful-boot path (IMG_STATE_UPDATING->TESTING transitions before and after the HW dualbank swap, and the same-version boundary of the anti-rollback check) was never exercised and mutations there would go undetected. Add three tests that reach do_boot() and pin each of those code paths.
FPGA_NONFATAL downgrades a failed PL bitstream load from fatal (panic) to a logged non-fatal warning, letting boot continue without the programmable logic. Every other fail-safe-weakening option in this file (ALLOW_DOWNGRADE, DISABLE_BACKUP, WOLFBOOT_UDS_UID_FALLBACK_FORTEST) emits a $(warning ...) so the tradeoff is visible at build time; FPGA_NONFATAL was missing one.
hal_flash_write() and hal_flash_erase() aligned the invalidation start address down to a 32-byte cache line but rounded the length up from "len" alone, omitting the (address - aligned_address) offset. Whenever (address % 32) + (len % 32) > 32, the invalidated range fell short of address + len, leaving the last cache line stale after a write/erase. Extract the range computation into hal_flash_cache_align_range() (hal/imx_rt.h, dependency-free so it's unit-testable without the NXP SDK) and include the down-alignment offset before rounding the length up, so the invalidated range always covers [address, address+len).
…source size bmpatch mmaps `base` sized to the source file (len1) then memcpy's the reconstructed output into base+len3, which runs past the mapping once the destination image is larger than the source (a common case for delta updates). This causes writes into the zero-fill slack of the last mapped page to be silently dropped (never synced to the file even after the trailing ftruncate) or, for larger overruns, a SIGBUS. Reproduced with the existing `delta-test` fixtures: patching 1.txt back to the (larger) 0.txt already corrupts the tail of the output today. Write reconstructed blocks with pwrite(fd1, ...) instead of memcpy into the undersized mapping; regular file writes grow the file naturally instead of faulting/silently discarding data past the mapped region. The mmap of `base` is only used for reads, so this is a minimal, in-place-preserving fix. Verified `make -C tools/delta delta-test` now passes both directions (shrinking 0->1 and growing 1->0).
… final cleanup update_disk.c wolfBoot_start() already zeroizes disk_encrypt_key/nonce and the decrypted header before most wolfBoot_panic() halts, but four paths were missed: anti-rollback rejection, FIT FPGA load failure, FIT kernel load failure, and flash-protect failure. Since wolfBoot_panic() halts forever on real targets, these paths left the key live in BSS on a halted device. Add the same disk_decrypted_header_clear()/disk_crypto_clear() pair used on the other panic paths immediately before each.
arg2num() parsed --custom-tlv values with signed strtoll(), which saturates to LLONG_MAX (0x7FFFFFFFFFFFFFFF) on positive overflow. For LEN==8 no masking is applied afterwards (unlike LEN 1/2/4), so any value >= 2^63 silently encoded as 0x7FFFFFFFFFFFFFFF instead of the value the user supplied, breaking the TLV encode/decode roundtrip. Switch to strtoull() and reject (exit 16) when it reports ERANGE for an 8-byte value, mirroring the existing fw_version range check.
…mage_elf The scattered-ELF flash integrity check's final image_CT_compare() rejection branch had zero unit test coverage, so a weakened comparison (e.g. != 0 -> == 0, or a dropped return -2) would silently pass. Add a positive case (correctly-hashed scattered image verifies OK) and a negative case (single corrupted byte in a scattered segment's flash-resident payload is rejected via the digest-mismatch branch).
SIGN=NONE disables firmware signature authenticity verification entirely (wolfBoot_verify_authenticity() becomes a stub that always confirms), leaving only a hash check an attacker can satisfy. Every other fail-safe-weakening option in this file (ALLOW_DOWNGRADE, DISABLE_BACKUP, WOLFBOOT_UDS_UID_FALLBACK_FORTEST, FPGA_NONFATAL) emits a $(warning ...) so the tradeoff is visible at build time; SIGN=NONE was missing one despite being the most security-critical.
…amr21/same51) In the else branch of hal_flash_write, off was computed once per call from the original (call-time) "address" instead of the current position "address + i". The word index dst_idx advanced with i, but the fill loop kept starting at the stale off, so once destination address and source buffer had different alignment mod 4, every word after the first was filled at the wrong byte offset, dropping and misplacing data. Derive off and dst from address + i so each word's offset tracks the current position, mirroring the fix already applied to mcxa.c for the same bug class (F-5963).
…ess/open_self_address The fw_size > (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE) guard was only tested on the reject side (oversize + 1); no test asserted that a firmware payload exactly filling the partition payload budget is accepted. A >/>= mutation at image.c:1402 or image.c:1618 would silently reject a maximally-sized valid image and survive the existing suite. Add positive boundary assertions (via wolfBoot_open_image()/wolfBoot_open_image_address() and wolfBoot_open_self_address()) paired with the existing reject-side checks in test_open_image.
…h_write (QSPI) spi_flash_read only rejected address > FLASH_DEVICE_SIZE (off-by-one, admits address == FLASH_DEVICE_SIZE) and never validated address+len against the device size, so an in-bounds start with an out-of-range extent was not rejected. spi_flash_write had no bounds check at all. Add an inclusive/full-extent check to both.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a batch of correctness + security-hardening fixes across wolfBoot’s update/flash/delta/signing paths, along with targeted regression and mutation-pinning unit tests to prevent reintroducing the reported issues.
Changes:
- Hardened bounds/offset handling in flash/QSPI/delta patching code paths to prevent OOB and under-coverage cases.
- Added/extended secret zeroization on error/panic paths (disk encryption material, ATA unlock secrets, PSA store cache sector, OTP UDS buffers).
- Added multiple new host unit tests to pin edge cases and reported regressions (HW-swap boot transitions, scattered-ELF hashing integrity, cache-line alignment, unaligned flash writes, delta ESC collision, etc.).
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-update-flash-hwswap.c | Adds successful-boot coverage and helpers for HW-assisted swap state transitions. |
| tools/unit-tests/unit-update-disk.c | Verifies disk encryption key/nonce are cleared on panic paths. |
| tools/unit-tests/unit-tpm-rsa-exp.c | Adds negative tests for RoT digest mismatch/size handling. |
| tools/unit-tests/unit-sign-custom-tlv-le.py | Adds regression cases for 8-byte custom TLV parsing saturation and refactors signing checks. |
| tools/unit-tests/unit-qspi-flash.c | Adds tests for QSPI read/write bounds rejection. |
| tools/unit-tests/unit-psa_store.c | Adds test ensuring cached_sector is zeroized after commit. |
| tools/unit-tests/unit-otp-keystore-gen-zeroize.c | New unit test interposing alloc/free/read to assert OTP UDS zeroization before free/exit. |
| tools/unit-tests/unit-imx-rt-cache-align.c | New test for i.MX RT cache invalidation alignment coverage correctness. |
| tools/unit-tests/unit-image.c | Pins partition-fit boundary acceptance for open_image/open_self boundary conditions. |
| tools/unit-tests/unit-image-elf-scatter.c | New mutation-pinning tests for scattered-ELF hash verification and corruption detection. |
| tools/unit-tests/unit-flash-write-samr21.c | New regression test for byte-wise unaligned write offset bug (samr21). |
| tools/unit-tests/unit-flash-write-same51.c | New regression test for byte-wise unaligned write offset bug (same51). |
| tools/unit-tests/unit-flash-write-mcxa.c | New regression test for unaligned write bookkeeping bug (mcxa). |
| tools/unit-tests/unit-delta.c | Adds coverage for ESC-colliding match-offset MSB behavior. |
| tools/unit-tests/unit-ata-security-passphrase-zeroize.c | New tests ensuring ATA command buffer passphrase is wiped on success/error. |
| tools/unit-tests/unit-ahci-unlock-panic.c | New test ensuring AHCI unlock stack secret is wiped before state-mismatch panic. |
| tools/unit-tests/mcxa_fsl_stub/fsl_spc.h | Adds stub header for MCUXpresso dependency-free unit tests. |
| tools/unit-tests/mcxa_fsl_stub/fsl_romapi.h | Adds stub ROM API header used by hal/mcxa.c unit test. |
| tools/unit-tests/mcxa_fsl_stub/fsl_common.h | Adds stub common types for MCUXpresso headers in tests. |
| tools/unit-tests/mcxa_fsl_stub/fsl_clock.h | Adds stub header for MCUXpresso dependency-free unit tests. |
| tools/unit-tests/Makefile | Registers new unit tests and builds; adds CFLAGS for scattered-ELF test. |
| tools/keytools/sign.c | Fixes custom TLV numeric parsing to use unsigned conversion and detect 8-byte ERANGE. |
| tools/keytools/otp/otp-keystore-gen.c | Adds secure_zero() and wipes UDS in heap+stack on all exit paths. |
| tools/delta/bmdiff.c | Fixes bmpatch OOB write by writing output via pwrite() instead of mmapped memcpy(). |
| src/x86/ata.c | Adds secure zeroization of passphrase bytes in static ATA command buffer (sync paths + early error). |
| src/x86/ahci.c | Zeroizes unlock secret before panic on post-unlock state mismatch. |
| src/update_disk.c | Clears decrypted header + crypto material before panic in several error paths. |
| src/qspi_flash.c | Enforces full-transfer bounds checks in spi_flash_read/write. |
| src/psa_store.c | Zeroizes cached_sector after flash commit to avoid retaining plaintext in RAM. |
| src/delta.c | Skips wb_diff match candidates whose offset MSB collides with ESC escape marker. |
| options.mk | Emits build-time warnings for SIGN=NONE and FPGA_NONFATAL=1 insecure/non-fatal configs. |
| hal/samr21.c | Fixes stale in-word offset in byte-wise flash write path; gates cpsid for host unit tests. |
| hal/same51.c | Fixes stale in-word offset in byte-wise flash write path; gates cpsid for host unit tests. |
| hal/mcxw.c | Fixes unaligned write bookkeeping (address/len over-advance). |
| hal/mcxa.c | Fixes unaligned write bookkeeping (address/len over-advance). |
| hal/kinetis.c | Fixes unaligned write bookkeeping (address/len over-advance). |
| hal/imx_rt.h | New helper to compute cache-line-aligned invalidate ranges (SDK-free). |
| hal/imx_rt.c | Uses hal_flash_cache_align_range() to correctly include down-alignment offset in invalidate length. |
| .gitignore | Ignores newly added unit-test binaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The bulk FLASH_Program path read data+w but never advanced w, so a partial-word tail after an aligned run re-read the input from offset 0. Affects kinetis, mcxa, mcxw. Pin with an mcxa bulk+tail test case.
empty_qword[0] was 0xFFFFFFF (7 nibbles), so a fully-erased 16-byte word never compared equal and was always reprogrammed.
Detect short writes and errors instead of advancing the output offset as if all bytes were written, which could silently corrupt the image.
src/keystore.c is generated and gitignored, so the test failed to build on a clean tree. Use the checked-in unit-keystore.c stub instead.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #814
Scan targets checked: wolfboot-bugs, wolfboot-src
No new issues found in the changed files. ✅
dgarske
approved these changes
Jul 2, 2026
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.
c4e853e F-6131: enforce full-transfer bounds check in spi_flash_read/spi_flash_write (QSPI)
dd16ccb F-6405: pin partition-fit accept boundary in wolfBoot_open_image_address/open_self_address
102ddc3 F-5964: fix stale in-word offset in hal_flash_write byte-wise path (samr21/same51)
eb5574f F-6124: emit build-time warning for SIGN=NONE / WOLFBOOT_NO_SIGN
72186fa F-6126: add mutation-pinning test coverage for wolfBoot_check_flash_image_elf
9a25fae F-6127: fix custom-TLV 8-byte value saturation in arg2num
dd0712e F-6130: clear disk_encrypt_key/nonce before panic paths that skip the final cleanup
e38296c F-6398: fix OOB write in bmpatch when reconstructed image grows past source size
32e41f0 F-6399: fix imx_rt DCACHE invalidation to include down-alignment offset
e09074a F-6401: emit build-time warning for FPGA_NONFATAL=1
f20a1e1 F-6403: add successful-boot test coverage for HW-swap wolfBoot_start
8df9689 F-6408: zeroize UDS from OTP keystore generator's heap and stack buffers
fea97c5 F-5963: fix over-advance of address/len in unaligned hal_flash_write (kinetis.c, mcxa.c, mcxw.c)
9ce750f F-5965: skip wb_diff match candidates whose offset MSB collides with ESC
cd4e4be F-5968: zeroize disk-unlock passphrase from static ATA command buffer
20f11ae F-6129: zeroize disk-unlock secret before the state-mismatch panic in sata_unlock_disk
60acdc2 F-6402: add negative tests pinning wolfBoot_check_rot digest comparison
228c9d6 F-6407: zeroize static cached_sector after each flash commit in psa_store