From ac519daf1155daa6460caa54eecbf7e7996bb8a8 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 18 Sep 2026 21:37:32 +0200 Subject: [PATCH] Address 3 Fenrir findings on PR 905 --- hal/nrf5340.c | 13 ++++++- tools/unit-tests/unit-nrf5340-flash-protect.c | 34 +++++++++++++++++++ tools/unit-tests/unit-nsc-update.c | 11 +++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/hal/nrf5340.c b/hal/nrf5340.c index c552b3c8..bc307f7b 100644 --- a/hal/nrf5340.c +++ b/hal/nrf5340.c @@ -827,7 +827,13 @@ void hal_init(void) } #ifdef __WOLFBOOT -/* enable write protection for the region of flash specified */ +/* Enable write protection for the region of flash specified. + * + * Contract: protects [start, start+len). A zero len protects nothing and + * succeeds; a negative len is rejected. Protection is granted in whole + * SPU_FLASH_BLOCK_SIZE blocks, so a partial block at either end is locked + * whole - the locked range may be wider than requested, never narrower. + */ int RAMFUNCTION hal_flash_protect(haladdr_t start, int len) { /* only application core supports SPU */ @@ -840,6 +846,11 @@ int RAMFUNCTION hal_flash_protect(haladdr_t start, int len) return -1; if (len < 0) return -1; + /* An empty range protects nothing. Return before the region math below: + * `tail` carries the start offset, so an unaligned start would round up + * to one block and lock 16 KiB the caller never asked to protect. */ + if (len == 0) + return 0; /* truncate if exceeds flash size */ if (start + (uint32_t)len > FLASH_SIZE) len = FLASH_SIZE - start; diff --git a/tools/unit-tests/unit-nrf5340-flash-protect.c b/tools/unit-tests/unit-nrf5340-flash-protect.c index 5a034396..d16a9164 100644 --- a/tools/unit-tests/unit-nrf5340-flash-protect.c +++ b/tools/unit-tests/unit-nrf5340-flash-protect.c @@ -31,6 +31,7 @@ */ #include +#include #include #include #include @@ -124,6 +125,37 @@ START_TEST(test_unaligned_start_covers_range) } END_TEST +/* A zero-length range protects nothing, so no region may be locked - not + * even when start sits mid-block. The round-up introduced for F-13623 made + * `tail` carry the start offset, so an unaligned start with len 0 rounded up + * to one block and locked 16 KiB that the caller never asked for. */ +START_TEST(test_zero_len_locks_nothing) +{ + sim_reset(); + ck_assert_int_eq(hal_flash_protect(0, 0), 0); + ck_assert_int_eq(locked_count(), 0); + + sim_reset(); + ck_assert_int_eq(hal_flash_protect(0x2000, 0), 0); + ck_assert_int_eq(locked_count(), 0); +} +END_TEST + +/* A negative len is rejected outright. Without the guard the cast to + * uint32_t turns -1 into ~4 GiB, which truncates to FLASH_SIZE and locks + * every region while still returning success. */ +START_TEST(test_negative_len_rejected) +{ + sim_reset(); + ck_assert_int_eq(hal_flash_protect(0, -1), -1); + ck_assert_int_eq(locked_count(), 0); + + sim_reset(); + ck_assert_int_eq(hal_flash_protect(0x2000, INT_MIN), -1); + ck_assert_int_eq(locked_count(), 0); +} +END_TEST + /* start past the end of flash is rejected and nothing is locked. */ START_TEST(test_start_past_flash_rejected) { @@ -154,6 +186,8 @@ Suite *wolfboot_suite(void) tcase_add_test(tc, test_sub_block_len_locks_containing_block); tcase_add_test(tc, test_partial_tail_block_is_locked); tcase_add_test(tc, test_unaligned_start_covers_range); + tcase_add_test(tc, test_zero_len_locks_nothing); + tcase_add_test(tc, test_negative_len_rejected); tcase_add_test(tc, test_start_past_flash_rejected); tcase_add_test(tc, test_range_past_flash_truncated); suite_add_tcase(s, tc); diff --git a/tools/unit-tests/unit-nsc-update.c b/tools/unit-tests/unit-nsc-update.c index 465d3171..22d3067a 100644 --- a/tools/unit-tests/unit-nsc-update.c +++ b/tools/unit-tests/unit-nsc-update.c @@ -43,11 +43,17 @@ const char *argv0; +/* Per-run backing file: a fixed path is rewritten (O_TRUNC|MAP_SHARED) by + * every concurrent run, so two suites in parallel would share one mapping. + * Set once in main() before Check forks, so the child that creates the file + * and the parent that unlinks it agree on the name. */ +static char update_flash_file[PATH_MAX]; + static void prepare_update_flash(void) { int ret; - ret = mmap_file("/tmp/wolfboot-nsc-update.bin", + ret = mmap_file(update_flash_file, (void *)WOLFBOOT_PARTITION_UPDATE_ADDRESS, WOLFBOOT_PARTITION_SIZE, NULL); @@ -126,6 +132,8 @@ int main(int argc, char *argv[]) SRunner *sr; argv0 = strdup(argv[0]); + snprintf(update_flash_file, sizeof(update_flash_file), + "/tmp/wolfboot-unit-nsc-update-%d.bin", (int)getpid()); s = wolfboot_suite(); sr = srunner_create(s); #if (NO_FORK == 1) @@ -134,5 +142,6 @@ int main(int argc, char *argv[]) srunner_run_all(sr, CK_NORMAL); fails = srunner_ntests_failed(sr); srunner_free(sr); + unlink(update_flash_file); return fails; }