Add missing test cases suggested by fenrir, fix two hal regressions by danielinux · Pull Request #905 · wolfSSL/wolfBoot

danielinux · 2026-09-18T13:38:09Z

a2f7d8b Fix nrf5340 link error + cypsoc6 partition-overlap false positive
b0db2b7 F-13622: guard partition overlap with the bootloader write-protect region
6cc8437 F-13643: test swap round-trip restores the original boot image
f80dde6 F-9753: accept the exact-fit update size in wolfBoot_update
cf56410 F-9752: guard swap-resume test out of EXT_ENCRYPTED targets
963bec3 F-6874: test whFlashH5 Erase/Verify/BlankCheck OOB bounds guards
c62b509 F-9752: test interrupted per-sector swap resumes from BACKUP
59c47a8 F-13624: guard keygen-emitted keystore accessors against negative id
dfe7150 F-13623: nRF5340 hal_flash_protect lock the full requested range
03ea0bb F-13636: unit target for NSC update-partition bounds checks
ef4d73d Add header_required_size cross-branch regression test (F-13644)
ba74601 F-13646: test HDR_CMDLINE sign/decode roundtrip
bdfddc5 Add gpt_parse_header hdr_size bounds test (F-6759)
f1aa75a F-13633: test encrypt_key_is_valid erased-key rejection
bad0fa3 F-13635: isolate each diag_read_header content gate
21c82fa F-12883: nRF5340 uart_write CRLF dropped multiline tail data

After emitting CRLF the buffer advanced to the newline, not the byte after it, so multiline input reprocessed the newline (stray CRLFs) and lost the following text. Extract the CRLF conversion to hal/nrf5340_uart.c (no nrfx) for host unit testing; add a regression test.

The one negative test corrupted byte 0, tripping the magic and CRC gates at once, so a deleted gate was still caught by its siblings. Add three tests (magic/version/crc), each recomputing the header CRC so only the corrupted field's gate can reject.

encrypt_key_is_valid() rejects an erased key (all 0x00 or all 0xFF) but had no test, so deleting the check or flipping its && to || survived. Add three cases (all-0x00, all-0xFF, valid); both erased cases fail under ||, which pins the &&.

The hdr_size < GPT_HDR_MIN_SIZE || > GPT_SECTOR_SIZE guard was untested; a crafted 0xFFFFFFFF hdr_size would drive the CRC read ~4GB past the 512-byte stack header. New test rejects both bounds; the lower case recomputes the CRC over the reduced size so a deleted lower clause would accept the header (return 0), pinning the clause.

The --cmdline option stores the OS command line as a signature-covered TLV (tag HDR_CMDLINE) that the bootloader recovers via wolfBoot_find_header(); the roundtrip had zero coverage. Add test_make_header_ex_roundtrip_cmdline_tlv: for lengths 1, 2, 3, 69, 70, 71, 255 (odd/even + extremes + the ~70-byte default-capacity boundary) sign via make_header_ex and assert the decoder returns exactly the signed bytes; 255 also pins the 256->512 header auto-grow.

The size model was only compared against make_header_ex for NO_SIGN, no-timestamp, non-delta. Drive the cross product of sign/policy/ timestamp/delta/dts/hash-algo and assert the writer's final content size stays <= the model, via a test hook that records header_idx before the 0xFF padding. Hybrid is excluded (its secondary signature needs a real key context). Mutation-verified: removing the model's timestamp term makes the test fail (idx > required). Also reject a custom TLV reusing the --dts digest tag (F-9754): it would serialize ahead of the real digest and shadow it.

Add unit-nsc-update, the first unit target defining TZEN. It compiles libwolfboot.c with __WOLFBOOT + TZEN (non-CMSE: WOLFBOOT_NSC_NS_RW is a pass-through) against the mock flash and checks the accept/reject sides of the wolfBoot_nsc_erase_update / wolfBoot_nsc_write_update range checks, which had no unit coverage. A weakened len bound is caught by the straddle case writing past the partition end.

The region count truncated (n = len / SPU_FLASH_BLOCK_SIZE), so a len smaller than one 16 KiB SPU block locked nothing and a non-multiple len left the tail block writable, while the function returned 0 either way. Round the block count up so the locked range covers [start, start+len) whole (partial blocks at both ends locked whole, which only ever widens protection), and reject len < 0. Add unit-nrf5340-flash-protect, which sed-extracts the real function and runs it against a mock SPU region array; mutation (revert to the truncating count) fails 3 of 6 tests.

The keystore keygen emits (default for non-OTP, non-wolfHSM builds) lacked the id < 0 guard the OTP backend has: get_buffer/get_size/get_mask tested only id >= num (a negative id indexed PubKeys[] out of bounds) and get_key_type had no bounds check. Add 'id < 0 ||' to all four. Add unit-keygen-keystore, which sed-extracts the real Keystore_API template from keygen.c, emits a self-contained keystore.c (keystore_gen.c), compiles it, and asserts the same out-of-range contract unit-otp-keystore pins. Mutation (drop the get_buffer guard) makes keystore_get_buffer(-1) return non-NULL, caught.

A power fail mid-swap leaves the sector loop at the SECT_FLAG_BACKUP fall-through entry point, which no prior test reached (the fault-injection tests only aborted on the first write, and the vault resume tests cover a different path). Fault the first internal write (the swap->BOOT copy of sector 0) via the existing single-shot hal_flash_write_fail, re-run wolfBoot_update, and verify it re-enters the loop at case SECT_FLAG_BACKUP and ends with the partitions swapped. Only the BACKUP state is a recoverable power fail: faulting the BOOT->update copy instead (SWAPPING state) erases the update header, so the resume's re-open fails - that entry point is not testable as a roundtrip. Mutation-verified (skipping the swap->boot copy breaks the test).

The region guard (offset > ctx->size || size > ctx->size - offset) in whFlashH5_Erase/Verify/BlankCheck was untested for out-of-bounds input. Erase's alignment checks run after the bounds check, so an aligned OOB erase would pass alignment and erase adjacent flash with the guard removed; Verify/BlankCheck would walk OOB. Add aligned OOB cases (offset = ctx.size, size = sector; and offset 0, size = ctx.size + sector) asserting WH_ERROR_BADARGS. Mutation-verified: removing the Erase guard makes the OOB erase return ABORTED (the mock rejects the OOB erase) instead of BADARGS.

The resume test stages a plain image, which the encrypted swap path (unit-update-flash-enc-full) does not accept, so the test failed there. The resume logic is identical with or without encryption, so the non-encrypted target covers it; guard the test and its registration behind !EXT_ENCRYPTED.

The signer (sign.c) accepts total_img_sz == max_img_sz (the exact fit: header + payload + trailer == partition size), but wolfBoot_update rejected fw_size == MAX_UPDATE_SIZE via '> MAX_UPDATE_SIZE - 1', leaving a 1-byte gap and an off-by-one between the two components. Drop the '- 1' so the updater accepts the exact fit the signer produces. Rename test_update_max_size_rejected to _accepted (version 2 now stages) and add test_update_max_size_plus_one_rejected (MAX_UPDATE_SIZE + 1 still rejected).

A completed swap must leave the update partition as a faithful copy of the previous boot image, so the emergency-rollback path (the IMG_STATE_TESTING branch calling wolfBoot_update(1) to swap back) can restore the original byte-for-byte. The backup half of the swap (the boot->update copy, which under EXT_ENCRYPTED runs under wolfBoot_enable_fallback_iv(1)) was otherwise never read back: the forward direction is implicitly checked by wolfBoot_verify_integrity, but the reverse direction had no backstop. Add test_update_then_rollback_{samesize,larger,smaller}: install version 2, snapshot the original boot image, re-run wolfBoot_start (the fallback branch), and assert version 1 + memcmp(boot, snapshot) == 0. Parameterised over same/larger/smaller payloads to cover the tail-sector copy guard in both directions. Mutation-verified: skipping the boot->update copy breaks the tests. Runs in the enc-full target, which exercises the encrypted backup path.

…gion The boot, update and swap partitions were never checked against the bootloader write-protect region [WOLFBOOT_ORIGIN, WOLFBOOT_ORIGIN + BOOTLOADER_PARTITION_SIZE). An over-sized BOOTLOADER_PARTITION_SIZE silently write-protects the head of a partition; the existing boot/update/swap overlap guards did not cover this. Add three #error guards (boot, update, swap vs bootloader region) to include/target.h.in, gated on WOLFBOOT_ORIGIN < the partition address so they only fire for real targets (the mock target has WOLFBOOT_ORIGIN == WOLFBOOT_PARTITION_BOOT_ADDRESS, a valid simplified geometry). The undersized guard (BOOTLOADER_PARTITION_SIZE not covering the self-update erase range) is deferred: it requires a compile-time constant for ARCH_FLASH_OFFSET, which is a runtime value (sim_ram_base) for the mock target. The compile-only negative tests (part 2 of the finding) are deferred.

nrf5340: the target references nrf5340_uart_crlf (src/update_flash.c) but the Makefile does not compile hal/nrf5340_uart.c, so the linker fails. Add the object to OBJS (Makefile) and APP_OBJS (test-app). cypsoc6: the new boot/update/swap vs bootloader-region #error guards in include/target.h.in fire on the cypsoc6 layout, where the boot partition intentionally overlaps the bootloader region (the bootloader and the boot image share the same flash region). Introduce WOLFBOOT_ALLOW_PART_OVERLAP (defined in cypsoc6.config, gated in target.h.in, added to CONFIG_VARS in tools/config.mk) to suppress the guards for targets that legitimately overlap. Verified: nrf5340 builds (make distclean + cp config + make, exit 0, wolfboot.bin created). cypsoc6 cannot be built locally (the CI checks out lib/psoc6pdl from Infineon, which is not available locally), but the WOLFBOOT_ALLOW_PART_OVERLAP guard is verified to suppress the #error for the cypsoc6 layout.

Copilot

🟡 Changes recommended

Unresolved findings affect partition overlap validation, nRF5340 zero-length protection behavior, Python keystore generation, and UART test compilation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds regression tests and fixes for flash HALs, update flows, signing, keystore bounds, diagnostics, partition validation, and nRF5340 behavior.

Changes:

Expands unit-test coverage across reported edge cases.
Fixes nRF5340 UART/protection and exact-fit update handling.
Adds partition-overlap configuration and keystore accessor guards.

File summaries

File	Summary
`tools/unit-tests/unit-wolfhsm_flash_hal.c`	Adds flash bounds tests
`tools/unit-tests/unit-update-flash.c`	Adds swap and size tests
`tools/unit-tests/unit-sign-encrypted-output.c`	Adds signing regression tests
`tools/unit-tests/unit-nsc-update.c`	Adds NSC bounds tests
`tools/unit-tests/unit-nrf5340-uart-crlf.c`	Adds UART CRLF tests
`tools/unit-tests/unit-nrf5340-flash-protect.c`	Adds protection tests
`tools/unit-tests/unit-keygen-keystore.c`	Adds keystore bounds tests
`tools/unit-tests/unit-enc-nvm.c`	Tests erased-key rejection
`tools/unit-tests/unit-disk.c`	Adds GPT bounds tests
`tools/unit-tests/unit-diagnostics.c`	Adds diagnostic gate tests
`tools/unit-tests/Makefile`	Registers unit tests
`tools/unit-tests/keystore_gen.c`	Generates keystore fixtures
`tools/keytools/sign.c`	Adds TLV validation
`tools/keytools/keygen.c`	Guards keystore accessor IDs
`tools/config.mk`	Adds overlap configuration
`test-app/Makefile`	Links UART helper
`src/update_flash.c`	Accepts exact-fit updates
`Makefile`	Adds overlap and UART build handling
`include/target.h.in`	Adds overlap checks
`hal/nrf5340.c`	Fixes UART and protection logic
`hal/nrf5340_uart.c`	Adds CRLF conversion
`config/examples/cypsoc6.config`	Allows intentional overlap
`CMakeLists.txt`	Includes UART source

Review details

Suppressed comments (1)

hal/nrf5340.c:855

The new rounding changes the existing zero-length behavior for an unaligned start: with len == 0, tail is the start's intra-block offset and the ceiling expression locks one whole SPU region even though the requested interval is empty. Preserve the previous no-op behavior by returning success before calculating tail when the truncated length is zero.

    tail = (start % SPU_FLASH_BLOCK_SIZE) + (uint32_t)len;
    n = (tail + SPU_FLASH_BLOCK_SIZE - 1) / SPU_FLASH_BLOCK_SIZE;

Files reviewed: 23/23 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The test-app Makefile was not linking nrf5340_uart.o (which defines nrf5340_uart_crlf) for nrf5340% targets with DEBUG_UART=1, causing 'undefined reference to nrf5340_uart_crlf' when linking image.elf. Also guard the main Makefile's nrf5340_uart.o with DEBUG_UART=1 for consistency.

- Add missing END_TEST after each START_TEST in unit-nrf5340-uart-crlf.c - Fix partition overlap checks in target.h.in to use standard interval intersection (catches partitions that start below WOLFBOOT_ORIGIN and extend into the bootloader region)

Copilot

🟡 Changes recommended

Critical and moderate review findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

tools/unit-tests/unit-nrf5340-uart-crlf.c:44

The capture buffer is not terminated after each write. Because reset_cap() only clears cap[0], the next test leaves stale bytes after the newly captured data; for example, after the multiline test, the no-newline test writes hello and ck_assert_str_eq() reads beyond it instead of seeing a NUL terminator. Terminate the buffer after incrementing caplen (and keep the existing size bounded).

hal/nrf5340.c:855

For an unaligned start and len == 0, this rounds the start offset up to one SPU block and locks it even though the requested range is empty. That changes the previous zero-length behavior and can unexpectedly write-protect memory; return success before calculating tail when len is zero.

    tail = (start % SPU_FLASH_BLOCK_SIZE) + (uint32_t)len;
    n = (tail + SPU_FLASH_BLOCK_SIZE - 1) / SPU_FLASH_BLOCK_SIZE;

tools/unit-tests/unit-disk.c:336

This upper-bound case does not actually pin the new guard: after setting hdr_size to 513, the header CRC remains the original CRC, so a version without the hdr_size > GPT_SECTOR_SIZE check still rejects on CRC mismatch. Recompute the CRC for this 513-byte header (the fake disk has space) before calling gpt_parse_header, so deleting the upper-bound check makes the test reach the intended failure.

    d_put32(gpt_hdr + D_HDR_SIZE, GPT_SECTOR_SIZE + 1);
    ck_assert_int_eq(gpt_parse_header(gpt_hdr, &hdr), -1);

tools/unit-tests/unit-sign-encrypted-output.c:915

This explicitly sets CMD.no_base_sha = 1, so the delta base-hash branch in both header_required_size() and make_header_ex() is skipped. An under-count or alignment regression in the !no_base_sha branch would therefore still pass this advertised “all branches” test; add a delta case with a valid base hash for each digest size, or separate that branch into a focused case.

                            if (is_diff) {
                                CMD.no_base_sha = 1;
                            }

Files reviewed: 23/23 changed files
Comments generated: 5
Review effort level: Lite

Three CI failures from the new bootloader-region overlap guards: renesas_rx72n: the swap check measured the swap area as a full WOLFBOOT_PARTITION_SIZE, but swap is one sector (the pre-existing boot/update-vs-swap guards already use WOLFBOOT_SECTOR_SIZE). On rx72n that inflated swap to 0xFFFE0000+0x1F0000 and falsely overlapped the write-protect region at 0xFFFF0000. Use WOLFBOOT_SECTOR_SIZE.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #905

Scan targets checked: wolfboot-src, wolfboot-bugs

Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #905

Scan targets checked: wolfboot-src, wolfboot-bugs

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

danielinux added 16 commits September 18, 2026 04:27

Copilot AI lite review requested due to automatic review settings September 18, 2026 13:38

Copilot started reviewing on behalf of danielinux September 18, 2026 13:39 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread tools/keytools/keygen.c

Comment thread tools/unit-tests/unit-nrf5340-uart-crlf.c

Comment thread include/target.h.in Outdated

danielinux self-assigned this Sep 18, 2026

danielinux added 2 commits September 18, 2026 17:24

Address Copilot review comments

d45f344

- Add missing END_TEST after each START_TEST in unit-nrf5340-uart-crlf.c - Fix partition overlap checks in target.h.in to use standard interval intersection (catches partitions that start below WOLFBOOT_ORIGIN and extend into the bootloader region)

danielinux requested a lite review from Copilot September 18, 2026 15:41

Copilot started reviewing on behalf of danielinux September 18, 2026 15:53 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread Makefile

Comment thread include/target.h.in

Comment thread include/target.h.in Outdated

Comment thread test-app/Makefile Outdated

Comment thread include/target.h.in Outdated

danielinux requested a review from wolfSSL-Fenrir-bot September 18, 2026 17:09

wolfSSL-Fenrir-bot previously requested changes Sep 18, 2026

View reviewed changes

Comment thread hal/nrf5340.c

Comment thread tools/unit-tests/unit-nrf5340-flash-protect.c

Comment thread tools/unit-tests/unit-nsc-update.c Outdated

Address 3 Fenrir findings on PR 905

ac519da

danielinux requested a review from wolfSSL-Fenrir-bot September 18, 2026 19:40

wolfSSL-Fenrir-bot reviewed Sep 18, 2026

View reviewed changes

danielinux assigned dgarske and unassigned danielinux Sep 18, 2026

danielinux requested a review from dgarske September 18, 2026 19:49

Navigation Menu

Add missing test cases suggested by fenrir, fix two hal regressions - #905

Add missing test cases suggested by fenrir, fix two hal regressions#905
danielinux wants to merge 20 commits into
wolfSSL:masterfrom
danielinux:fenrir-fixes-2026-09-18

danielinux commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

danielinux commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #905

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #905

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants