Fenrir fixes 2026-08 -11 + build regressions fixes - #851
Conversation
When a read starts in the middle of an encryption block, the head copy size was computed as ENCRYPT_BLOCK_SIZE - row_offset without regard for the requested length. A read shorter than the remainder of the block (e.g. 1 byte at offset 1) copied up to 15 decrypted bytes into a buffer sized for fewer, and left read_remaining negative, so the subsequent flash_read_size = read_remaining & ~(ENCRYPT_BLOCK_SIZE - 1) was passed to ext_flash_read() as a negative length. Clamp the head size to the bytes actually requested. Add a unit test covering short unaligned reads (offsets 1/4/8/15) that checks the return value, the decrypted contents and that no byte past the requested length is written; the ext_flash_read() mock now also rejects negative lengths.
When a write starts in the middle of an encryption block, or is shorter than a full block, the head copy size was computed as ENCRYPT_BLOCK_SIZE - row_offset without regard for the requested length. A write shorter than the remainder of the block (e.g. 1 byte at offset 1) copied up to ENCRYPT_BLOCK_SIZE-1 bytes out of the caller's buffer into the read-modify-write block, and left sz negative, so the subsequent step = sz & ~(ENCRYPT_BLOCK_SIZE - 1) was passed to ext_flash_write() as a negative length. This is reachable from wb_flash_write_verify_word() (4-byte writes), from wolfBoot_nsc_write_update() and from the delta patch writer. Clamp the head size to the bytes actually requested, and return the result of the head block write when the request fits within that block. Add a unit test covering short unaligned writes (offsets 1/0/8/blk-1) that checks the return value, the data read back and that the bytes past the requested length were not taken from the caller's buffer; the ext_flash_write() mock now also rejects negative lengths.
wolfBoot_delta_update() compared the boot partition digest against the delta base hash using base_hash_sz, the length returned by wolfBoot_find_header() for the boot header's hash TLV, without ever checking it. When the tag is absent, find_header() sets base_hash to NULL and returns 0, so wolfBoot_hardened_CT_compare(NULL, ..., 0) compared zero bytes and reported a match: the base image digest gate silently succeeded instead of rejecting the patch. A short or oversized TLV length would likewise truncate the comparison or read past the delta base hash in the update header. The gate is reachable because wolfBoot_update() runs before the boot partition is verified, so the boot header contents are not guaranteed to carry a well-formed digest TLV at that point. Reject the patch when the base image has no usable digest, and compare a fixed WOLFBOOT_SHA_DIGEST_SIZE. The inverse and resume paths are unaffected, as they do not use this gate. Add unit-update-flash-delta coverage for a boot header without a digest TLV.
make_header_ex() validated the delta base image digest with direct exit(1) calls. Those are reachable in normal use: base_diff() looks up the base digest for the selected hash algorithm, and when the base image was signed with a different algorithm the lookup yields NULL, yet make_header_delta() is still called. Aborting there skips base_diff()'s cleanup (the temporary patch file is left in /tmp) and, more importantly, main()'s zero_and_free(kbuf, key_buffer_sz) and algorithm-specific key free, so the raw and decoded private signing key are never scrubbed. Use the function's existing 'failure:' path instead, which returns -1 and propagates through base_diff() to main()'s unified cleanup. Reaching 'failure:' from there uncovered a latent double fclose(): the image-size probe closes 'f' without clearing it, so the cleanup block closed the same stream again. Clear the pointer after the fclose(). Add unit-sign-delta-basehash-cleanup.py, which signs a SHA256 base image, requests a SHA384 delta against it, and asserts the run fails with the temporary patch file removed.
The sign tool kept a single file-static struct for the decoded private key, so a hybrid run that picks two algorithms sharing one member (e.g. ECC521 primary + ECC256 secondary, or RSA2048 + RSAPSS2048) had the secondary load_key() re-init and overwrite the still-live primary key before either signature was produced. The primary signature was then made with the secondary key, and the final cleanup in main() dispatched only on CMD.sign, so the secondary key never reached its algorithm specific zeroizing free. Give the primary and the secondary signer their own storage, select it with key_obj(secondary) in load_key()/load_key_ecc()/load_key_rsa()/ sign_digest()/set_signature_sizes(), and free both keys at exit through the new free_key() helper.
ext_flash_encrypt_write() encrypted the whole caller-supplied buffer into ENCRYPT_CACHE, which is only NVM_CACHE_SIZE bytes, without any check that the request fits. A request longer than the cache (reachable from the non-secure world through wolfBoot_nsc_write_update(), which only bounds len against the partition size) overran the staging buffer and made ext_flash_write() read past its end. Stage and flush the ciphertext in NVM_CACHE_SIZE chunks instead. The encryption stream is not restarted between chunks, so the resulting flash content is unchanged for requests that already fitted.
wolfBoot_start() parsed both ih_load and ih_ep from the U-Boot legacy uImage header, relocated the payload to ih_load, then discarded ih_ep and passed the load address to do_boot(). An image built with the entry point ahead of the load address (a preamble before the entry, as U-Boot bootm handles by copying to ih_load and jumping to ih_ep) was staged correctly but entered at the wrong address. Keep ih_load as the relocation destination and remember ih_ep as the entry point when the two differ, then override load_address just before do_boot(). The override is skipped when a later stage (ELF/FIT) re-derived the load address, since that stage supplies its own entry point. Extend unit-update-ram-uboot with a case where ih_ep = ih_load + 0x40: it asserts the payload lands at ih_load and do_boot() is entered at ih_ep. Fails before this change (jumps to ih_load).
With WOLFTPM_ADV_IO the TIS layer hands the raw command payload to the HAL callback, so TPM2_IoCb() stages it in stack-local txBuf/rxBuf. Both were left intact on the normal return and on the wait-state error return, keeping a TPM command's plaintext authValue (and the response bytes) in bootloader stack SRAM. Wipe them like TPM2_TIS_Read()/TPM2_TIS_Write() already do for their own staging buffers in the non-advanced-IO path. Adds unit-tpm-advio-zeroize, which drives TPM2_IoCb() through the write, read, payload-error and wait-state-timeout paths with a mock SPI slave and inspects the staging buffers afterwards.
wolfBoot_copy_sector() discarded the return value of every flash operation it performed and unconditionally returned the number of bytes processed. Callers therefore treated a partially written sector as a completed one and advanced the persistent sector flags, which are the only record used to resume an interrupted swap. A write error while copying BOOT into UPDATE (the backup step) could leave both the running image and its backup corrupted with no way to redo the sector. Check the result of every erase/read/write in wolfBoot_copy_sector() and return -1 on the first failure. In the interruptible swap loop, the delta loop and the DISABLE_BACKUP direct copy, stop on a negative return without advancing the sector flag or confirming the boot partition, so the swap is retried from the last completed step on the next boot.
hal_flash_erase() in hal/mcxw.c rounded the start address down with the runtime pflash_sector_size (queried from FLASH_GetProperty() in hal_init()) but stepped address and len by the compile-time WOLFBOOT_SECTOR_SIZE. When the two differ, a larger WOLFBOOT_SECTOR_SIZE steps over hardware sectors inside the requested range and leaves them unerased, while a smaller one issues erase commands at non-sector-aligned addresses. A zero size reported by the driver would also divide by zero. Take a local sector_size, fall back to WOLFBOOT_SECTOR_SIZE when the driver reports zero and use it for the alignment and both loop steps, as hal/mcxn.c already does. Add unit-flash-erase-mcxw, using the existing WOLFBOOT_UNIT_TEST_FLASH_ERASE guard convention to compile hal_flash_erase() in isolation without the NXP MCUXpresso SDK headers.
spi_flash_write() chunked purely by length, issuing up to a full FLASH_PAGE_SIZE page program at address + page*FLASH_PAGE_SIZE. NOR flash page program wraps within the device's own page, so a transfer starting mid-page (e.g. 0x10F0 with 256 bytes) programmed the tail of the page and then wrapped the rest back over the start of the same page, corrupting already-programmed data and leaving the intended range unwritten. Drive the loop from the running address and clip each transfer to the bytes remaining in the current page, matching src/spi_flash.c.
…mestamp wolfBoot_tpm2_get_timestamp() derives (or copies) the endorsement-hierarchy authValue into the stack-local eh_handle before issuing TPM2_GetTime. The wolfTPM2_UnsetAuth() calls on the way out only clear the copies wolfTPM keeps in the device session slots, and the existing TPM2_ForceZero() only clears the reel master secret, so the derived per-device authValue was left resident in the Secure stack frame after the non-secure entry veneer returned. Wipe eh_handle before returning, matching the scrubbing already done for the master secret. Add unit-tpm-mfgid-eh-zeroize, which captures the handle passed to wolfTPM2_SetIdentityAuth() and snapshots the dead frame on both the success and TPM2_GetTime-error paths.
The byte-wise branch of hal_flash_write() derived the containing word from
the call-time "address" instead of the current position "address + i":
int off = (address + i) - (((address + i) >> 2) << 2);
dst = (uint32_t *)(address - off);
val = dst[i >> 2];
so "dst[i >> 2]" addressed physical byte "address - off + (i & ~3)". Any
iteration with "i" not a multiple of 4 modified the wrong byte, and with
off != 0 it did so through a misaligned 32-bit flash access (a HardFault on
the Cortex-M0+ of stm32l0). A word-aligned 6-byte write, for instance, put
data[5] at "address + 4" and left "address + 5" erased.
Use the form already applied to hal/samr21.c and hal/same51.c: base the
word on "address + i - off", read it with a single aligned access, and fill
it byte by byte up to the next word boundary.
Add unit-flash-write-nrf52, covering the aligned-with-tail, mismatched
alignment and single-word cases against hal/nrf52.c.
dd0712e added the disk_decrypted_header_clear()/disk_crypto_clear() pair to the wolfBoot_start() panic paths that were missing it, but the FIT flat-device-tree load failure was one more: when wolfBoot_fit_memcpy() fails to relocate the DTS, wolfBoot_panic() is entered with disk_encrypt_key/disk_encrypt_nonce still live in BSS, and that call never returns on a real target. Add unit-update-disk-fit, which drives wolfBoot_start() through the FIT branch with DISK_ENCRYPT enabled and snapshots the module statics from the WOLFBOOT_HOOK_PANIC hook.
ForceZero() is only visible in libwolfboot.c, which pulls in misc.c inline; update_disk.c called it without a declaration, so any config with disk encryption failed to build. Use the exported wc_ForceZero() from memory.o instead, which is always linked. Add a zynqmp_sdcard ENCRYPT build job to CI, the only one that compiles these paths.
Under WOLFSSL_ARMASM, chacha.c calls wc_chacha_crypt_bytes(), which arch.mk never adds for AArch64 -- it only pulls in the aes/sha ports. Any AArch64 build using ChaCha failed to link. Add the object in options.mk, where ChaCha is selected. Also add a ChaCha variant of the zynqmp_sdcard ENCRYPT build to CI.
There was a problem hiding this comment.
Pull request overview
This PR bundles a set of boot/update security and correctness regressions fixes across wolfBoot’s update flows (RAM/flash/disk), TPM handling, signing tooling, and related HAL drivers, and backs them with expanded unit-test coverage and CI build matrix tweaks.
Changes:
- Fix/extend boot and update correctness (uImage ih_ep handling in RAM boot, abort swap on sector copy failures, QSPI page-boundary clipping, MCXW erase stride consistency, partial-word flash-write fixes).
- Harden key/material handling (disk decrypt key/nonce zeroization on FIT failure paths, TPM stack buffer/handle wiping, signing tool cleanup paths & hybrid key separation).
- Add regression tests and CI coverage for the above behaviors (new unit tests + Makefile/workflow updates).
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-update-ram-uboot.c | Adds uImage ih_ep≠ih_load test coverage. |
| tools/unit-tests/unit-update-flash.c | Adds tests for aborting swap on copy failure and delta base-hash rejection when boot lacks digest TLV. |
| tools/unit-tests/unit-update-disk.c | Updates test stub to wc_ForceZero naming. |
| tools/unit-tests/unit-update-disk-fit.c | New tests ensuring disk decrypt key/nonce scrub on FIT DTS load failure paths. |
| tools/unit-tests/unit-tpm-mfgid-eh-zeroize.c | New regression test ensuring EH authValue doesn’t persist on stack. |
| tools/unit-tests/unit-tpm-advio-zeroize.c | New regression test ensuring TPM ADV_IO staging buffers are wiped on all exits. |
| tools/unit-tests/unit-sign-hybrid-keyload.c | Adds test ensuring secondary hybrid key load doesn’t clobber primary decoded key. |
| tools/unit-tests/unit-sign-delta-basehash-cleanup.py | New integration-style test ensuring sign tool unwinds/cleans up on base-hash validation failure. |
| tools/unit-tests/unit-qspi-flash.c | Adds test for clipping first page program at boundary. |
| tools/unit-tests/unit-flash-write-nrf52.c | New regression tests for partial-word flash writes (nrf52). |
| tools/unit-tests/unit-flash-erase-mcxw.c | New unit tests for consistent sector-size stride in mcxw erase. |
| tools/unit-tests/unit-extflash.c | Adds negative-length assertions and new unaligned/oversized encrypted extflash read/write tests. |
| tools/unit-tests/Makefile | Wires new unit tests and python test into build/run targets. |
| tools/keytools/sign.c | Separates primary/secondary decoded key storage; ensures base-hash failures unwind; centralizes key free/zeroize paths. |
| src/update_ram.c | Enters legacy uImage at ih_ep when it differs from ih_load (when applicable). |
| src/update_flash.c | Propagates flash read/write/erase failures up to abort swaps and avoid advancing sector flags incorrectly. |
| src/update_disk.c | Uses wc_ForceZero and scrubs disk key/nonce on FIT DTS load failure before panic. |
| src/tpm.c | Wipes ADV_IO staging buffers and clears derived EH authValue from stack on return. |
| src/qspi_flash.c | Clips page program transfers at device page boundaries. |
| src/libwolfboot.c | Bounds/stages encrypted extflash writes and clamps unaligned head copies; clamps unaligned decrypt head size. |
| options.mk | Links AArch64 ARM ChaCha port object when ChaCha is selected. |
| hal/stm32l0.c | Fixes partial-word flash write addressing/word assembly. |
| hal/nrf5340.c | Fixes partial-word flash write addressing/word assembly. |
| hal/nrf52.c | Fixes partial-word flash write addressing/word assembly. |
| hal/mcxw.c | Uses a consistent runtime sector size (with zero fallback) for erase alignment/stride. |
| .gitignore | Ignores new unit-test binaries. |
| .github/workflows/test-configs.yml | Adds encrypted disk-loader build coverage (AES/ChaCha) for zynqmp_sdcard config. |
Suppressed comments (1)
src/update_flash.c:336
- ext_flash_check_read() (ext_flash_read/ext_flash_decrypt_read) also returns a byte count on success. Only checking for <0 can accept short reads and continue the swap with incomplete data. Require the requested size here so a partial read aborts the sector copy.
if (ext_flash_check_read((uintptr_t)(src->hdr) +
src_sector_offset + pos,
(void *)buffer, FLASHBUFFER_SIZE) < 0) {
ret = -1;
goto out;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The encrypted key handling calls ForceZero(), but misc.c is only included inline under __WOLFBOOT or UNIT_TEST. The test-app build of libwolfboot.c defines neither, so on MMU targets with EXT_ENCRYPTED the call had no declaration. GCC 14 rejects that; older compilers only warned. Add EXT_ENCRYPTED to the guard, which keeps ForceZero() static and adds no link dependency.
The error handling added by the fixes in this branch costs 40 bytes of common code, so every stm32f407-discovery configuration grew by that amount. Raise each limit by 40, keeping the previous headroom.
ext_flash_encrypt_write() writes whole ENCRYPT_BLOCK_SIZE blocks:
- A length that is not a multiple of the block size dropped the trailing
bytes, since the remainder loop rounds down. Merge them into the block
that already backs them, as the unaligned head is handled.
- len == 0 fell through to a read-modify-write of the containing block,
re-encrypting it in place. Return early instead.
wolfBoot_copy_sector() checked the reads added in F-7987 for a negative
return, but ext_flash_read() and ext_flash_check_read() return the number
of bytes read (docs/HAL.md), so a short read was accepted and a partially
filled buffer copied on. Require the full FLASHBUFFER_SIZE.
Add unit-update-flash-enc coverage for the two write cases.
42a428b to
fca6bf0
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 19 total — 12 posted, 7 skipped
10 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [High] ext_flash_encrypt_write() discards the head-block write failure, defeating the F-7987 swap-abort it is paired with —
src/libwolfboot.c:2638-2648 - [Medium] *The load_address == uboot_load identity guard is defeated by elf_load_image_mmu() publishing pentry before it validates —
src/update_ram.c:665-670 - [Medium] disk_crypto_clear() leaves the expanded key schedule and nonce live, so the new scrub does not actually destroy the key —
src/update_disk.c:237-241 - [Medium] main() still exit(1)s on secondary-key load failure, skipping the very key scrubbing F-8006 restores —
tools/keytools/sign.c:3825-3828 - [Medium] nrf52/nrf5340/stm32l0: the sibling 32-bit fast path still indexes off the original address and is left broken by the F-6757 fix —
hal/nrf52.c:75-82 - [Low] ext_flash_encrypt_write() chunk size is not rounded down to an ENCRYPT_BLOCK_SIZE multiple —
src/libwolfboot.c:2650-2665 - [Low] mcxw hal_flash_erase() does not extend len by the round-down amount, leaving the last sector unerased —
hal/mcxw.c:232-247 - [Low] ih_ep is still discarded when ih_load == 0, and the TODO that documented that gap was removed —
src/update_ram.c:513-527 - [Low] free_key() can run on a never-initialized key2 object —
tools/keytools/sign.c:3859-3862 - [Low] Two new unit-test binaries are missing from .gitignore —
.gitignore:207-227
Findings not tied to a diff line
F-8006's double-fclose fix is incomplete: three more fclose(f) sites in make_header_ex still leave f dangling
File: tools/keytools/sign.c:1779,1855,1929
Function: make_header_ex
Severity: Medium
The f = NULL; added at line 1515 is correct and necessary - without it the four new goto failure sites in the delta base-hash block would double-fclose at the cleanup label. But the identical pattern remains at three other sites in the same function: line 1779 (SHA256 image-hash loop), line 1855 (SHA384) and line 1929 (SHA3-384) all call fclose(f) without clearing f. Between those points and the next reassignment of f (line 1951 or 1988) several goto failure sites are reachable: line 1968 (signature malloc failure), line 1982 (primary sign_digest failure), lines 2008/2015 (secondary sign_digest failure). The cleanup block at line 2347 then does if (f) fclose(f); on the stale pointer. Reproduced on the PR build under valgrind: ./sign --rsapss2048 --sha3 image.bin rsa2048.der 7 (sign_digest returns -1 for RSA-PSS + SHA3, then goto failure) gives Invalid read of size 4 ... fclose ... Block was alloc'd at ... fopen ... make_header_ex. This is pre-existing on master, but it is exactly the defect class F-8006 set out to remove, a few lines away in the same function.
Recommendation: Add f = NULL; after the fclose(f) calls at lines 1779, 1855 and 1929. That makes if (f) fclose(f); in the cleanup block correct for every goto failure path in the function, matching what the rest of the function already does.
Referenced code: tools/keytools/sign.c:1779,1855,1929 (8 lines)
PART_SANITY_CHECK() panics three lines above the DISK_ENCRYPT scrub, leaving the last terminal exit uncovered
File: src/update_disk.c:687-692
Function: wolfBoot_start
Severity: Low
Auditing the terminal exits reached by the F-6130 change: every wolfBoot_panic() after the key is populated is now covered (lines 300, 307, 318, 332, 371, 415, 513, 563, 598, 613, 638 (new) and 675), and the FSP "doesn't fit in low memory" break falls into the if (failures) scrub. The one remaining hole is PART_SANITY_CHECK(&os_image) at line 687, which expands to if (hdr_ok != 1 || sha_ok != 1 || signature_ok != 1) wolfBoot_panic(); (include/image.h:1692) or to the ARMORED bne . spin (include/image.h:278). It sits immediately above the success-path scrub at lines 690-692, so a sanity-check failure hangs forever with disk_encrypt_key, disk_encrypt_nonce and dec_hdr still live in RAM - the exact exposure this PR closes on the DTS path. wolfBoot_hook_boot(&os_image) at line 684 has the same property if a project hook does not return. (dec_hdr is correctly in scope and populated at the new cleanup site.)
Recommendation: Move the #ifdef DISK_ENCRYPT scrub block above wolfBoot_hook_boot() and PART_SANITY_CHECK(). The key material is no longer needed once the payload is decrypted, so scrubbing earlier costs nothing and closes the last terminal exit.
Referenced code: src/update_disk.c:687-693 (7 lines)
Skipped findings
- [Low]
New trailing partial-block RMW programs up to ENCRYPT_BLOCK_SIZE-1 bytes past the requested range - [Low]
F-7987's abort-on-failure is a no-op for internal-flash HALs, including three the same PR touches - [Low]
The delta loop's non-encrypted wb_flash_write() return is still ignored while everything around it now checks - [Low]
unit-flash-write-nrf52 build rule omits -Wno-int-to-pointer-cast, unlike its sibling rules - [Info]
Delta base-hash validation failure now exits with status 255 instead of 1 - [Info]
TPM2_IoCb wipes the full staging buffers on every TIS transaction instead of just the transferred bytes - [Info]
Blanket +40 byte size-limit bump across every algorithm with no explanation
Review generated by Skoll
| XMEMCPY(block + row_offset, data, step); | ||
| crypto_encrypt(enc_block, block, ENCRYPT_BLOCK_SIZE); | ||
| ext_flash_write(row_address, enc_block, ENCRYPT_BLOCK_SIZE); | ||
| ret = ext_flash_write(row_address, enc_block, ENCRYPT_BLOCK_SIZE); |
There was a problem hiding this comment.
🔴 [High] ext_flash_encrypt_write() discards the head-block write failure, defeating the F-7987 swap-abort it is paired with
The PR now captures the head block's write result (ret = ext_flash_write(row_address, enc_block, ENCRYPT_BLOCK_SIZE);) but only returns it when the whole request fit inside that block (if (step == len) return ret;). When the request extends past the head block, ret is unconditionally overwritten by ret = 0; ten lines later, so a failure programming the first (unaligned) block is silently swallowed and the function can still return 0. This matters much more after this PR than before it: F-7987 rewires wolfBoot_copy_sector() to abort the swap on wb_flash_write(...) < 0, and wb_flash_write() on an external encrypted partition is ext_flash_encrypt_write(). A failed head-block program therefore still reports success, wolfBoot_copy_sector() returns pos, and wolfBoot_update() advances the persistent sector flag over a partially written sector - exactly the corrupted-backup scenario F-7987 sets out to prevent. The same call is error-checked in the new chunk loop (if (ret < 0) return ret;), so this is an internal inconsistency, not a deliberate policy.
Fix: Add if (ret < 0) return ret; immediately after the head-block ext_flash_write(), matching the check already added to the chunk loop below. Extend test_encrypt_write_keeps_trailing_partial_block (or add a sibling) that forces the head-block write to fail and asserts a negative return.
| #endif /* MMU */ | ||
|
|
||
| #ifdef WOLFBOOT_UBOOT_LEGACY | ||
| /* Enter the uImage at ih_ep. Skipped if a later stage (ELF/FIT) re-derived |
There was a problem hiding this comment.
*🟠 [Medium] The load_address == uboot_load identity guard is defeated by elf_load_image_mmu() publishing pentry before it validates
The new override comment states it is "Skipped if a later stage (ELF/FIT) re-derived the load address, since that stage supplies its own entry point." That inference relies on pointer identity, which is not a reliable proxy. src/elf.c:111 does *pentry = GET_H64(entry); before the program-header bounds check and the segment loop, so the -3 (ph table out of bounds), -4 (segment offset/size out of bounds) and -5 (segment destination out of range) returns all leave load_address overwritten with the ELF's declared entry. update_ram.c then prints "Invalid elf, falling back to raw binary" and continues - but load_address != uboot_load, so the new ih_ep override is silently skipped and wolfBoot enters at the ELF-declared address rather than at either ih_load or ih_ep. The converse also exists: if a later stage legitimately re-derives an address numerically equal to ih_load (an ELF whose entry == ih_load, or a FIT kernel sub-image loaded at ih_load), the identity test passes and the override wrongly redirects to ih_ep. Verified: both do_boot() call sites and the ZynqMP BL31 handoff are after the override, so those are consistent.
Fix: Track "a later stage took over the entry point" explicitly with a flag set inside the ELF/FIT success branches, instead of comparing load_address to uboot_load. Separately, elf_load_image_mmu() should not publish *pentry until it has passed all of its validation.
| @@ -235,13 +236,13 @@ static int decrypt_header(const uint8_t *src, uint8_t *dst) | |||
|
|
|||
| static void disk_crypto_clear(void) | |||
There was a problem hiding this comment.
🟠 [Medium] disk_crypto_clear() leaves the expanded key schedule and nonce live, so the new scrub does not actually destroy the key
The PR adds a disk_decrypted_header_clear(dec_hdr); disk_crypto_clear(); pair before the FIT DTS-load panic (F-6130), whose stated purpose is that key material must not stay resident when wolfBoot_panic() spins forever. But disk_crypto_clear() only zeroes this file's own copies (disk_encrypt_key, disk_encrypt_nonce). The cipher state that actually performs the decryption lives in globals in src/libwolfboot.c: Aes aes_dec, aes_enc; (line 2155) or ChaCha chacha; (line 2105), plus static uint8_t encrypt_iv_nonce[ENCRYPT_NONCE_SIZE] (line 92). aes_init()/chacha_init() expand the key into those structs via wc_AesSetKeyDirect()/wc_Chacha_SetKey(), and crypto_decrypt() uses them directly. None are touched. After the new scrub (and after every other scrub site in this file) the raw key is still trivially recoverable from aes_dec.key[] / chacha.X[] and the nonce from encrypt_iv_nonce. This is a pre-existing limitation of the helper, but the PR is specifically extending its reach, so it is worth closing in the same change.
Fix: Add a crypto_deinit() helper in libwolfboot.c that wc_ForceZero()s aes_dec/aes_enc (plus wc_AesFree()) or chacha, and encrypt_iv_nonce, then call it from disk_crypto_clear(). Extend unit-update-disk-fit.c's panic-hook snapshot to also inspect the cipher context, so the test proves the key is really gone.
| else if (CMD.sign == SIGN_ML_DSA) { | ||
| wc_MlDsaKey_Free(&key.ml_dsa); | ||
| free_key(CMD.sign, 0); | ||
| if (CMD.hybrid) { |
There was a problem hiding this comment.
🟠 [Medium] main() still exit(1)s on secondary-key load failure, skipping the very key scrubbing F-8006 restores
F-8006's rationale is that exit(1) deep in the call chain "skips ... main()'s zero_and_free(kbuf, key_buffer_sz) and algorithm-specific key free, so the raw and decoded private signing key are never scrubbed." That reasoning applies verbatim to the hybrid path a few lines above the changed cleanup: when load_key(..., 1) fails, exit(1) runs while the primary raw key buffer kbuf is live and the primary decoded key object key is fully initialized. Neither zero_and_free(kbuf, key_buffer_sz) nor the new free_key(CMD.sign, 0) is reached. A missing or unreadable secondary key file is an ordinary user error, so this is easy to hit. Note unit-sign-hybrid-keyload.c's tcase_add_exit_test(..., 1) currently pins the exit(1) behaviour, so a fix needs that test updated too.
Fix: Replace the exit(1) with a jump to the existing tail cleanup (or set ret and skip to it), so the primary raw and decoded key are scrubbed on this path too. Update test_sign_main_fails_when_secondary_key_missing to match the new exit status.
| XMEMCPY(block, data + (ENCRYPT_BLOCK_SIZE * i), ENCRYPT_BLOCK_SIZE); | ||
| crypto_encrypt(ENCRYPT_CACHE + (ENCRYPT_BLOCK_SIZE * i), block, | ||
| ENCRYPT_BLOCK_SIZE); | ||
| while (step > 0) { |
There was a problem hiding this comment.
🔵 [Low] ext_flash_encrypt_write() chunk size is not rounded down to an ENCRYPT_BLOCK_SIZE multiple
The new staging loop caps chunk at NVM_CACHE_SIZE but never rounds it down to a whole number of encryption blocks. The for loop then encrypts only floor(chunk / ENCRYPT_BLOCK_SIZE) blocks while ext_flash_write(address, ENCRYPT_CACHE, chunk) writes all chunk bytes - so if NVM_CACHE_SIZE % ENCRYPT_BLOCK_SIZE != 0 the tail of every chunk is stale/uninitialized cache content, address stops being block-aligned, and the keystream desynchronises against ext_flash_decrypt_read(). Worse, if NVM_CACHE_SIZE < ENCRYPT_BLOCK_SIZE the loop encrypts nothing at all and writes pure garbage. Today NVM_CACHE_SIZE defaults to WOLFBOOT_SECTOR_SIZE, whose smallest value across config/examples/ is 512, so every shipped config is a clean multiple of both 16 (AES) and 64 (ChaCha) and this is latent. It is a silent trap for any -DNVM_CACHE_SIZE override or a user-supplied WOLFBOOT_ENCRYPT_CACHE (whose size is never checked against NVM_CACHE_SIZE).
Fix: Round the cap down to a block multiple, and add a compile-time assertion (the file already uses the typedef char ...[cond ? 1 : -1] idiom at line 2803) that NVM_CACHE_SIZE >= ENCRYPT_BLOCK_SIZE and NVM_CACHE_SIZE % ENCRYPT_BLOCK_SIZE == 0.
| } | ||
| #endif /* !WOLFBOOT_UNIT_TEST_FLASH_ERASE */ | ||
|
|
||
| int RAMFUNCTION hal_flash_erase(uint32_t address, int len) |
There was a problem hiding this comment.
🔵 [Low] mcxw hal_flash_erase() does not extend len by the round-down amount, leaving the last sector unerased
The fix correctly unifies the stride, but len is still the caller's original value after address has been rounded down to the sector boundary. A request whose start is unaligned and whose end falls into a later sector therefore under-erases: with sector_size = 0x1000, hal_flash_erase(FLASH_BASE + 0xF00, 0x200) rounds the address to FLASH_BASE, runs one iteration (len goes 0x200 -> -0xE00) and erases only sector 0, leaving [0x1000, 0x1100) in sector 1 unerased even though it was inside the requested range. hal/mcxn.c (cited as the model) has the same shape, and the new test_erase_unaligned_start_rounds_down picks (0x800, 0x1800) which happens to be covered, so the gap is codified rather than caught. Practical reachability is low - the standard wb_flash_erase() callers pass sector-aligned addresses - but the guard is cheap.
Fix: Add the alignment adjustment to len when rounding the address down, and add a unit-flash-erase-mcxw case such as hal_flash_erase(FLASH_BASE + 0xF00, 0x200) with sector_size = 0x1000 asserting two erase commands. Consider the same fix in hal/mcxn.c.
| os_image.fw_size); | ||
| } | ||
| #endif | ||
| /* bootm relocates to ih_load but enters at ih_ep: kernels built |
There was a problem hiding this comment.
🔵 [Low] ih_ep is still discarded when ih_load == 0, and the TODO that documented that gap was removed
The PR deletes (void)ih_ep; /* TODO: pass through to do_boot when ih_ep != ih_load */ and replaces it with the ih_ep capture - but the capture lives inside the if (ih_load != 0) branch. In the else branch (ih_load == 0, the Linux/PPC convention) the code still only advances load_address past the 64-byte uImage header and never looks at ih_ep. A uImage built with mkimage -a 0 -e \<ep> is valid and U-Boot bootm would enter at ih_ep; wolfBoot will still enter at load_address + 64. The capture point within the ih_load != 0 branch is otherwise correct - load_address = (uint32_t*)(uintptr_t)ih_load happens above it, so uboot_load == ih_load as intended, and the (uintptr_t) cast matches the existing ih_load cast so it is fine on 64-bit.
Fix: Either handle ih_load == 0 as well (capture uboot_load after the +UBOOT_IMG_HDR_SZ advance and set uboot_entry when ih_ep differs), or add an explicit comment in the else branch stating that ih_ep is intentionally ignored there, so the information carried by the removed TODO is not lost.
| } | ||
| else if (CMD.sign == SIGN_ML_DSA) { | ||
| wc_MlDsaKey_Free(&key.ml_dsa); | ||
| free_key(CMD.sign, 0); |
There was a problem hiding this comment.
🔵 [Low] free_key() can run on a never-initialized key2 object
main() now calls free_key(CMD.secondary_sign, 1) whenever CMD.hybrid is set. There are paths where the corresponding member of key2 was never initialized: with --manual-sign or --sha-only, load_key_rsa() takes the branch at line 599 and never calls wc_InitRsaKey(); load_key()'s ED25519/ED448 raw-public-key branches (lines 736-740, 805-808) never call wc_ed25519_init()/wc_ed448_init(); and in the DER-public-key branches load_key() already frees "no matter what" (lines 754-755, 822-823), so free_key() frees a second time. In practice this is safe - key/key2 are zero-initialized file-scope statics and the wolfCrypt frees are NULL-checked and idempotent - and the primary object had exactly the same exposure before the PR, so the change is symmetric rather than worse. Flagged because the new helper makes it cheap to state the invariant instead of relying on it implicitly.
Fix: Either add a loaded flag to struct signing_key, set where the object is actually initialized and cleared in load_key()'s "free no matter what" paths, or document the zero-initialized/idempotent assumption in the free_key() comment.
| tools/unit-tests/unit-otp-keystore-gen-zeroize | ||
| tools/unit-tests/unit-tpm-api-names | ||
| tools/unit-tests/unit-tpm-nsc-cert | ||
| tools/unit-tests/unit-tpm-advio-zeroize |
There was a problem hiding this comment.
🔵 [Low] Two new unit-test binaries are missing from .gitignore
The PR adds five new unit-test targets to tools/unit-tests/Makefile (unit-tpm-advio-zeroize, unit-tpm-mfgid-eh-zeroize, unit-flash-erase-mcxw, unit-flash-write-nrf52, unit-update-disk-fit) but only three of the resulting binaries to .gitignore. tools/unit-tests/unit-tpm-mfgid-eh-zeroize and tools/unit-tests/unit-flash-write-nrf52 are missing, so they show up as untracked files after make run in tools/unit-tests.
Fix: Add the two missing entries next to their siblings in .gitignore.
| @@ -83,15 +83,19 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) | |||
| } else { | |||
There was a problem hiding this comment.
🟠 [Medium] nrf52/nrf5340/stm32l0: the sibling 32-bit fast path still indexes off the original address and is left broken by the…
The PR fixes the byte-wise branch to derive the containing word from address + i, but the 32-bit fast branch immediately above it still uses dst = (uint32_t *)address; ... dst[i >> 2] = src[i >> 2];. That addresses physical byte address + 4*(i>>2), which equals the intended address + i only when i % 4 == 0 - i.e. only when address % 4 == 0. When the destination is misaligned and the source shares that misalignment (e.g. address % 4 == 1 and data % 4 == 1), the fixed byte loop advances i to 3, at which point both fast-path conditions are satisfied and the code writes data[0..3] to address..address+3 instead of data[3..6] to address+3..address+6 - plus it issues an unaligned 32-bit flash access, which faults outright on the Cortex-M0+ of stm32l0. This is the same defect class F-6757 fixes in the byte path, in the same function, in all three files the PR touches. The new unit-flash-write-nrf52.c deliberately picks a source misaligned by 2 against a destination misaligned by 1 so that "the fast 32-bit path is never taken", so the regression suite does not cover it.
Fix: Fix the fast path the same way as the byte path (index the pointers by i directly rather than by i >> 2 off the base) in hal/nrf52.c, hal/nrf5340.c and hal/stm32l0.c, and add a test case to unit-flash-write-nrf52.c with address % 4 == data % 4 != 0 and len >= 8, which is the combination that currently reaches it.
Note: Referenced line (
hal/nrf52.c:75-82) is outside the diff hunk. Comment anchored to nearest changed region.
e24fff0 libwolfboot: declare ForceZero() in the test-app build
db8e768 aarch64: link the ARM ChaCha port when ChaCha is selected
4726dce update_disk: use wc_ForceZero() in the DISK_ENCRYPT helpers
f9fe138 F-6130: clear disk_encrypt_key/nonce on the FIT DTS load failure path
f755125 F-6757: fix partial-word hal_flash_write on nrf52, nrf5340 and stm32l0
40021b8 F-7069: clear the EH authValue from the stack in wolfBoot_tpm2_get_timestamp
a799a98 F-7382: clip QSPI page program transfers at the device page boundary
ab7b79c F-7383: use one consistent sector size in mcxw hal_flash_erase
bc743ad F-7987: abort the swap when a sector copy fails
f446a4a F-8007: wipe TPM advanced-IO staging buffers in TPM2_IoCb()
93edc29 F-7985: enter legacy uImage at ih_ep when it differs from ih_load
5dfdec3 F-7992: bound staged ciphertext in ext_flash_encrypt_write()
f9957da F-8003: separate decoded key objects for hybrid signers
748fa8a F-8006: return errors from delta base-hash validation in sign tool
e200579 F-7969: validate boot-side digest before delta base hash compare
7608e33 F-7988: clamp unaligned head size in ext_flash_encrypt_write()
e6655e6 F-7989: clamp unaligned head size in ext_flash_decrypt_read()