Fenrir fixes 2026 08 21 - #868
Open
danielinux wants to merge 27 commits into
Open
Conversation
ext_flash_read() ignored the intra-page offset of the start address (a partial read returned bytes from the head of the page), copied sub-page reads in 32-bit words (dropping a sub-word tail), and wrote a full NAND page into the caller's buffer when a multi-page read ended mid-page (overrunning it). The integrity check hashes the image in 64-byte blocks from fw_base + offset, so any read crossing a page mid-block returned the wrong bytes. Walk the read page by page from the exact address: full aligned pages go straight into the caller's buffer; partial first/last pages are staged through the page buffer and copied from the column offset. Proven by unit-sama5d3-ext-read, which extracts the real function and runs it against an emulated device: unaligned starts, 1-3 byte lengths, the 64-byte integrity-hash block pattern, page/block boundaries, and multi-page reads with a short tail. Pre-fix 6 of the 8 checks failed.
ext_flash_write() and ext_flash_erase() discarded their arguments and returned 0, so any update flow that reaches this HAL was told an erase/program succeeded while the NAND contents were left untouched. NAND page program and block erase are not implemented for this target; report failure so callers that branch on the result can detect it instead of continuing with a write that never happened. The read-only boot path is unaffected.
The P1021 QE and T10xx QE/FMan microcode blobs are validated only for structural integrity (header magic, version, size bounds) before they are activated; no cryptographic authentication is applied. State that in Targets.md for both targets, with the deployment-model reasoning: the P1021 microcode region sits inside the update partition, and the T10xx regions require local board flash write access, which also allows replacing the wolfBoot image itself. Same residual claim as F-8000 and F-9761 (T10xx QE and FMan paths); the bounds fixes for all three are already in master (000c05c, 91c020a). Product decision: no microcode authentication feature.
wolfBoot_verify_signature_lms() returned without calling wc_LmsKey_Free() when wc_LmsKey_SetParameters() or wc_LmsKey_ImportPubRaw() failed after a successful wc_LmsKey_Init(), leaking the key (and whatever heap state the LMS backend allocated for it). Free the key on both error paths, matching the success path.
In the wolfHSM branch of wolfBoot_verify_signature_ecc(), both failure paths after wc_ecc_init_ex() - the HSM key-ID setup and wc_ecc_import_unsigned() - did a bare return that skipped the wc_ecc_free(&ecc) at the end of the function, leaking the key (and its heap-allocated mp_ints in non-SP-math builds). Free the key before returning on both paths, matching the plain wolfCrypt path. The wc_ecc_import_unsigned() path sits in the server-only, non-cert-chain configuration, which does not compile today (it references pubkey/ point_sz, declared only for the software and client builds) - see the F-7995 note; the free is added there for consistency so the path is correct if that configuration is ever made buildable.
libwolfboot.c defined the external-flash header-cache flag as uint32_t while image.c declared it extern int - the same object with incompatible types in two translation units is undefined behaviour. It is benign on every supported target (both are 32-bit) but a latent portability defect; the flag is written from both files. Make the definition int, matching both declarations.
The fman and esdhc fixup guards tested 'off != !FDT_ERR_NOTFOUND', which is 'off != 0'. fdt_node_offset_by_compatible() returns a negative offset when the node is absent, so the guard passed and the fixups ran with a negative offset (fdt_setprop() rejects it, the fixups fail silently with misleading log lines); conversely a node at struct offset 0 (the root node) matched but was skipped. Every other guard in the function already used '-FDT_ERR_NOTFOUND'. Use the correct guard in both blocks and fix the esdhc status fixup's log label, a copy-paste from the cpu fixup block. unit-t10xx-dts-memac gains four cases driving the real hal_dts_fixup(): a root node compatible with fsl,fman gets the clock fixup (failed before the guard fix), child fman and esdhc nodes get their fixups, and absent nodes are skipped cleanly.
fdt_node_offset_by_compatible() compared each entry of a multi-string compatible value with memcmp(compatible, prop, complen+1) before locating the entry's NUL terminator. When the declared property length equals the search length (no room for a NUL), the comparison reads one byte past the property data and accepts the entry when that byte happens to be zero (e.g. the 4-byte alignment padding). Locate each entry's NUL terminator within the declared length first and only compare entries whose length equals the search length: nothing is read past the property, and an unterminated trailing entry can no longer match. unit-fdt gains four cases with a minimal single-node FDT builder: an unterminated exact-length entry does not match (it did before the fix), a terminated exact-length entry matches, multi-string lists still match on later entries, and a longer entry that starts with the search string does not match.
…ring fdt_get_string() bounded stroffset against size_dt_strings but formed the string-table pointer from off_dt_strings without ever validating either header field against totalsize; a DTB declaring a large off_dt_strings with a small size_dt_strings made every property lookup (fdt_getprop -> fdt_get_string) scan far outside the blob. Validate the structural layout in fdt_check_header() for finalized (FDT_MAGIC) blobs: the reservation map, structure block and string table must sit inside the blob and not overlap, checked in 64-bit so the size fields cannot wrap. fdt_get_string() now requires a valid header before forming the pointer. The SW_MAGIC (in-progress edit) state keeps its existing check, since its layout is different. Test fixtures are adjusted to the validated layout: the two pre-existing fdt_get_string fixtures now set the header fields the lookup relies on, the compatible-test builder sets the magic word and points the reservation map at the canonical empty list right after the header (it pointed into the string table before).
fit_find_images() took the FIT configuration's image names (kernel/fdt/ramdisk/fpga) and the configuration name (default) straight from fdt_getprop() and passed them on to fdt_find_node_offset(), which strlen()s them; fit_load_image_inner() strcmp()'d the compression property after only checking it was non-empty. A property value not NUL-terminated within its declared length makes those calls scan past the property - and past the end of the blob for a property at the tail. Add fit_getprop_string(), which returns the property value only when it is NUL-terminated within its declared length, and use it for the five name properties (a malformed value is rejected and the type-based search still applies). Compare compression within the declared length: the value must be exactly "gzip" or "none"; any other shape fails closed with the existing unsupported-compression path instead of being strcmp()'d past the property. unit-fdt gains a FIT whose configuration kernel property is unterminated (the valid default is still honored, the image name is rejected); unit-fit-gzip gains a truncated compression="none" value, which used to pass the subimage through as raw and now fails closed. Both build variants (gzip enabled/disabled) run it.
The FIT boot path relocated the flat-dt sub-image with a copy whose length came from the FIT-declared data property length, never bounded against the WOLFBOOT_LOAD_DTS_ADDRESS staging region - unlike the sibling DTB paths, which all validate the parsed size against WOLFBOOT_DTS_MIN_SIZE/WOLFBOOT_DTS_MAX_SIZE first. The length was also harvested through a (int*)&dts_size cast of a uint32_t. Relocate the parsed DTB size instead: validate it against the same MIN/MAX bounds as the other DTB sources and copy that many bytes. An out-of-range DTB is rejected (dts_addr stays NULL and the existing fallback chain applies) rather than partially or oversize copied. Applied to both call sites of the pattern: update_ram.c (memcpy) and update_disk.c (wolfBoot_fit_memcpy), which also gains the DTS bounds macros it was missing. unit-update-disk-fit (drives the real update_disk.c wolfBoot_start) gains two cases: a parsed size above WOLFBOOT_DTS_MAX_SIZE and one below WOLFBOOT_DTS_MIN_SIZE are both rejected without a copy, while the existing success/failure-copy cases keep passing. The staging stand-in is grown so the pre-fix unbounded copy is observable as a copy instead of a crash.
wolfBoot_check_flash_image_elf() fed every PT_LOAD entry's
paddr/BASE_OFF straight into update_hash_flash_addr() with the 64-bit
file_size truncated to the uint32_t the reader consumes, and never
bounded an intermediate segment's file layout against the manifest.
The read loop then memcpy's from (or drives the flash driver at)
whatever address the image declares - an unauthenticated partition
(e.g. WOLFBOOT_SKIP_BOOT_VERIFY builds) or a corrupt one could walk
the hash over unmapped memory.
Validate each loadable segment before hashing and fail the check
instead of continuing:
- file_size must fit the uint32_t hash length,
- offset + file_size must stay inside the manifest image
(overflow-safe comparison; previously only the last segment was
checked, after the loop),
- paddr + BASE_OFF + file_size must not overflow the address space.
The mismatch log no longer prints the first 8 digest bytes.
Note: a full flash-region bound for paddr needs a configured
scatter-region size; no such knob exists in the target configuration
today (scattered segments are deliberately placed outside the
boot/update/swap partitions), so the region check is left as a
follow-up.
unit-image-elf-scatter gains three cases with a multi-segment
fixture: a 2^32 file_size and a segment layout extending past
fw_size (both verified OK pre-fix because the stored digest matched
the truncated/out-of-layout walk) are now rejected with -1, and a
paddr whose range overflows the address space is rejected before any
flash read (pre-fix: read at 0xfffffffffffffffb, segfault on host).
The Doxygen comment documented a 'forcedEnc' parameter that does not exist (the function takes address, data, len) and named AES for a routine whose encryption step is the configured cipher - ChaCha20, AES-CTR, or a PKCS#11-backed cipher, per build configuration.
wolfBoot_swap_and_final_erase reads the staging-sector trailer into tmpBuffer (which also stages the firmware key/nonce under EXT_ENCRYPTED) and scrubs it on every exit except the resume early-return, which returned -1 with the buffer still holding the bytes just read from flash. Add the zeroize there so all four exits share the same invariant.
wolfBoot_crypto_set_iv() copies the firmware encryption nonce onto the stack (local_nonce) for the AES/PKCS#11 backends and aes_set_iv() derives iv_buf from it, and both returned without scrubbing the copies. The rest of the codebase pairs key scrubs with nonce scrubs (e.g. update_disk.c); these helpers are called once per encrypted block, leaving a nonce copy on the stack at the end of every encrypted I/O sequence, including the one preceding do_boot(). ForceZero both buffers after the derived IV is consumed. The PKCS#11 set_iv helper is intentionally left as-is: it writes the counter into the persistent pkcs11_params CTR state, which the token updates in-place and which must survive the call.
aes_set_iv() and pkcs11_crypto_set_iv() added the block counter to the nonce-derived counter block and propagated the carry with a loop that only runs - and whose trip count depends - on the nonce words: the overflow branch reveals that the high word was within one count of wrapping, and the inner loop's exit point reveals how many of the low words are 0xFFFFFFFF. crypto_set_iv() runs once per encrypted block, so a timing attacker gets one measurement per block. Replace both with an unconditional branch-free four-word carry (standard carry-out flags, three iterations regardless of content). Arithmetic is identical: verified old-vs-new expression equality over 5M random counter/nonce inputs plus the full-carry, zero-counter and max-counter boundary cases; unit-aes128/unit-aes256 encrypted roundtrips pass with the new code, and the ENCRYPT_PKCS11 CKM_AES_CTR path compiles clean.
Under EXT_ENCRYPTED + MMU, decrypt_header() decrypts the firmware manifest into the file-scope dec_hdr buffer, which the blob-field lookups and wolfBoot_ram_decrypt() consume - but never clear, so a plaintext manifest of an image whose confidentiality is the point of EXT_ENCRYPTED sat in .bss through do_boot(). The disk-boot twin (update_disk.c) wipes its equivalent on every exit. Add dec_hdr_clear() and invoke it once the field of interest has been extracted: in wolfBoot_get_blob_version/type/diffbase_version (the tails now extract into a local and return it, identical values in all builds) and in wolfBoot_ram_decrypt right after the length field is taken - the only field read from the manifest, the copy loop that follows uses its own block buffer.
wolfBoot_find_header() and the sign tool's re-parser checked each field's 4+len against (uint16_t)(header_size - IMAGE_HEADER_OFFSET). For any header of 64 KiB or more the cast wraps (0x10000 -> 0), so the guard rejects every field and an image the tool signs cannot be parsed by the bootloader - a pack/parse roundtrip break, fail-safe but fatal for large TLVs (post-quantum signatures, big cert chains). Compare in the uint32_t domain in both walkers. No shipped config reaches this size yet (largest example is 12288), so this pins the roundtrip for future large-header configs. unit-parser-large-header (new) builds the walker with IMAGE_HEADER_SIZE = 0x10008 - exactly the wrap boundary - and asserts a 300-byte TLV and a 4-byte version field are located (both were rejected pre-fix, proven against the pre-fix walker in a scratch build).
The EXT_ENCRYPTED wolfBoot_erase_encrypt_key() call at the tail of wolfBoot_success() - the only point in the normal update lifecycle that wipes the temporary firmware-decryption key/nonce from the boot-partition trailer - was never asserted: the default unit-update-flash build preprocessed it away (no EXT_ENCRYPTED), and the encrypted target (unit-update-flash-enc) ran only its fallback-only subset, so deleting the call would have survived the full suite. Give the CUSTOM_ENCRYPT_KEY mock a call counter, add test_boot_success_erases_encrypt_key asserting exactly one erase after confirmation, and register it in the UNIT_TEST_FALLBACK_ONLY branch so it runs under unit-update-flash-enc.
Both partial-block read-modify-write paths in ext_flash_encrypt_write read the stored block (ciphertext), spliced the new plaintext in, and re-encrypted the whole block. The untouched bytes were therefore XOR'd with the keystream a second time: stored ciphertext came back as plaintext in flash, and the next read of those bytes returned raw ciphertext instead of the original data. Any encrypted update whose first or last block partially overlaps a block with previous content - e.g. a retry over a previously written update image - silently corrupted the neighbouring bytes. Decrypt the stored block before splicing (into the scratch buffer, so no backend has to handle in-place decrypt) and re-encrypt the merged plaintext. Erased (0xFF) bytes round-trip unchanged because the decrypt/encrypt pair is the identity on the stored value. The re-encryption re-syncs the stream to the block index first: the decrypt step consumes keystream, and on the ChaCha/PKCS#11 backends encrypt and decrypt share a single stream state, while on the AES backends the decrypt context had not advanced with the full-block writes. The tail path also syncs the decrypt context, which on the AES backends sits at the first block's index after the aligned writes. Fallback-IV offset handling mirrors ext_flash_decrypt_read. New unit-extflash tests (run under the plain, AES-128, AES-256 and ChaCha20 variants): a mid-block patch must leave the untouched bytes of a previously written block intact, a trailing partial block must leave the rest of a previously written block intact, and a stream written in small unaligned chunks must round-trip byte for byte. All three fail on the pre-fix code with every cipher.
Add unit-update-flash-enc-full, the full end-to-end suite (forward updates, rollback, empty boot, diffbase) against the encrypted swap, plus a byte-for-byte fallback-IV roundtrip test. The suite exposes two product defects: - ext_flash_encrypt_write() partial-block re-syncs re-anchored the keystream at the standard-IV position once the one-shot fallback IV offset had been consumed by the initial set_iv, corrupting the tail of fallback-IV images. Capture the IV offset in effect at entry and re-apply it on every re-sync. - wolfBoot_final_swap() called wolfBoot_set_encrypt_key() with the internal flash unlocked, but the backend expects the flash locked (it manages the unlock/lock around the key write itself) and ends with the flash locked. Lock before the call and drop the now redundant lock on the failure path. Test plumbing for the encrypted target: update-partition writes in the tests now go through the encryption-aware writer, as the update tool does; the hand-rolled TLV headers use the sign tool's dense layout (padding gaps are ciphertext in encrypted builds); and the testing-flag sites anchor on the state trailer, which sits ahead of the key/nonce region in encrypted builds. Verified: unit-update-flash-enc-full 35/35, unit-update-flash-enc 8/8, unit-extflash + AES128/256/ChaCha20 variants 8/8 each, full unit suite green, stm32wb + AES256 cross-build green.
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.
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 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.
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.
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.
Copilot stopped reviewing on behalf of
danielinux due to an error
August 21, 2026 07:18
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.
dgarske
requested changes
Aug 21, 2026
| /* Add the block counter with an unconditional, branch-free carry: | ||
| * a conditional carry loop's trip count would depend on the nonce | ||
| * content and leak it through timing. */ | ||
| { |
Member
There was a problem hiding this comment.
Let's avoid the empty braces please.
| cb_words[3] += iv_ctr; | ||
| if (cb_words[3] < iv_ctr) { /* overflow */ | ||
| /* Unconditional, branch-free carry (see aes_set_iv) */ | ||
| { |
| #ifdef EXT_FLASH | ||
| uint8_t hdr_cpy[IMAGE_HEADER_SIZE] XALIGNED(4); | ||
| uint32_t hdr_cpy_done = 0; | ||
| int hdr_cpy_done = 0; |
Member
There was a problem hiding this comment.
Curious what drove changing this type considering hdr_cpy_done is only ever set to 0 and 1.
| uint32_t pages_to_read; | ||
| if (sz > remaining) | ||
| sz = remaining; | ||
| while (remaining > 0) { |
Member
There was a problem hiding this comment.
The F-7380 rewrite correctly fixes three real bugs (column offset, sub-word tail copy, full-page overrun) but breaks bad-block relocation. I confirmed it against the diff:
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
2 participants
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.
04531cb F-9751: Add positive E2E encrypted-update test, fix what it exposes
5caac27 F-9750: decrypt the stored block before the encrypted RMW patch
d64bf67 F-9755: assert wolfBoot_success erases the firmware encryption key
00bc8b5 F-6762: compare the TLV field budget in a 32-bit domain
e9ee837 F-7396: scrub the decrypted-header cache after field extraction
495b80b F-7966: propagate the CTR counter carry without branching on the nonce
f3098cb F-9766: scrub the nonce copies in the IV-derivation helpers
58390cb F-7071: scrub the staging buffer on the swap resume early-return
8d39a5f F-9721: make ext_flash_encrypt_write doc match its signature
f08c62a F-9756: validate PT_LOAD segments before the scatter flash hash walk
d554bbb F-7066: bound the FIT DTS relocation copy to the staging size
fe1dcb0 F-9757: length-bound the FIT name/compression string properties
19d8a9e F-9758: validate FDT layout in fdt_check_header, use it in fdt_get_string
08903c2 F-7990: bound the compatible string walk to declared lengths
8aa5221 F-7056: fix node-found guards and esdhc log label in hal_dts_fixup
c5ae368 F-7974: declare hdr_cpy_done as int in both translation units
d722c0f F-7971: free the ECC key on the wolfHSM setup error paths
ce5f20c F-7970: free the LMS key on the parameter/import error paths
431b015 F-9759: document that QE/FMan microcode is not authenticated
0a6a5fe F-7381: fail SAMA5D3 NAND write/erase instead of reporting success
f5401b0 F-7380: make SAMA5D3 ext_flash_read byte-accurate for partial pages