Fenrir fixes 2026 09 04 - #882
Merged
Merged
Conversation
The per-sector fill loop in wolfBoot_delta_update() advances in DELTA_BLOCK_SIZE steps, so a WOLFBOOT_SECTOR_SIZE that is not a multiple of DELTA_BLOCK_SIZE writes past the one-sector SWAP partition and misaligns the resume path. Enforce the invariant with a #error and add a negative build test (unit-delta-sector-align.py). Verification: full unit suite 1096 checks, 0 failures; new test fails pre-fix (misaligned config built), passes post-fix (build rejected).
In the DISABLE_BACKUP branch of wolfBoot_update() the ELF-scatter restore block passed the boot struct by value to the pointer-taking PART_IS_EXT macro, so DISABLE_BACKUP + WOLFBOOT_ELF_FLASH_SCATTER + EXT_FLASH did not even compile, and the load result was discarded. Mirror the wolfBoot_start() pattern (PART_IS_EXT(&boot), panic on load failure), drop the dead base local, and add a compile check for the combination (unit-elf-scatter-db-build.py) guarding the one test that needs a DISABLE_BACKUP-excluded symbol. Verification: full unit suite 1096 checks, 0 failures; new test fails pre-fix (struct vs pointer compile error), passes post-fix.
STAGE1_AUTH only authenticates the stage2 wolfBoot payload; the FSP-M and FSP-S blobs are executed unverified. Remove the dead fsp_m/ret declarations in start(), the orphaned .sig_fsp_s placeholder section (no stage1 linker script places it), and correct the comment that claimed the FSPs were authenticated. Add a compile check for the STAGE1_AUTH variant (unit-x86-fsp-stage1auth-build.py), which the unit test CI never builds for lack of an i686 toolchain. Verification: full unit suite 1096 checks, 0 failures; both STAGE1_AUTH variants of boot_x86_fsp.c compile clean; no references to sig_fsp_s remain.
main() loaded a file of any size and parsed it as a manifest: header fields (size, TLVs) were read past the end of the heap allocation, and a header claiming a larger fw_size drove the image hash over an unbounded range. Reject files smaller than IMAGE_HEADER_SIZE before parsing and clamp fw_size to the bytes actually loaded.
The WOLFBOOT_FSP low-memory size check was the only per-slot rejection in the boot retry loop that used a bare break, so an image whose header-declared fw_size exceeded the tolum window aborted the boot instead of trying the other slot. Every other rejection in the same loop switches partitions and retries; match that.
The UEFI MEMMAP_DEVICE_PATH EndingAddress is inclusive (last valid byte), but x86_64_efi_do_boot() set it to boot_addr + size, describing every image as one byte longer than it is; the AArch64 sibling already uses size - 1. A zero-size image would underflow that computation and hand an empty range to LoadImage, so reject it up front, as the AArch64 sibling does. The local panic() returns to the unit test under UNIT_TEST so the zero-size path is observable.
boot_x86_64.c declared x86_64_efi_do_boot(uint8_t *) while the HAL defines (uint32_t *, uint8_t *): the linker connected the incompatible pair and the call was undefined behavior, mostly latent because the second parameter was discarded. Match the AArch64 sibling: single const uint32_t *boot_addr in the declaration, definition and call, and remove the unused dts_address parameter.
The PCR-extension block in wolfBoot_unlock_disk() is guarded with !defined(ARCH_SIM) while the function itself only builds for ARCH_SIM, so it can never compile. The exclusion is deliberate (eb2978a: do not extend the unseal PCR on the simulator, or the secret becomes un-unsealable), not an oversight. Clarify the comment with the intended build scope instead of removing the block: the code and the WOLFBOOT_NO_UNSEAL_PCR_EXTEND option exist for the day the unlock-disk path is ported to a non-sim target.
wc_ecc_rs_raw_to_sig() takes a word32* outlen, but the wolfHSM client/server path in wolfBoot_verify_signature_ecc() declared the buffer length as size_t and cast the pointer. On a 64-bit big-endian target the API reads the high (zero) half, so the DER conversion sees outlen 0 and the write-back lands in the wrong half of the size_t. Declare tmpSigSz as word32 and pass &tmpSigSz directly; this also matches the word32 sigLen of wc_ecc_verify_hash() below. Add a host compile check for the WOLFBOOT_ENABLE_WOLFHSM_CLIENT build of image.c, which unit CI did not cover (only the PIC32CZ cross build).
wolfBoot_load_flash_image_elf() ignored read_flash_fwimage failures and used the program-header fields unchecked, so a failed read consumed indeterminate stack data and a malformed (but signed) ELF could drive an out-of-bounds source read and erase/write at an unintended destination. Check the ELF header and program header reads, then validate each PT_LOAD segment before copying: file_size fits a 32-bit length, the source stays inside the manifest image, the paddr range fits the destination address width, and the destination stays inside the boot partition (mirrors the sibling check-function validation). Check the copy result. New unit tests in unit-image-elf-scatter.c cover the load path: a valid restore (positive control) and rejections for source past fw_size, paddr range overflow, destination outside the boot partition, and a program header that cannot be read. The first three fail pre-fix (the mock flash layer also catches the two out-of-range destinations with its own address check); the phdr read-failure case consumed uninitialized stack data pre-fix (valgrind: conditional jump on uninitialised value at the is_loadable check).
get_sha_block() accepted offset == fw_size and always read WOLFBOOT_SHA_BLOCK_SIZE bytes; wolfBoot_peek_image() always reported the full block size. Both could hand callers a window past the end of the image. Reject offset >= fw_size, clamp the external read to the bytes remaining, and report the clamped size (0 when no bytes remain). Hash callers already clamp their input, so this is API hardening; pinned by test_peek_image_bounds (internal + ext cases).
wolfPKCS11/PSA_Store_Read/Write added int len to unsigned in_buffer_offset before validation: a sufficiently negative len wrapped, hit the truncation branch, and was replaced by the remaining object or capacity bytes, bypassing Write's later len < 0 guard. Reject len < 0 first in all four functions (post-clamp guard kept).
keystore_get_size() returns -1 on invalid or oversized OTP slot data; storing it in uint16_t made -1 become 65535, which passed the hdrSz <= 0 check and was fed to the ECC/RSA parser as a 65535-byte read from the keystore buffer. Keep it as int and reject values <= 0 or above KEYSTORE_PUBKEY_SIZE.
The H7 OTP memory is one-time programmable, so the keystore and UDS are permanent once written; the H7 has no OTP block-lock register (unlike the H5), so there is no write-protection step to perform. Replace the misleading TODO with the reason the no-op is correct.
If the snap allocation fails but the work allocation succeeds, the initialisation loop never runs and nsc_tmpl_free() would release indeterminate work[].pValue pointers. Zero work[] right after the allocation (before the NULL check) so every path that reaches the free hands out initialised, NULL pValue entries.
The ELF scatter destination is the exec region, which sits outside the boot partition that stores the signed ELF. Bounding it to the boot partition rejected every legitimate segment (aurix exec is below boot, sim scatter is above it) and bricked corruption recovery. The paddr is covered by the image signature verified before this restore path and the overflow check keeps it from wrapping, so no destination bound is needed; this also matches the check function, which bounds no destination.
panic() returns under UNIT_TEST, so the LoadImage-failure site fell through to StartImage on a failed load. Add the return to match the zero-size guard; on target panic() never returns, so behavior is unchanged.
danielinux
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 4, 2026 13:56
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR hardens several boot/update/security-related code paths (ELF scatter restore, delta updates, TPM key loading, store APIs, EFI boot) and adds/extends unit tests and compile-only build checks to prevent regressions across rarely-built configurations.
Changes:
- Add runtime/compile-time guards to prevent out-of-bounds reads/writes and integer narrowing issues across image loading, delta updates, and store read/write APIs.
- Fix x86 FSP and x86_64 EFI flows (slot fallback behavior, device path address range correctness, unit-test-safe panic behavior).
- Expand unit coverage and CI checks with new unit tests plus compile-only targets for configuration-specific branches.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-x86-fsp-stage1auth-build.py | Adds compile-only CI check for STAGE1_AUTH x86 FSP variant. |
| tools/unit-tests/unit-update-flash.c | Gating of swap-resume test when backup is disabled. |
| tools/unit-tests/unit-update-disk-fsp.c | New unit tests for FSP low-memory rejection fallback behavior. |
| tools/unit-tests/unit-tpm-rsa-exp.c | Adds test coverage for rejecting failed keystore size and tracking DER decode calls. |
| tools/unit-tests/unit-psa_store.c | Adds negative-length regression tests for PSA store read/write. |
| tools/unit-tests/unit-pkcs11_store.c | Adds negative-length regression tests for PKCS11 store read/write. |
| tools/unit-tests/unit-pkcs11-nsc-zeroize.c | Strengthens tests around XFREE on partial allocation failures. |
| tools/unit-tests/unit-image.c | Adds bounds tests for wolfBoot_peek_image size clamping behavior. |
| tools/unit-tests/unit-image-wolfhsm-client-build.py | Adds compile-only CI check for WOLFHSM client signature conversion path. |
| tools/unit-tests/unit-image-elf-scatter.c | Adds load/restore-path tests for ELF scatter with additional validation cases. |
| tools/unit-tests/unit-elf-scatter-db-build.py | Adds compile-only CI check for DISABLE_BACKUP + ELF scatter + EXT_FLASH configuration. |
| tools/unit-tests/unit-efi-x86-open-image.c | Adds tests/mocks to validate UEFI memory device path correctness and prototype consistency. |
| tools/unit-tests/unit-delta-sector-align.py | Adds build-failure test for misaligned delta sector/block sizes. |
| tools/unit-tests/Makefile | Wires new unit tests and compile-only targets into the test run. |
| src/update_flash.c | Adds delta sector/block invariant and hardens ELF scatter restore error handling/logging. |
| src/update_disk.c | Fixes FSP low-memory rejection to fall back to the alternate slot instead of aborting. |
| src/tpm.c | Prevents narrowing keystore size and bounds-checks public key header size. |
| src/psa_store.c | Rejects negative len before unsigned arithmetic in read/write. |
| src/pkcs11_store.c | Rejects negative len before unsigned arithmetic in read/write. |
| src/pkcs11_callable.c | Zero-initializes work buffer to avoid freeing indeterminate pointers on failure paths. |
| src/image.c | Fixes WOLFHSM sig-length type, clamps SHA block reads/peeks, and validates ELF scatter segments before flash writes. |
| src/boot_x86_fsp.c | Removes dead scaffolding and clarifies STAGE1_AUTH behavior/comments. |
| src/boot_x86_64.c | Fixes x86_64_efi_do_boot prototype to avoid pointer truncation/ABI mismatch. |
| hal/x86_64_efi.c | Makes panic unit-test-friendly, fixes MEMMAP device path end address, rejects zero-size images. |
| hal/stm32h7.c | Documents OTP immutability rationale for readonly behavior. |
| hal/library.c | Rejects undersized files and bounds hash to the bytes actually loaded. |
| .github/workflows/test-library.yml | Adds workflow coverage for rejecting undersized image files in test-lib. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
previously requested changes
Sep 4, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #882
Scan targets checked: wolfboot-bugs, wolfboot-src
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.
paddr/filesz/offset were unsigned long, which is 32-bit on the ELF-scatter targets (aurix-tc375, sim32). ELF64 program headers truncate before the segment guards run, so filesz > UINT32_MAX is never true and an out-of-range paddr wraps in-range. Use uint64_t to match wolfBoot_check_flash_image_elf so the guards see untruncated values.
Cap the library fw_size clamp at UINT32_MAX (a > 4 GiB file would otherwise truncate to a small value), say 'restore' not 'store' in the scatter-restore error message, and print stdout as well as stderr in the compile-check scripts on failure.
wolfSSL-Fenrir-bot
previously requested changes
Sep 4, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #882
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
offset/filesz are uint64_t now; cast them for the %08lx/%lu conversions so the varargs read matches on 32-bit targets, matching the check function and the rejection messages above.
wolfSSL-Fenrir-bot
previously requested changes
Sep 4, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #882
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
do_boot() already returned after its panic() calls, but GetVolume() and efi_main() did not: under UNIT_TEST panic() returns, so GetVolume() hit a NULL deref / uninitialized return and efi_main() ran wolfBoot_start(). Return NULL / EFI_LOAD_ERROR at each site (unreachable on target).
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #882
Scan targets checked: wolfboot-bugs, wolfboot-src
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 stale reviews from themself
September 4, 2026 19:43
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
dgarske
approved these changes
Sep 5, 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.
01a71c7 x86_64_efi: return after LoadImage-failure panic under UNIT_TEST
58e79da F-11047: image: drop incorrect boot-partition destination check
62918f2 F-12107: pkcs11 nsc: zero work buffer before it can be freed
d64f15a F-7392: stm32h7: document inherent OTP immutability in set_readonly
b9068ce F-12105: tpm: keep keystore size as int in load_pubkey
b5ab88a F-9744: store: reject negative length before unsigned arithmetic
a6628a3 F-12108: image: clamp sha block/peek to fw_size
0e2a463 F-11047: image: validate ELF scatter segments before flash writes
5571124 F-11026: image: declare wolfHSM DER sig length as word32
785a285 F-11025: update_flash: document deliberately inert PCR extend block
eb3d587 F-9746: x86_64_efi: fix do_boot prototype mismatch, drop unused param
d244fd9 F-9745: x86_64_efi: inclusive mem path end, reject zero-size image
a2d5543 F-9740: update_disk: FSP low-mem rejection falls back to other slot
77fe733 F-7391: library hal: reject undersized files, bound hash to loaded size
88af5b6 F-7390: x86 fsp: drop dead FSP auth scaffolding, fix comment
455c1c6 F-7067: elf scatter: fix PART_IS_EXT arg and check load result
f598b3f F-6878: delta: reject non-multiple SECTOR_SIZE/DELTA_BLOCK_SIZE at build