From 7db653b29210d9e7ba210f3233bce93a7040c051 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 17:16:39 +0200 Subject: [PATCH 01/10] F-11031: initialize PCI MMIO pool limits as exclusive ends pci_enum_do() set mem_limit/mem_pf_limit to base + length - 1, i.e. the last usable byte, while every consumer of the limits compares them as exclusive ends: pci_enum_next_aligned32() rejects a start >= limit, the BAR end check rejects a region whose end is > limit, and pci_align_check_up() rejects an aligned start >= limit. The IO pool limit (PCI_IO32_LIMIT) is already the exclusive 16-bit ceiling. With the inclusive-style init the pool effectively lost its last byte and a BAR that exactly fills a configured pool (e.g. a 128 MB non-prefetchable MMIO BAR on the default 128 MB pool) was skipped instead of mapped. Initialize the MMIO and prefetch limits as base + length and reject a pool whose end would wrap the 32-bit address space (custom PCI_MMIO32_BASE/LENGTH definitions), computed in 64 bits so the check holds on every host word size. unit-pci gains test_enum_do_pool_fill, which drives the real pci_enum_do() over a 128 MB BAR that exactly fills the default pool; pre-fix the BAR was restored to its original value (never mapped). Verification: - Built: gcc (host) unit-pci with -DWOLFBOOT_USE_PCI: clean. - Tested: unit-pci 29/29; pre-fix the new test failed with the BAR restored to 0 instead of programmed at 0x80000000. - Pitfalls: single- and multi-BAR allocations under a partially filled pool are unaffected (region end <= base + length still fits); the overflow guard only rejects pools that cannot be represented in 32-bit address space. - Style: cstyle-check.sh flag output on src/pci.c identical to the pre-change file; the new test adds one C99-decl line in the suite registration, the class the existing registrations already trip. - Message: F-11031: prefix, no co-author trailers. --- src/pci.c | 19 +++++++++++++++++-- tools/unit-tests/unit-pci.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/pci.c b/src/pci.c index 357bf9cc1d..64240ee2c2 100644 --- a/src/pci.c +++ b/src/pci.c @@ -926,11 +926,26 @@ int pci_enum_do(void) struct pci_enum_info enum_info; int ret; + /* Pool limits are exclusive ends: the allocator accepts a region + * when its end is <= limit (pci_enum_next_aligned32, the BAR end + * check, pci_align_check_up) and the IO limit is the 16-bit IO + * ceiling, not the last usable address. A region ending exactly + * at base + length must fit, so initialize base + length; reject + * a pool whose end would wrap the 32-bit address space. */ + if ((uint64_t)PCI_MMIO32_BASE + PCI_MMIO32_LENGTH > 0xFFFFFFFFULL || + (uint64_t)PCI_MMIO32_PREFETCH_BASE + + PCI_MMIO32_PREFETCH_LENGTH > 0xFFFFFFFFULL) + { + PCI_DEBUG_PRINTF("PCI MMIO pool overflows the 32-bit address " + "space\r\n"); + return -1; + } + enum_info.mem = PCI_MMIO32_BASE; - enum_info.mem_limit = enum_info.mem + (PCI_MMIO32_LENGTH - 1); + enum_info.mem_limit = enum_info.mem + PCI_MMIO32_LENGTH; enum_info.mem_pf = PCI_MMIO32_PREFETCH_BASE; enum_info.mem_pf_limit = enum_info.mem_pf + - (PCI_MMIO32_PREFETCH_LENGTH - 1); + PCI_MMIO32_PREFETCH_LENGTH; enum_info.io = PCI_IO32_BASE; enum_info.curr_bus_number = 0; diff --git a/tools/unit-tests/unit-pci.c b/tools/unit-tests/unit-pci.c index 3dd28c0e2b..bff4659613 100644 --- a/tools/unit-tests/unit-pci.c +++ b/tools/unit-tests/unit-pci.c @@ -1631,6 +1631,38 @@ START_TEST(test_enum_do_full) } END_TEST +/* test_enum_do_pool_fill: a BAR that exactly fills the configured MMIO + * pool [PCI_MMIO32_BASE, PCI_MMIO32_BASE + PCI_MMIO32_LENGTH) must be + * mapped. The pool limits are exclusive ends: the allocator accepts a + * region when its end is <= limit (pci_enum_next_aligned32, the BAR + * end check, pci_align_check_up) and the IO pool limit is the 16-bit + * ceiling, not the last usable address. Initializing the MMIO limits + * as base + length - 1 rejects this BAR and strands the last byte of + * the pool. */ +START_TEST(test_enum_do_pool_fill) +{ + struct test_pci_topology t; + int dev_node; + uint32_t bar_val; + int ret; + + test_pci_init(&t); + dev_node = test_pci_add_dev(&t, 0, 0, 0x1234, 0x5678, TEST_PCI_ROOT_BUS); + /* 128 MB MMIO BAR: exactly the default pool size */ + test_pci_dev_set_bar(&t, dev_node, 0, 0x08000000, TEST_PCI_BAR_MMIO); + test_pci_commit(&t); + + ret = pci_enum_do(); + ck_assert_int_eq(ret, 0); + + /* The BAR must be programmed at the pool base */ + bar_val = pci_config_read32(0, 0, 0, PCI_BAR0_OFFSET); + ck_assert_uint_eq(bar_val, 0x80000000); + + test_pci_cleanup(&t); +} +END_TEST + /* test_enum_do_nested_bridges: end-to-end nested bridge enumeration */ START_TEST(test_enum_do_nested_bridges) @@ -1936,6 +1968,10 @@ Suite *wolfboot_suite(void) tcase_add_test(tc_enum_nested, test_enum_do_nested_bridges); suite_add_tcase(s, tc_enum_nested); + TCase *tc_enum_pool = tcase_create("enum-do-pool-fill"); + tcase_add_test(tc_enum_pool, test_enum_do_pool_fill); + suite_add_tcase(s, tc_enum_pool); + TCase *tc_rw8 = tcase_create("config-rw-8bit-positions"); tcase_add_test(tc_rw8, test_config_rw_8bit_all_positions); suite_add_tcase(s, tc_rw8); From 7290d87657ade70ed6d3b46c46063b7d0630dd45 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 17:18:17 +0200 Subject: [PATCH 02/10] F-11046: reject bridge programming when the bus number is exhausted curr_bus_number is a uint8_t advanced once per bridge level. At 0xFF the increment wrapped to 0: pci_program_bridge() wrote SECONDARY_BUS 0 to the new bridge and then called pci_enum_bus(0), re-walking the already configured tree from the root. Every re-walk consumed the bus numbers again and reached the same wrap, so a bridge chain deep enough to exhaust the 256 bus numbers recursed without bound (stack exhaustion / boot hang) instead of degrading gracefully. Reject the bridge when curr_bus_number is already 0xFF, before the increment: the existing error path restores the saved allocator and bus state, disables the bridge window, and leaves enumeration of the remaining buses on the parent bus untouched. With the guard, nesting is bounded at 255 bridge levels, one per bus number. unit-pci gains test_program_bridge_bus_exhaustion with the two boundary cases: at 0xFE the last usable number 0xFF is assigned and the bridge is programmed; at 0xFF the call fails, the info state is restored, and the bridge registers are cleared. Pre-fix the 0xFF case returned success with the wrapped bus number. Verification: - Built: gcc (host) unit-pci with -DWOLFBOOT_USE_PCI: clean. - Tested: unit-pci 30/30; pre-fix the 0xFF case returned 0 (ret) with curr_bus_number wrapped to 1. - Pitfalls: the guard runs after the command register is read, so the error path restores a valid orig_cmd; bridges beyond the 255th level are disabled (their windows unmapped) rather than mis-programmed, which is the same outcome the OOM path already produces for a windowless bridge. - Style: cstyle-check.sh output on src/pci.c unchanged in class from the pre-change file; the new test adds one C99-decl line in the suite registration, the class the existing registrations already trip. - Message: F-11046: prefix, no co-author trailers. --- src/pci.c | 7 ++++ tools/unit-tests/unit-pci.c | 72 +++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/src/pci.c b/src/pci.c index 64240ee2c2..797cc6b55f 100644 --- a/src/pci.c +++ b/src/pci.c @@ -635,6 +635,13 @@ static int pci_program_bridge(uint8_t bus, uint8_t dev, uint8_t fun, orig_cmd = pci_config_read16(bus, dev, fun, PCI_COMMAND_OFFSET); pci_config_write16(bus, dev, fun, PCI_COMMAND_OFFSET, 0); + /* curr_bus_number is one bus per bridge level; at 0xFF the next + * increment wraps to 0, which would write SECONDARY_BUS 0 and + * re-enumerate bus 0 over the already configured tree. Disable + * this bridge instead. */ + if (info->curr_bus_number == 0xFF) + goto err; + info->curr_bus_number++; PCI_DEBUG_PRINTF("Bridge: %x.%x.%x (using bus number: %d)\r\n", (int)bus, (int)dev, (int)fun, info->curr_bus_number); diff --git a/tools/unit-tests/unit-pci.c b/tools/unit-tests/unit-pci.c index bff4659613..f75dfd7407 100644 --- a/tools/unit-tests/unit-pci.c +++ b/tools/unit-tests/unit-pci.c @@ -1539,6 +1539,74 @@ START_TEST(test_program_bridge_oom_post_enum) } END_TEST +/* test_program_bridge_bus_exhaustion: curr_bus_number is one bus per + * bridge level; at 0xFF the next increment wraps to 0, writing + * SECONDARY_BUS 0 and re-enumerating bus 0 over the already configured + * tree (unbounded recursion). The bridge at the exhaustion boundary + * must take the error path (bridge disabled, info restored); one below + * it the last usable number 0xFF is assigned. */ +START_TEST(test_program_bridge_bus_exhaustion) +{ + struct { + const char *label; + uint8_t curr; + int exp_ret; + uint8_t exp_curr; + } cases[] = { + { "last usable bus number 0xFE", 0xFE, 0, 0xFF }, + { "exhausted at 0xFF", 0xFF, -1, 0xFF }, + }; + int i; + + for (i = 0; i < (int)(sizeof(cases) / sizeof(cases[0])); i++) { + struct test_pci_topology t; + struct pci_enum_info info; + int br, ret; + uint16_t cmd_before = 0x0007; + uint8_t sec, sub; + + test_pci_init(&t); + br = test_pci_add_bridge(&t, 1, 0, 0xAAAA, 0xBBBB, TEST_PCI_ROOT_BUS); + test_pci_commit(&t); + memcpy(&t.nodes[br].cfg[PCI_COMMAND_OFFSET], &cmd_before, 2); + + memset(&info, 0, sizeof(info)); + info.mem = 0x80000000; + info.mem_limit = 0x88000000; + info.mem_pf = 0x90000000; + info.mem_pf_limit = 0xFFFFFFFF; + info.io = 0x2000; + info.curr_bus_number = cases[i].curr; + + ret = pci_program_bridge(0, 1, 0, &info); + ck_assert_msg(ret == cases[i].exp_ret, + "%s: ret", cases[i].label); + ck_assert_msg(info.curr_bus_number == cases[i].exp_curr, + "%s: curr_bus_number", cases[i].label); + + /* command register restored on both paths */ + ck_assert_msg(pci_config_read16(0, 1, 0, PCI_COMMAND_OFFSET) + == cmd_before, "%s: cmd", cases[i].label); + + if (cases[i].exp_ret != 0) { + /* the exhausted bridge must be left disabled */ + sec = pci_config_read8(0, 1, 0, PCI_SECONDARY_BUS); + sub = pci_config_read8(0, 1, 0, PCI_SUB_SEC_BUS); + ck_assert_msg(sec == 0, "%s: secondary not cleared", + cases[i].label); + ck_assert_msg(sub == 0, "%s: subordinate not cleared", + cases[i].label); + } + else { + ck_assert_msg(pci_config_read8(0, 1, 0, PCI_SECONDARY_BUS) + == 0xFF, "%s: secondary", cases[i].label); + } + + test_pci_cleanup(&t); + } +} +END_TEST + /* test_enum_bus_topology: device dispatch + multifunction handling */ START_TEST(test_enum_bus_topology) { @@ -1956,6 +2024,10 @@ Suite *wolfboot_suite(void) tcase_add_test(tc_oom_post, test_program_bridge_oom_post_enum); suite_add_tcase(s, tc_oom_post); + TCase *tc_bus_exhaust = tcase_create("bridge-bus-exhaustion"); + tcase_add_test(tc_bus_exhaust, test_program_bridge_bus_exhaustion); + suite_add_tcase(s, tc_bus_exhaust); + TCase *tc_enum_topo = tcase_create("enum-bus-topology"); tcase_add_test(tc_enum_topo, test_enum_bus_topology); suite_add_tcase(s, tc_enum_topo); From 300e5db11051ae7a39feb73fd071753f7b5890f6 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 17:22:17 +0200 Subject: [PATCH 03/10] F-11036: advance the page address in the SAMR21 erase loop hal_flash_erase() used the length decrement as the unbraced body of the NVMREADY wait loop. With the peripheral idle (NVMREADY set) the wait body never ran, the length never shrank, and the outer loop re-erased the first page of the range forever; whatever the wait duration, the number of decrements tracked wait-loop iterations instead of completed erases, and the address was never advanced, so later pages of the requested range were never erased. Brace the ready wait, and after a completed erase advance the address by FLASH_PAGESIZE and decrement the length once, as the sibling P1021 multi-block erase loop does (F-11034). unit-samr21-erase-advance extracts the real function and register macros and runs it against a host NVMCTRL window with NVMREADY preset (an idle peripheral): a 128-byte range must end with page 0x1040 programmed, a 256-byte range with page 0xC0, and a single 64-byte erase must complete. Pre-fix all three cases hang in the re-erase loop and fail on the tcase timeout. Verification: - Built: arm-none-eabi-gcc -fsyntax-only -Wall -Wextra hal/samr21.c: clean. - Tested: unit-samr21-erase-advance 3/3; pre-fix all three timed out (10 s tcase limit). - Pitfalls: single-page erases and page-aligned ranges behave as before; a non-page-multiple len erases the final partial page's page, unchanged from the pre-existing decrement semantics. - Style: cstyle-check.sh FMT diff on hal/samr21.c byte-identical to the pre-change file; the new test trips only the uncrustify START_TEST brace class the sibling unit tests trip. - Unverified: no SAMR21 board execution. - Message: F-11036: prefix, no co-author trailers. --- hal/samr21.c | 3 +- tools/unit-tests/Makefile | 21 +++ tools/unit-tests/unit-samr21-erase-advance.c | 133 +++++++++++++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 tools/unit-tests/unit-samr21-erase-advance.c diff --git a/hal/samr21.c b/hal/samr21.c index d9c4f19538..98e57bf284 100644 --- a/hal/samr21.c +++ b/hal/samr21.c @@ -210,7 +210,8 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) while (len > 0) { NVMCTRL_ADDR = (address >> 1); /* This register holds the address of a 16-bit row */ NVMCTRLA_REG = NVMCMD_ERASE | NVMCMD_KEY; - while(!(NVMCTRL_INTFLAG & NVMCTRL_INTFLAG_NVMREADY)) + while (!(NVMCTRL_INTFLAG & NVMCTRL_INTFLAG_NVMREADY)) { } + address += FLASH_PAGESIZE; len -= FLASH_PAGESIZE; } return 0; diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index f09f32e513..33463a3653 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -115,6 +115,7 @@ TESTS+=unit-versal-ext-write TESTS+=unit-t10xx-qe-firmware TESTS+=unit-t10xx-flash-status TESTS+=unit-p1021-erase-advance +TESTS+=unit-samr21-erase-advance TESTS+=unit-hifive1-flash-write TESTS+=unit-fwtpm-rsp-overrun TESTS+=unit-fwtpm-cmd-toctou @@ -1078,6 +1079,26 @@ unit-p1021-erase-advance: unit-p1021-erase-advance.c p1021_erase_extract.h \ p1021_erase_fn_extract.h gcc -o $@ unit-p1021-erase-advance.c $(CFLAGS) $(LDFLAGS) +# unit-samr21-erase-advance runs the real hal_flash_erase() from +# hal/samr21.c against a host NVMCTRL register window (F-11036: the +# length decrement was the body of the NVMREADY wait and the address +# never advanced, so the loop re-erased the first page forever). +samr21_erase_extract.h: ../../hal/samr21.c + sed -n '/#define FLASH_PAGESIZE /p' $< > $@ + sed -n '/#define NVMCTRLA_REG /p' $< >> $@ + sed -n '/#define NVMCTRL_INTFLAG /p' $< >> $@ + sed -n '/#define NVMCTRL_ADDR /p' $< >> $@ + sed -n '/#define NVMCMD_KEY /p' $< >> $@ + sed -n '/#define NVMCMD_ERASE /p' $< >> $@ + sed -n '/#define NVMCTRL_INTFLAG_NVMREADY /p' $< >> $@ + +samr21_erase_fn_extract.h: ../../hal/samr21.c + sed -n '/^int RAMFUNCTION hal_flash_erase/,/^}/p' $< > $@ + +unit-samr21-erase-advance: unit-samr21-erase-advance.c samr21_erase_extract.h \ + samr21_erase_fn_extract.h + gcc -o $@ unit-samr21-erase-advance.c $(CFLAGS) $(LDFLAGS) + # unit-hifive1-flash-write runs the real hal_flash_write() from # hal/hifive1.c against a mock fespi model (F-11035: the final partial # page of a multi-page write took the full-page branch, over-reading diff --git a/tools/unit-tests/unit-samr21-erase-advance.c b/tools/unit-tests/unit-samr21-erase-advance.c new file mode 100644 index 0000000000..d9570d83eb --- /dev/null +++ b/tools/unit-tests/unit-samr21-erase-advance.c @@ -0,0 +1,133 @@ +/* unit-samr21-erase-advance.c + * + * Regression test for F-11036: hal_flash_erase() in hal/samr21.c + * decremented the remaining length as the body of the NVMREADY wait + * loop instead of once per completed erase, and never advanced the + * address. With the peripheral idle (NVMREADY set) the wait body never + * ran, the length never shrank, and the loop re-erased the same page + * forever; the rest of the requested range was never reached. + * + * hal/samr21.c needs the SAMR21 SDK headers and cannot be built on the + * host, so the Makefile extracts the register macros and the function + * verbatim. The NVMCTRL register window is a host array with NVMREADY + * preset (an idle peripheral whose erases complete immediately), and + * the test checks the page left programmed by the loop. + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include + +/* The extracted function is a RAMFUNCTION; on the host that is nothing. */ +#define RAMFUNCTION + +/* Host mock of the NVMCTRL register window. The extracted macros index + * it through NVMCTRL_BASE: command @ 0x0, INTFLAG @ 0x14, ADDR @ 0x1c. */ +static uint8_t g_nvm[0x20]; +#define NVMCTRL_BASE ((uintptr_t)g_nvm) + +/* NVMCTRL register macros + command codes + page size from + * hal/samr21.c (extracted by the Makefile). */ +#include "samr21_erase_extract.h" + +static void mock_reset(void) +{ + memset(g_nvm, 0, sizeof(g_nvm)); + /* Idle peripheral: the last erase completed, NVMREADY is set. */ + g_nvm[0x14] = NVMCTRL_INTFLAG_NVMREADY; +} + +/* The real hal_flash_erase() from hal/samr21.c (extracted). */ +#include "samr21_erase_fn_extract.h" + +/* A 64-byte page is one erase; a multi-page range must end with the + * last page of the range programmed, not the first. */ +START_TEST (test_erase_multi_page_advances) +{ + int ret; + + mock_reset(); + + ret = hal_flash_erase(0x1000, 128); + + ck_assert_int_eq(ret, 0); + /* Two 64-byte pages: 0x1000 then 0x1040. The programmed row + * register holds (byte address) >> 1; the final one must be the + * last page of the range. Pre-fix the loop re-erased page 0x1000 + * forever (this case times out). */ + ck_assert_uint_eq(NVMCTRL_ADDR, (0x1040 >> 1)); +} +END_TEST + +/* Four pages from the start of the flash must advance to the last one. */ +START_TEST (test_erase_four_pages_advances) +{ + int ret; + + mock_reset(); + + ret = hal_flash_erase(0, 256); + + ck_assert_int_eq(ret, 0); + ck_assert_uint_eq(NVMCTRL_ADDR, (192u >> 1)); +} +END_TEST + +/* A single-page erase completes and leaves its page programmed. */ +START_TEST (test_erase_single_page) +{ + int ret; + + mock_reset(); + + ret = hal_flash_erase(0x2000, 64); + + ck_assert_int_eq(ret, 0); + ck_assert_uint_eq(NVMCTRL_ADDR, (0x2000 >> 1)); +} +END_TEST + +Suite *samr21_erase_suite(void) +{ + Suite *s = suite_create("samr21 erase advance"); + TCase *tc = tcase_create("erase-address"); + + tcase_add_test(tc, test_erase_multi_page_advances); + tcase_add_test(tc, test_erase_four_pages_advances); + tcase_add_test(tc, test_erase_single_page); + /* Pre-fix the idle-peripheral model hangs in the re-erase loop; + * the tcase timeout is what turns that hang into a failure. */ + tcase_set_timeout(tc, 10); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + Suite *s = samr21_erase_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return fails; +} From c47135746adc04bc5cc60f82b53aa4861cc3e9e5 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 17:31:56 +0200 Subject: [PATCH 04/10] F-11032: bound the ACMD41 OCR readiness poll in SD card init sdcard_card_full_init() polled ACMD41 in an unbounded do/while until the card set OCR ready, so a card that answers every ACMD41 without ever setting the bit held the bootloader in the loop forever. F-7984 bounded the separate DATA0/CMD13 waits in sdhci_wait_busy(); this is the OCR readiness path, which still had no limit. Bound the poll with the same shape as sdhci_wait_busy(): a 30000 ms budget (SDCARD_ACMD41_TIMEOUT_MS, #ifndef-able) measured against hal_get_timer_us(), the watchdog serviced inside the loop, and -1 returned to fail the SD boot path. A healthy card reports ready in milliseconds, so the budget is far above any real initialization time. unit-sdhci-acmd41-timeout compiles the real driver (generated sdhci_host.c, as in the wait-busy test) against a scripted controller: commands complete without error, SRS12 is modeled write-1-to-clear, and the card model sets OCR ready after a configurable number of ACMD41 polls. A never-ready card must return -1 inside the shipped budget (and service the watchdog); a card ready after 5 polls must exit the loop promptly and proceed to the end of the init path. A command-write cap turns the pre-fix infinite loop into an abort instead of a hung build. Verification: - Built: gcc (host) unit-sdhci-acmd41-timeout with -DDISK_SDCARD: clean. - Tested: unit-sdhci-acmd41-timeout 2/2; pre-fix the never-ready case aborted at the 50001st command (the loop never terminates); post-fix it runs 3001 polls, reaches the 30 s budget, pets the watchdog every iteration and returns -1. - Pitfalls: the timeout returns -1 from the SD path, the same contract as a failed CMD0/CMD8; the budget is per init call, not shared with sdhci_wait_busy, and a card that becomes ready before the deadline is unaffected. - Style: cstyle-check.sh FMT diff on src/sdhci.c byte-identical to the pre-change file; the new test is flag-free. - Unverified: no SD card hardware execution. - Message: F-11032: prefix, no co-author trailers. --- src/sdhci.c | 21 +- tools/unit-tests/Makefile | 11 + tools/unit-tests/unit-sdhci-acmd41-timeout.c | 231 +++++++++++++++++++ 3 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 tools/unit-tests/unit-sdhci-acmd41-timeout.c diff --git a/src/sdhci.c b/src/sdhci.c index 242425508d..af046f2441 100644 --- a/src/sdhci.c +++ b/src/sdhci.c @@ -829,6 +829,13 @@ static int sdcard_card_init(uint32_t acmd41_arg, uint32_t *ocr_reg) static int sdcard_set_bus_width(uint32_t bus_width); static int sdcard_set_function(uint32_t function_number, uint32_t group_number); +/* A card that answers ACMD41 forever without setting OCR ready must + * not hold the boot. The budget matches the sdhci_wait_busy() wait; + * a healthy card reports ready in milliseconds. */ +#ifndef SDCARD_ACMD41_TIMEOUT_MS +#define SDCARD_ACMD41_TIMEOUT_MS 30000 +#endif + /* Full SD card initialization sequence * Returns 0 on success */ static int sdcard_card_full_init(void) @@ -898,6 +905,9 @@ static int sdcard_card_full_init(void) } if (status == 0) { + uint64_t start = hal_get_timer_us(); + const uint64_t timeout_us = + (uint64_t)SDCARD_ACMD41_TIMEOUT_MS * 1000U; /* configure operating conditions */ uint32_t cmd_arg = SDCARD_ACMD41_HCS; cmd_arg |= card_volts; @@ -911,10 +921,17 @@ static int sdcard_card_full_init(void) wolfBoot_printf("sdcard_init: sending OCR arg: 0x%08X\n", cmd_arg); #endif - /* retry until OCR ready */ + /* retry until OCR ready; a card that never sets it must not + * hold the boot, so bound the poll like sdhci_wait_busy() and + * service the watchdog inside it */ do { status = sdcard_card_init(cmd_arg, ®); - } while (status == 0 && (reg & SDCARD_REG_OCR_READY) == 0); + if (status != 0 || (reg & SDCARD_REG_OCR_READY) != 0) + break; + sdhci_platform_wdt_pet(); + if (hal_get_timer_us() - start > timeout_us) + status = -1; + } while (status == 0); } if (status == 0) { diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 33463a3653..1483b3fcc2 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -130,6 +130,7 @@ TESTS+=unit-stm32u5-write TESTS+=unit-nvm-cache-scrub TESTS+=unit-sdhci-uhs-recover TESTS+=unit-sdhci-wait-busy +TESTS+=unit-sdhci-acmd41-timeout TESTS+=unit-ti-hercules-write TESTS+=unit-p1021-qe-firmware TESTS+=unit-t10xx-dts-memac @@ -1212,6 +1213,16 @@ unit-sdhci-uhs-recover: unit-sdhci-uhs-recover.c sdhci_host.c unit-sdhci-wait-busy: unit-sdhci-wait-busy.c sdhci_host.c gcc -o $@ unit-sdhci-wait-busy.c -DDISK_SDCARD -DWOLFBOOT_NO_PRINTF $(CFLAGS) $(LDFLAGS) +# unit-sdhci-acmd41-timeout drives sdcard_card_full_init()'s ACMD41 OCR +# readiness poll from the real src/sdhci.c (F-11032: the do/while had no +# bound, so a card answering ACMD41 without ever setting OCR ready held +# the boot forever). Same sdhci_host.c generation as the wait-busy test; +# the card model sets OCR ready after N scripted ACMD41 polls, the timer +# stub steps 10 ms per read, and a command-write cap turns the pre-fix +# infinite loop into an abort. +unit-sdhci-acmd41-timeout: unit-sdhci-acmd41-timeout.c sdhci_host.c + gcc -o $@ unit-sdhci-acmd41-timeout.c -DDISK_SDCARD -DWOLFBOOT_NO_PRINTF $(CFLAGS) $(LDFLAGS) + # unit-ti-hercules-write runs the real hal_flash_write() staging logic # from hal/ti_hercules.c (a short write crossing a block # boundary overran the staging buffer and lost the next block's bytes). diff --git a/tools/unit-tests/unit-sdhci-acmd41-timeout.c b/tools/unit-tests/unit-sdhci-acmd41-timeout.c new file mode 100644 index 0000000000..7f90700311 --- /dev/null +++ b/tools/unit-tests/unit-sdhci-acmd41-timeout.c @@ -0,0 +1,231 @@ +/* unit-sdhci-acmd41-timeout.c + * + * Regression test for F-11032: sdcard_card_full_init() polled ACMD41 + * in an unbounded do/while until the card set OCR ready, so a card + * that answers every ACMD41 without ever setting the bit held the + * bootloader in the loop forever. (F-7984 bounded the separate + * DATA0/CMD13 waits in sdhci_wait_busy(); this is the OCR readiness + * path.) + * + * The real driver is compiled from the generated sdhci_host.c + * (identical to src/sdhci.c except the three x86-incompatible asm + * statements are blanked and sdhci_read() is renamed to + * sdhci_read_hw()); the controller is scripted through the host + * register file. The card model sets OCR READY after a configurable + * number of ACMD41 polls (0 = never). The command-write cap turns the + * pre-fix infinite loop into an abort instead of a hung CI. + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include +#include + +#include "sdhci.h" +#include "disk.h" + +/* Host register file for the platform-provided register accessors. */ +static uint32_t g_sdhci_regs[0x400 / sizeof(uint32_t)]; + +/* Command-write cap: the pre-fix ACMD41 loop never terminates, so the + * cap aborts the test (a failure) instead of hanging the build. The + * fixed loop reaches the shipped 30 s budget in well under the cap. */ +static uint32_t g_cmd_writes; +#define CMD_WRITE_CAP 50000 + +/* Card model: after g_ready_after ACMD41 polls the card sets OCR + * ready (0 = the card never becomes ready). */ +static uint32_t g_ready_after; +static uint32_t g_acmd41_polls; + +uint32_t sdhci_reg_read(uint32_t offset) +{ + return g_sdhci_regs[offset / sizeof(uint32_t)]; +} + +void sdhci_reg_write(uint32_t offset, uint32_t val) +{ + if (offset == SDHCI_SRS12) { + /* interrupt status register: write-1-to-clear */ + g_sdhci_regs[offset / sizeof(uint32_t)] &= ~val; + return; + } + + g_sdhci_regs[offset / sizeof(uint32_t)] = val; + + if (offset == SDHCI_SRS03) { + uint32_t idx; + + /* the scripted controller completes every command without + * error; the response is read from the preset SRS04 */ + g_sdhci_regs[SDHCI_SRS12 / sizeof(uint32_t)] |= SDHCI_SRS12_CC; + + g_cmd_writes++; + if (g_cmd_writes > CMD_WRITE_CAP) + ck_abort_msg("ACMD41 poll ran away: %u commands", + g_cmd_writes); + + idx = (val & SDHCI_SRS03_CIDX_MASK) >> SDHCI_SRS03_CIDX_SHIFT; + if (idx == SD_ACMD41_SEND_OP_COND && g_ready_after > 0) { + g_acmd41_polls++; + if (g_acmd41_polls >= g_ready_after) { + g_sdhci_regs[SDHCI_SRS04 / sizeof(uint32_t)] |= + SDCARD_REG_OCR_READY; + g_ready_after = 0; + } + } + } +} + +/* Timer: advance by g_timer_step microseconds on every read, so the + * shipped SDCARD_ACMD41_TIMEOUT_MS is exercised as built. */ +static uint64_t g_timer_us; +static uint64_t g_timer_step = 10000; +uint64_t hal_get_timer_us(void) +{ + g_timer_us += g_timer_step; + return g_timer_us; +} + +/* Counts watchdog services performed inside the poll loop. */ +static unsigned int g_wdt_pets; +void sdhci_platform_wdt_pet(void) +{ + g_wdt_pets++; +} + +/* Platform hooks the SD init path touches. */ +void sdhci_platform_init(void) +{ +} + +void sdhci_platform_irq_init(void) +{ +} + +void sdhci_platform_set_bus_mode(int is_emmc) +{ + (void)is_emmc; +} + +/* The real driver (see the Makefile for the transforms). */ +#include "sdhci_host.c" + +/* The data path is not under test; the stub ends the init sequence + * after the ACMD41 loop, the bus-width setup and the SCR read. */ +int sdhci_read(uint32_t cmd_index, uint32_t block_addr, uint32_t *dst, + uint32_t sz) +{ + (void)cmd_index; (void)block_addr; (void)dst; (void)sz; + return -1; +} + +/* Script the controller for a card present at 3.3 V: every command + * completes immediately (SRS12 CC preset), responses come from the + * preset SRS04 (READY_FOR_DATA for R1, voltage bits for the OCR), + * the host is 3.3-V capable, and no 1.8-V / UHS features are + * advertised so the init stays on the plain path. */ +static void script_card_present(void) +{ + g_sdhci_regs[SDHCI_SRS12 / sizeof(uint32_t)] = SDHCI_SRS12_CC; + g_sdhci_regs[SDHCI_SRS04 / sizeof(uint32_t)] = + (1U << 8) | SDCARD_REG_OCR_3_3_3_4; + g_sdhci_regs[SDHCI_SRS16 / sizeof(uint32_t)] = SDHCI_SRS16_VS33; + g_sdhci_regs[SDHCI_SRS18 / sizeof(uint32_t)] = 0; /* no 1.8V current */ +} + +static void setup(void) +{ + memset(g_sdhci_regs, 0, sizeof(g_sdhci_regs)); + g_timer_us = 0; + g_timer_step = 10000; /* 10 ms per timer read */ + g_wdt_pets = 0; + g_cmd_writes = 0; + g_acmd41_polls = 0; + g_ready_after = 0; +} + +static void teardown(void) +{ +} + +/* A card that answers ACMD41 forever without setting OCR ready must + * time out with an error instead of holding the boot. Pre-fix the + * loop never terminates and the command-write cap aborts. */ +START_TEST(test_acmd41_never_ready_times_out) +{ + int ret; + + script_card_present(); + g_ready_after = 0; /* never ready */ + + ret = sdcard_card_full_init(); + + ck_assert_int_eq(ret, -1); + /* the poll ran to the shipped budget, so it must have petted */ + ck_assert_uint_gt(g_wdt_pets, 0); + /* and stayed far below the runaway cap */ + ck_assert_uint_lt(g_cmd_writes, CMD_WRITE_CAP); +} +END_TEST + +/* A card that reports ready after a few polls must initialize: the + * loop exits promptly (well under the cap) and the sequence proceeds + * to the end of the init path (the stubbed SCR read ends it). */ +START_TEST(test_acmd41_ready_after_polls) +{ + int ret; + + script_card_present(); + g_ready_after = 5; + + ret = sdcard_card_full_init(); + + /* the loop must have exited on OCR ready, not on the timeout */ + ck_assert_uint_eq(g_acmd41_polls, 5); + ck_assert_uint_lt(g_cmd_writes, 1000); + /* the init ran to the stubbed data path, which fails by design */ + ck_assert_int_eq(ret, -1); +} +END_TEST + +Suite *sdhci_acmd41_suite(void) +{ + Suite *s = suite_create("sdhci-acmd41-timeout"); + TCase *tc = tcase_create("acmd41-timeout"); + + tcase_add_checked_fixture(tc, setup, teardown); + tcase_add_test(tc, test_acmd41_never_ready_times_out); + tcase_add_test(tc, test_acmd41_ready_after_polls); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + Suite *s = sdhci_acmd41_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return fails; +} From 9b89b5e643d2f93a1c595695d67ed7f2315cc39b Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 17:35:22 +0200 Subject: [PATCH 05/10] F-11037: scrub NVM_CACHE after the write-once update trigger Under NVM_FLASH_WRITEONCE wolfBoot_update_trigger() stages a whole flash sector into the file-scope NVM_CACHE before rewriting the update partition flags. In EXT_ENCRYPTED builds that sector is where the firmware key/nonce live (ENCRYPT_CACHE aliases NVM_CACHE, and with FLAGS_HOME the update flags sit in the boot trailer), so after an update trigger the plaintext key material sat in the buffer at a fixed address. The partition-trailer helpers scrub the buffer with nvm_cache_scrub() on return (F-9765); the write-once update path copied the sector and never scrubbed it. Scrub the staged sector after the final erase, before the flash lock is released. unit-update-trigger-scrub extracts the real function together with nvm_cache_scrub() (Makefile, built with NVM_FLASH_WRITEONCE) and runs the write-once branch over a staged sector carrying a key/nonce pattern: one flags write and two sector erases are expected, and the buffer must be zero after the call. Pre-fix the staged pattern remained in NVM_CACHE. Verification: - Built: gcc (host) unit-update-trigger-scrub with -DNVM_FLASH_WRITEONCE: clean, no warnings. - Tested: unit-update-trigger-scrub 1/1 (pre-fix: key pattern remained); unit-nvm-cache-scrub 3/3 (the non-write-once extraction build is unaffected). - Pitfalls: the scrub runs unconditionally in the write-once branch, which has no early-return flash-error path; the non-write-once and external-flash branches stage nothing and are unchanged. - Style: cstyle-check.sh FMT diff on src/libwolfboot.c byte-identical to the pre-change file; the new test is warning-free. - Message: F-11037: prefix, no co-author trailers. --- src/libwolfboot.c | 5 + tools/unit-tests/Makefile | 14 ++ tools/unit-tests/unit-update-trigger-scrub.c | 184 +++++++++++++++++++ 3 files changed, 203 insertions(+) create mode 100644 tools/unit-tests/unit-update-trigger-scrub.c diff --git a/src/libwolfboot.c b/src/libwolfboot.c index b52172b9e3..46c8e7eeaa 100644 --- a/src/libwolfboot.c +++ b/src/libwolfboot.c @@ -914,6 +914,11 @@ void RAMFUNCTION wolfBoot_update_trigger(void) /* erase the previously selected sector */ hal_flash_erase(lastSector - WOLFBOOT_SECTOR_SIZE * selSec, WOLFBOOT_SECTOR_SIZE); + /* The staged sector may hold the firmware key/nonce (see + * ENCRYPT_CACHE under NVM_FLASH_WRITEONCE): scrub it, as the + * partition-trailer helpers do, before releasing the flash + * lock. */ + nvm_cache_scrub(); #endif } diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 1483b3fcc2..d1b0064c4a 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -128,6 +128,7 @@ TESTS+=unit-stm32g4-write TESTS+=unit-stm32l5-write TESTS+=unit-stm32u5-write TESTS+=unit-nvm-cache-scrub +TESTS+=unit-update-trigger-scrub TESTS+=unit-sdhci-uhs-recover TESTS+=unit-sdhci-wait-busy TESTS+=unit-sdhci-acmd41-timeout @@ -1186,6 +1187,19 @@ nvm_cache_scrub_extract.h: ../../src/libwolfboot.c unit-nvm-cache-scrub: unit-nvm-cache-scrub.c nvm_cache_scrub_extract.h gcc -o $@ unit-nvm-cache-scrub.c $(CFLAGS) $(LDFLAGS) +# unit-update-trigger-scrub runs the real wolfBoot_update_trigger() +# from src/libwolfboot.c over the NVM_FLASH_WRITEONCE branch +# (F-11037: the staged trailer sector, which in EXT_ENCRYPTED builds +# carries the firmware key/nonce, was never scrubbed from NVM_CACHE). +# The function is extracted together with nvm_cache_scrub(); the build +# defines NVM_FLASH_WRITEONCE so the write-once branch compiles. +update_trigger_scrub_extract.h: ../../src/libwolfboot.c + sed -n '/^static void RAMFUNCTION nvm_cache_scrub(/,/^}/p' $< > $@ + sed -n '/^void RAMFUNCTION wolfBoot_update_trigger(/,/^}/p' $< >> $@ + +unit-update-trigger-scrub: unit-update-trigger-scrub.c update_trigger_scrub_extract.h + gcc -o $@ unit-update-trigger-scrub.c -DNVM_FLASH_WRITEONCE $(CFLAGS) $(LDFLAGS) + # unit-sdhci-uhs-recover drives disk_read()'s UHS recovery path from the # real src/sdhci.c (any read error permanently switched the host # to 1.8V signaling with no rollback). sdhci_host.c (generated below) is diff --git a/tools/unit-tests/unit-update-trigger-scrub.c b/tools/unit-tests/unit-update-trigger-scrub.c new file mode 100644 index 0000000000..e35ec8e986 --- /dev/null +++ b/tools/unit-tests/unit-update-trigger-scrub.c @@ -0,0 +1,184 @@ +/* unit-update-trigger-scrub.c + * + * Regression test for F-11037: under NVM_FLASH_WRITEONCE, + * wolfBoot_update_trigger() stages a whole flash sector into the + * file-scope NVM_CACHE before rewriting the update partition flags. + * In EXT_ENCRYPTED builds that sector is where the firmware + * key/nonce live (ENCRYPT_CACHE aliases NVM_CACHE, and with + * FLAGS_HOME the update flags sit in the boot trailer), so after an + * update trigger the plaintext key material sat in the buffer until + * the next use. The partition-trailer helpers scrub with + * nvm_cache_scrub() (F-9765); the write-once update path did not. + * + * The real function is extracted by the Makefile (together with + * nvm_cache_scrub()) and run with a test-owned NVM_CACHE, a staged + * sector carrying a key pattern and stubbed flash calls; the buffer + * must be zero after the call. + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include +#include + +#define RAMFUNCTION +#define WOLFBOOT_SECTOR_SIZE 4096 +#define NVM_CACHE_SIZE WOLFBOOT_SECTOR_SIZE +#define XMEMCPY(a, b, n) memcpy((a), (b), (n)) +#define PART_UPDATE 0 +#define IMG_STATE_UPDATING 0x8F +#define WOLFBOOT_MAGIC_TRAIL 0x0000DEAD +#define SECTOR_FLAGS_SIZE 5 +#define FLAGS_UPDATE_EXT() 0 + +/* The buffer under test (real: file-scope in src/libwolfboot.c). */ +static uint8_t NVM_CACHE[NVM_CACHE_SIZE]; + +/* The staged sector image: the boot/update trailer sector carrying + * the firmware key/nonce pattern at a fixed offset. */ +#define KEY_OFF 0x0F00 +#define KEY_LEN 64 +static uint8_t g_sector[NVM_CACHE_SIZE]; + +/* The update partition flags end at the top of the staged sector, so + * lastSector in wolfBoot_update_trigger() resolves to g_sector. */ +#define PART_UPDATE_ENDFLAGS ((uintptr_t)(g_sector + WOLFBOOT_SECTOR_SIZE)) + +/* Stubbed flash layer: records calls. */ +static int g_flash_writes; +static int g_flash_erases; + +int hal_flash_write(uint32_t address, const uint8_t *data, int len) +{ + (void)address; (void)data; (void)len; + g_flash_writes++; + return 0; +} + +int hal_flash_erase(uint32_t address, int len) +{ + (void)address; (void)len; + g_flash_erases++; + return 0; +} + +void hal_flash_unlock(void) +{ +} + +void hal_flash_lock(void) +{ +} + +/* External-flash stubs: not taken (FLAGS_UPDATE_EXT() == 0) but + * referenced, so they must link. */ +void ext_flash_unlock(void) +{ +} + +void ext_flash_lock(void) +{ +} + +void ext_flash_erase(uintptr_t address, int len) +{ + (void)address; (void)len; +} + +int nvm_select_fresh_sector(int part) +{ + (void)part; + return 0; +} + +int wolfBoot_set_partition_state(uint8_t part, uint8_t newst) +{ + (void)part; (void)newst; + return 0; +} + +/* The real functions from src/libwolfboot.c (extracted by the + * Makefile). */ +#include "update_trigger_scrub_extract.h" + +static int cache_scrubbed(void) +{ + int i; + + for (i = 0; i < NVM_CACHE_SIZE; i++) + if (NVM_CACHE[i] != 0) + return 0; + return 1; +} + +static void setup(void) +{ + memset(g_sector, 0x11, sizeof(g_sector)); + memset(g_sector + KEY_OFF, 0xA5, KEY_LEN); /* key/nonce pattern */ + memset(NVM_CACHE, 0, sizeof(NVM_CACHE)); + g_flash_writes = 0; + g_flash_erases = 0; +} + +static void teardown(void) +{ +} + +/* wolfBoot_update_trigger() must leave NVM_CACHE scrubbed: the + * write-once path staged the whole trailer sector (key included) + * before rewriting the flags. Pre-fix the staged pattern remained. */ +START_TEST(test_update_trigger_scrubs_cache) +{ + ck_assert_int_eq(cache_scrubbed(), 1); + + wolfBoot_update_trigger(); + + /* the write-once path wrote the fresh flags sector and erased + * both candidate sectors */ + ck_assert_int_eq(g_flash_writes, 1); + ck_assert_int_eq(g_flash_erases, 2); + ck_assert_int_eq(cache_scrubbed(), 1); +} +END_TEST + +Suite *update_trigger_scrub_suite(void) +{ + Suite *s = suite_create("update-trigger-scrub"); + TCase *tc = tcase_create("update-trigger-scrub"); + + tcase_add_checked_fixture(tc, setup, teardown); + tcase_add_test(tc, test_update_trigger_scrubs_cache); + suite_add_tcase(s, tc); + + return s; +} + +int main(void) +{ + int fails; + Suite *s = update_trigger_scrub_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + + return fails; +} From 00cbbaeebbad3a0cf0f79442f2c4abd10732f259 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 18:55:05 +0200 Subject: [PATCH 06/10] samr21: advance flash erase by the 256-byte row size NVMCMD_ERASE (0x02) is the NVMCTRL row erase: one command erases a 256-byte row (4 pages). The erase loop advanced the address by one page per iteration, so it issued four row-erase commands against the same row - the second through fourth with a non-row-aligned address - quadrupling erase time and wear on every flash erase. Stride the loop by FLASH_ROW_SIZE (4 * FLASH_PAGESIZE). The row containing a sub-row request is erased once, as the command granularity requires. Unit test updated to row semantics: a sub-row request ends on the containing row, a two-row range advances to the second row, and an exact row erases once with no extra row. All three fail on the page-stride loop. --- hal/samr21.c | 7 ++- tools/unit-tests/Makefile | 1 + tools/unit-tests/unit-samr21-erase-advance.c | 62 +++++++++++--------- 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/hal/samr21.c b/hal/samr21.c index 98e57bf284..0ad4945e2b 100644 --- a/hal/samr21.c +++ b/hal/samr21.c @@ -39,6 +39,9 @@ #define FLASH_SIZE (256 * 1024) #define FLASH_PAGESIZE 64 #define FLASH_N_PAGES 4096 +/* NVMCMD_ERASE (0x02) is the NVMCTRL row erase: one command erases a + * 256-byte row (4 pages), so erase loops stride by the row size. */ +#define FLASH_ROW_SIZE (4 * FLASH_PAGESIZE) #define WDT_CTRL *((volatile uint8_t *)(0x40001000)) #define WDT_EN (1 << 1) @@ -211,8 +214,8 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) NVMCTRL_ADDR = (address >> 1); /* This register holds the address of a 16-bit row */ NVMCTRLA_REG = NVMCMD_ERASE | NVMCMD_KEY; while (!(NVMCTRL_INTFLAG & NVMCTRL_INTFLAG_NVMREADY)) { } - address += FLASH_PAGESIZE; - len -= FLASH_PAGESIZE; + address += FLASH_ROW_SIZE; + len -= FLASH_ROW_SIZE; } return 0; } diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index d1b0064c4a..39dcc8f3a9 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -1087,6 +1087,7 @@ unit-p1021-erase-advance: unit-p1021-erase-advance.c p1021_erase_extract.h \ # never advanced, so the loop re-erased the first page forever). samr21_erase_extract.h: ../../hal/samr21.c sed -n '/#define FLASH_PAGESIZE /p' $< > $@ + sed -n '/#define FLASH_ROW_SIZE /p' $< >> $@ sed -n '/#define NVMCTRLA_REG /p' $< >> $@ sed -n '/#define NVMCTRL_INTFLAG /p' $< >> $@ sed -n '/#define NVMCTRL_ADDR /p' $< >> $@ diff --git a/tools/unit-tests/unit-samr21-erase-advance.c b/tools/unit-tests/unit-samr21-erase-advance.c index d9570d83eb..987af43271 100644 --- a/tools/unit-tests/unit-samr21-erase-advance.c +++ b/tools/unit-tests/unit-samr21-erase-advance.c @@ -1,17 +1,21 @@ /* unit-samr21-erase-advance.c * - * Regression test for F-11036: hal_flash_erase() in hal/samr21.c - * decremented the remaining length as the body of the NVMREADY wait - * loop instead of once per completed erase, and never advanced the - * address. With the peripheral idle (NVMREADY set) the wait body never - * ran, the length never shrank, and the loop re-erased the same page - * forever; the rest of the requested range was never reached. + * Regression test for the hal_flash_erase() loop in hal/samr21.c: + * the remaining length was decremented as the body of the NVMREADY + * wait loop instead of once per completed erase, and the address was + * never advanced, so the loop re-erased the first row forever and the + * rest of the requested range was never reached. The erase command + * (NVMCMD_ERASE, 0x02) is the SAM D/R NVMCTRL row erase: one command + * erases a 256-byte row (4 x FLASH_PAGESIZE), so the loop must + * advance by the row size. A page-size stride issues four row-erase + * commands against the same row (the later ones with a + * non-row-aligned address), quadrupling erase time and wear. * * hal/samr21.c needs the SAMR21 SDK headers and cannot be built on the * host, so the Makefile extracts the register macros and the function * verbatim. The NVMCTRL register window is a host array with NVMREADY * preset (an idle peripheral whose erases complete immediately), and - * the test checks the page left programmed by the loop. + * the test checks the row left programmed by the loop. * * Copyright (C) 2026 wolfSSL Inc. * @@ -44,7 +48,7 @@ static uint8_t g_nvm[0x20]; #define NVMCTRL_BASE ((uintptr_t)g_nvm) -/* NVMCTRL register macros + command codes + page size from +/* NVMCTRL register macros + command codes + erase granularity from * hal/samr21.c (extracted by the Makefile). */ #include "samr21_erase_extract.h" @@ -58,9 +62,10 @@ static void mock_reset(void) /* The real hal_flash_erase() from hal/samr21.c (extracted). */ #include "samr21_erase_fn_extract.h" -/* A 64-byte page is one erase; a multi-page range must end with the - * last page of the range programmed, not the first. */ -START_TEST (test_erase_multi_page_advances) +/* A sub-row request issues a single row erase: the row containing the + * range is erased once and the row register holds its start. A + * page-size stride would program the next page (0x1040) instead. */ +START_TEST (test_erase_sub_row_single_erase) { int ret; @@ -69,36 +74,37 @@ START_TEST (test_erase_multi_page_advances) ret = hal_flash_erase(0x1000, 128); ck_assert_int_eq(ret, 0); - /* Two 64-byte pages: 0x1000 then 0x1040. The programmed row - * register holds (byte address) >> 1; the final one must be the - * last page of the range. Pre-fix the loop re-erased page 0x1000 - * forever (this case times out). */ - ck_assert_uint_eq(NVMCTRL_ADDR, (0x1040 >> 1)); + /* The programmed row register holds (byte address) >> 1; the row + * erased must be the one containing the requested range. */ + ck_assert_uint_eq(NVMCTRL_ADDR, (0x1000 >> 1)); } END_TEST -/* Four pages from the start of the flash must advance to the last one. */ -START_TEST (test_erase_four_pages_advances) +/* Two rows from a row-aligned start must advance to the second row. */ +START_TEST (test_erase_two_rows_advances) { int ret; mock_reset(); - ret = hal_flash_erase(0, 256); + ret = hal_flash_erase(0, 512); ck_assert_int_eq(ret, 0); - ck_assert_uint_eq(NVMCTRL_ADDR, (192u >> 1)); + /* Rows 0x0 and 0x100: the final programmed row must be 0x100. + * A page-size stride ends at 0x2C0; pre-advance it re-erased row + * 0 forever (this case times out). */ + ck_assert_uint_eq(NVMCTRL_ADDR, (0x100 >> 1)); } END_TEST -/* A single-page erase completes and leaves its page programmed. */ -START_TEST (test_erase_single_page) +/* Exactly one full row erases once and stops, no extra row. */ +START_TEST (test_erase_single_row) { int ret; mock_reset(); - ret = hal_flash_erase(0x2000, 64); + ret = hal_flash_erase(0x2000, 256); ck_assert_int_eq(ret, 0); ck_assert_uint_eq(NVMCTRL_ADDR, (0x2000 >> 1)); @@ -110,11 +116,11 @@ Suite *samr21_erase_suite(void) Suite *s = suite_create("samr21 erase advance"); TCase *tc = tcase_create("erase-address"); - tcase_add_test(tc, test_erase_multi_page_advances); - tcase_add_test(tc, test_erase_four_pages_advances); - tcase_add_test(tc, test_erase_single_page); - /* Pre-fix the idle-peripheral model hangs in the re-erase loop; - * the tcase timeout is what turns that hang into a failure. */ + tcase_add_test(tc, test_erase_sub_row_single_erase); + tcase_add_test(tc, test_erase_two_rows_advances); + tcase_add_test(tc, test_erase_single_row); + /* Pre-advance the idle-peripheral model hangs in the re-erase + * loop; the tcase timeout is what turns that hang into a failure. */ tcase_set_timeout(tc, 10); suite_add_tcase(s, tc); return s; From b115363291ddae38205de918bd50586e76c7ada5 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 18:59:48 +0200 Subject: [PATCH 07/10] pci: accept MMIO pools whose exclusive end is 4 GiB A pool ending exactly at 0x100000000 (e.g. 0xC0000000 + 0x40000000, the classic top-half 32-bit MMIO layout) was a working configuration: the old base + length - 1 initialization wrapped to 0xFFFFFFFF in 32-bit arithmetic. The overflow guard from the exclusive-limit fix rejected such pools with base + length > 0xFFFFFFFF, aborting enumeration - and the FSP caller discards the return value, so the platform would boot with no PCI BARs programmed. The limit fields cannot hold the exclusive end 0x100000000 while 32-bit, so widen mem_limit and mem_pf_limit (and the limit parameters of pci_enum_next_aligned32 and pci_align_check_up, plus the local in pci_program_bar) to 64-bit, and reject only pools whose end is above the 32-bit space. The initialization now casts to 64-bit before the addition so the sum cannot wrap. The T10xx PCIe setup initializes the same struct with the old inclusive base + length - 1 form; align it to the exclusive semantics the allocator enforces, or the last byte of the configured pool is unusable. New unit-pci-4gib build of the existing test file with the MMIO pool [0xC0000000, 0x100000000): pci_enum_do() must accept the pool and map a 1 MB BAR at the pool base. Fails on the old guard. --- hal/nxp_t10xx.c | 9 ++++--- include/pci.h | 6 +++-- src/pci.c | 20 ++++++++------- tools/unit-tests/Makefile | 16 ++++++++++-- tools/unit-tests/unit-pci.c | 51 +++++++++++++++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 16 deletions(-) diff --git a/hal/nxp_t10xx.c b/hal/nxp_t10xx.c index 9fcd2f7ecb..292feaa0bd 100644 --- a/hal/nxp_t10xx.c +++ b/hal/nxp_t10xx.c @@ -1796,10 +1796,13 @@ static int hal_pcie_init(void) memset(&enum_info, 0, sizeof(enum_info)); enum_info.curr_bus_number = 0; enum_info.mem = CONFIG_PCIE_MEM_BUS; - enum_info.mem_limit = enum_info.mem + (CONFIG_PCIE_MEM_LENGTH - 1); + /* Pool limits are exclusive ends (the allocator accepts a + * region when its end is <= limit). */ + enum_info.mem_limit = (uint64_t)enum_info.mem + + CONFIG_PCIE_MEM_LENGTH; enum_info.mem_pf = (enum_info.mem + CONFIG_PCIE_MEM_PREFETCH_LENGTH); - enum_info.mem_pf_limit = enum_info.mem_pf + - (CONFIG_PCIE_MEM_PREFETCH_LENGTH - 1); + enum_info.mem_pf_limit = (uint64_t)enum_info.mem_pf + + CONFIG_PCIE_MEM_PREFETCH_LENGTH; enum_info.io = CONFIG_PCIE_IO_BASE; /* Setup PCIe Output Windows */ diff --git a/include/pci.h b/include/pci.h index 77c669d728..e481987129 100644 --- a/include/pci.h +++ b/include/pci.h @@ -90,10 +90,12 @@ typedef struct { struct pci_enum_info { uint32_t mem; - uint32_t mem_limit; + /* Exclusive pool ends. 64-bit: a pool may end exactly at 4 GiB + * (0x100000000), which a 32-bit field cannot represent. */ + uint64_t mem_limit; uint32_t io; uint32_t mem_pf; - uint32_t mem_pf_limit; + uint64_t mem_pf_limit; uint8_t curr_bus_number; }; diff --git a/src/pci.c b/src/pci.c index 797cc6b55f..b2376f38bb 100644 --- a/src/pci.c +++ b/src/pci.c @@ -109,7 +109,7 @@ static inline uint32_t align_down(uint32_t address, uint32_t alignment) { } static int pci_align_check_up(uint32_t address, uint32_t alignment, - uint32_t limit, uint32_t *aligned) + uint64_t limit, uint32_t *aligned) { uint32_t a; a = align_up(address, alignment); @@ -364,7 +364,7 @@ static int pci_enum_is_mmio(uint32_t value) } static int pci_enum_next_aligned32(uint32_t address, uint32_t *next, - uint32_t align, uint32_t limit) + uint32_t align, uint64_t limit) { uintptr_t addr; @@ -422,7 +422,7 @@ static int pci_program_bar(uint8_t bus, uint8_t dev, uint8_t fun, uint8_t bar_off; int is_prefetch; uint32_t *base; - uint32_t limit; + uint64_t limit; uint32_t reg; int is_mmio; int ret = 0; @@ -937,11 +937,13 @@ int pci_enum_do(void) * when its end is <= limit (pci_enum_next_aligned32, the BAR end * check, pci_align_check_up) and the IO limit is the 16-bit IO * ceiling, not the last usable address. A region ending exactly - * at base + length must fit, so initialize base + length; reject - * a pool whose end would wrap the 32-bit address space. */ - if ((uint64_t)PCI_MMIO32_BASE + PCI_MMIO32_LENGTH > 0xFFFFFFFFULL || + * at base + length must fit, so initialize base + length. The + * limit fields are 64-bit because a pool may end exactly at + * 0x100000000 (4 GiB), the top of the 32-bit space; reject only + * pools whose end is above it. */ + if ((uint64_t)PCI_MMIO32_BASE + PCI_MMIO32_LENGTH > 0x100000000ULL || (uint64_t)PCI_MMIO32_PREFETCH_BASE + - PCI_MMIO32_PREFETCH_LENGTH > 0xFFFFFFFFULL) + PCI_MMIO32_PREFETCH_LENGTH > 0x100000000ULL) { PCI_DEBUG_PRINTF("PCI MMIO pool overflows the 32-bit address " "space\r\n"); @@ -949,9 +951,9 @@ int pci_enum_do(void) } enum_info.mem = PCI_MMIO32_BASE; - enum_info.mem_limit = enum_info.mem + PCI_MMIO32_LENGTH; + enum_info.mem_limit = (uint64_t)enum_info.mem + PCI_MMIO32_LENGTH; enum_info.mem_pf = PCI_MMIO32_PREFETCH_BASE; - enum_info.mem_pf_limit = enum_info.mem_pf + + enum_info.mem_pf_limit = (uint64_t)enum_info.mem_pf + PCI_MMIO32_PREFETCH_LENGTH; enum_info.io = PCI_IO32_BASE; enum_info.curr_bus_number = 0; diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 39dcc8f3a9..6ff20bac61 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -56,8 +56,8 @@ endif TESTS:=unit-parser unit-parser-large-header unit-fdt unit-extflash unit-string \ unit-spi-flash unit-aes128 \ unit-uart-flash \ - unit-aes256 unit-chacha20 unit-pci unit-mock-state unit-sectorflags \ - unit-max-space \ + unit-aes256 unit-chacha20 unit-pci unit-pci-4gib unit-mock-state \ + unit-sectorflags unit-max-space \ unit-image unit-image-hybrid unit-image-rsa unit-nvm unit-nvm-flagshome unit-enc-nvm \ unit-enc-nvm-flagshome unit-delta unit-gzip unit-update-flash unit-update-flash-delta \ unit-update-flash-hook \ @@ -619,6 +619,18 @@ unit-chacha20: ../../include/target.h unit-extflash.c unit-pci: unit-pci.c ../../src/pci.c gcc -o $@ $< $(CFLAGS) -DWOLFBOOT_USE_PCI $(LDFLAGS) +# unit-pci-4gib reruns the same test file with the MMIO pool ending +# exactly at the 4 GiB boundary ([0xC0000000, 0x100000000)), which a +# 32-bit pool limit cannot represent. Only the pool-end test runs. +unit-pci-4gib: unit-pci.c ../../src/pci.c + gcc -o $@ $< $(CFLAGS) -DWOLFBOOT_USE_PCI \ + -DUNIT_TEST_PCI_POOL_4GIB \ + -DPCI_MMIO32_BASE=0xC0000000ULL \ + -DPCI_MMIO32_LENGTH=0x40000000ULL \ + -DPCI_MMIO32_PREFETCH_BASE=0x80000000ULL \ + -DPCI_MMIO32_PREFETCH_LENGTH=0x40000000ULL \ + $(LDFLAGS) + # linux_loader.c is x86 32bit only and pulls in inline asm guarded on 32bit; # build standalone with -m32 and without coverage (no 32bit gcov/check libs). unit-linux-loader-e820: ../../include/target.h unit-linux-loader-e820.c diff --git a/tools/unit-tests/unit-pci.c b/tools/unit-tests/unit-pci.c index f75dfd7407..6dd2527e16 100644 --- a/tools/unit-tests/unit-pci.c +++ b/tools/unit-tests/unit-pci.c @@ -1731,6 +1731,36 @@ START_TEST(test_enum_do_pool_fill) } END_TEST +#ifdef UNIT_TEST_PCI_POOL_4GIB +/* Compiled only in the unit-pci-4gib build, where the MMIO pool is + * [0xC0000000, 0x100000000): its exclusive end sits exactly on the + * 4 GiB boundary. A 32-bit limit field cannot represent that end, so + * the pool may be rejected only when the end is above the boundary, + * and a BAR must still be mappable from such a pool. */ +START_TEST (test_pool_end_4gib) +{ + struct test_pci_topology t; + int dev_node; + uint32_t bar_val; + int ret; + + test_pci_init(&t); + dev_node = test_pci_add_dev(&t, 0, 0, 0x1234, 0x5678, TEST_PCI_ROOT_BUS); + test_pci_dev_set_bar(&t, dev_node, 0, 0x00100000, TEST_PCI_BAR_MMIO); + test_pci_commit(&t); + + ret = pci_enum_do(); + ck_assert_int_eq(ret, 0); + + /* The BAR is allocated at the pool base */ + bar_val = pci_config_read32(0, 0, 0, PCI_BAR0_OFFSET); + ck_assert_uint_eq(bar_val, 0xC0000000); + + test_pci_cleanup(&t); +} +END_TEST +#endif /* UNIT_TEST_PCI_POOL_4GIB */ + /* test_enum_do_nested_bridges: end-to-end nested bridge enumeration */ START_TEST(test_enum_do_nested_bridges) @@ -2059,6 +2089,7 @@ Suite *wolfboot_suite(void) return s; } +#ifndef UNIT_TEST_PCI_POOL_4GIB int main(void) { int fails; @@ -2069,3 +2100,23 @@ int main(void) srunner_free(sr); return fails; } +#else +/* The 4 GiB-end pool build runs only the pool-end test: the rest of + * the suite assumes the default 128 MB pool layout. */ +int main(void) +{ + int fails; + Suite *s = suite_create("pci-pool-4gib"); + TCase *tc = tcase_create("pool-end-4gib"); + SRunner *sr; + + tcase_add_test(tc, test_pool_end_4gib); + suite_add_tcase(s, tc); + + sr = srunner_create(s); + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return fails; +} +#endif /* UNIT_TEST_PCI_POOL_4GIB */ From ceae23e34fc4f9695c4898fe04e59cdb8a361540 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 19:01:41 +0200 Subject: [PATCH 08/10] unit-tests: sector-align the update-trigger scrub fixture wolfBoot_update_trigger() derives the staged sector by rounding the update flags address down to a sector boundary and copies a full sector from it. The g_sector fixture carried no sector alignment, so the boundary landed inside the array and the copy read up to 4095 bytes past its end - the test passed only because the over-read happened to fall in a neighboring global. Align g_sector to WOLFBOOT_SECTOR_SIZE so the staged sector is the array itself; the copy stays in bounds by construction. Also ignore the generated unit-test extraction headers, as the other generated sources in that list are. --- .gitignore | 3 +++ tools/unit-tests/unit-update-trigger-scrub.c | 11 ++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 7d474844af..890f1a4463 100644 --- a/.gitignore +++ b/.gitignore @@ -492,12 +492,15 @@ tools/unit-tests/nxp_ls1028a_host.c tools/unit-tests/nxp_p1021_host.c tools/unit-tests/nxp_t10xx_fixup_extract.h tools/unit-tests/sama5d3_read_extract.h +tools/unit-tests/samr21_erase_extract.h +tools/unit-tests/samr21_erase_fn_extract.h tools/unit-tests/sdhci_host.c tools/unit-tests/stm32l5_write_extract.h tools/unit-tests/stm32u5_write_extract.h tools/unit-tests/t10xx_qe_firmware_extract.h tools/unit-tests/t2080_fman_extract.h tools/unit-tests/ti_hercules_write_extract.h +tools/unit-tests/update_trigger_scrub_extract.h tools/unit-tests/versal_ext_write_extract.h tools/unit-tests/versal_host.c tools/unit-tests/versal_host.h diff --git a/tools/unit-tests/unit-update-trigger-scrub.c b/tools/unit-tests/unit-update-trigger-scrub.c index e35ec8e986..b69d5585ff 100644 --- a/tools/unit-tests/unit-update-trigger-scrub.c +++ b/tools/unit-tests/unit-update-trigger-scrub.c @@ -52,13 +52,18 @@ static uint8_t NVM_CACHE[NVM_CACHE_SIZE]; /* The staged sector image: the boot/update trailer sector carrying - * the firmware key/nonce pattern at a fixed offset. */ + * the firmware key/nonce pattern at a fixed offset. Sector-aligned: + * wolfBoot_update_trigger() derives the staged sector by rounding the + * flag address down to a sector boundary, and the sector copy must + * stay inside this array. */ #define KEY_OFF 0x0F00 #define KEY_LEN 64 -static uint8_t g_sector[NVM_CACHE_SIZE]; +static uint8_t g_sector[NVM_CACHE_SIZE] + __attribute__((aligned(WOLFBOOT_SECTOR_SIZE))); /* The update partition flags end at the top of the staged sector, so - * lastSector in wolfBoot_update_trigger() resolves to g_sector. */ + * lastSector in wolfBoot_update_trigger() resolves to the base of + * g_sector (the alignment above makes that true by construction). */ #define PART_UPDATE_ENDFLAGS ((uintptr_t)(g_sector + WOLFBOOT_SECTOR_SIZE)) /* Stubbed flash layer: records calls. */ From 2fdc99f323f1663a5cd551c032bdf7e9b34b2a09 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 20:05:56 +0200 Subject: [PATCH 09/10] pci: keep the allocation cursors 64-bit at the 4 GiB pool end The cursors mem, mem_pf and io advanced as 32-bit values: after a BAR allocation whose end is exactly the pool end 0x100000000 (now reachable), *base = bar_value + length wrapped to 0. Every later allocation then passed the start and end checks and programmed its BAR at address 0, over the legacy IO range and DRAM. Widen the three cursors to 64-bit and advance with a 64-bit sum, so an exhausted pool leaves the cursor at the end. The address parameters of pci_enum_next_aligned32 and pci_align_check_up widen with them; pci_enum_next_aligned32 computes in uint64_t rather than uintptr_t, which on 32-bit targets would truncate the exhausted cursor back to 0 and defeat the addr > 0xffffffff rejection. The programmed BAR value stays 32-bit. The 4 GiB pool unit test now also adds a second device with a preset (previously programmed) BAR: exactly filling the pool must leave that BAR untouched instead of re-allocating it from a wrapped cursor. The mock learns to seed a BAR preset from the bar info, and the loop variable shadowing in test_pci_commit that it exposed is fixed (the inner preset loop clobbered the outer node loop counter, so only the first node was ever committed). Verified: unit-pci 30/30, unit-pci-4gib green, full nxp_t1024 powerpc build green. --- include/pci.h | 12 +++++---- src/pci.c | 52 ++++++++++++++++++++----------------- tools/unit-tests/unit-pci.c | 32 ++++++++++++++++++++--- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/include/pci.h b/include/pci.h index e481987129..9c0128883f 100644 --- a/include/pci.h +++ b/include/pci.h @@ -89,12 +89,14 @@ typedef struct { } pci_ctrlr_info_t; struct pci_enum_info { - uint32_t mem; - /* Exclusive pool ends. 64-bit: a pool may end exactly at 4 GiB - * (0x100000000), which a 32-bit field cannot represent. */ + /* Allocation cursors and exclusive pool ends. All 64-bit: a pool + * may end exactly at 4 GiB (0x100000000), which a 32-bit value + * cannot represent, and an exhausted cursor must stay at the pool + * end instead of wrapping to 0 and re-allocating over address 0. */ + uint64_t mem; uint64_t mem_limit; - uint32_t io; - uint32_t mem_pf; + uint64_t io; + uint64_t mem_pf; uint64_t mem_pf_limit; uint8_t curr_bus_number; }; diff --git a/src/pci.c b/src/pci.c index b2376f38bb..d87b6b964e 100644 --- a/src/pci.c +++ b/src/pci.c @@ -100,18 +100,18 @@ static int pci_enum_is_64bit(uint32_t value); static int pci_enum_is_mmio(uint32_t value); -static inline uint32_t align_up(uint32_t address, uint32_t alignment) { - return (address + alignment - 1) & ~(alignment - 1); +static inline uint64_t align_up(uint64_t address, uint32_t alignment) { + return (address + alignment - 1) & ~(uint64_t)(alignment - 1); } static inline uint32_t align_down(uint32_t address, uint32_t alignment) { return address & ~(alignment - 1); } -static int pci_align_check_up(uint32_t address, uint32_t alignment, - uint64_t limit, uint32_t *aligned) +static int pci_align_check_up(uint64_t address, uint32_t alignment, + uint64_t limit, uint64_t *aligned) { - uint32_t a; + uint64_t a; a = align_up(address, alignment); if (a < address || a >= limit) return -1; @@ -363,17 +363,20 @@ static int pci_enum_is_mmio(uint32_t value) return (value & PCI_ENUM_MMIND_MASK) == 0; } -static int pci_enum_next_aligned32(uint32_t address, uint32_t *next, +static int pci_enum_next_aligned32(uint64_t address, uint32_t *next, uint32_t align, uint64_t limit) { - uintptr_t addr; + uint64_t addr; - addr = (uintptr_t)address; + /* 64-bit on purpose: an exhausted pool leaves the cursor at + * 0x100000000, which a 32-bit type (uintptr_t included on 32-bit + * targets) would truncate back to 0. */ + addr = address; align = align-1; - addr = (addr + align) & (~align); + addr = (addr + align) & (~(uint64_t)align); if (addr > 0xffffffff) return -1; - if (addr < (uintptr_t)address) + if (addr < address) return -1; if (addr >= limit) return -1; @@ -421,7 +424,7 @@ static int pci_program_bar(uint8_t bus, uint8_t dev, uint8_t fun, uint32_t length, align; uint8_t bar_off; int is_prefetch; - uint32_t *base; + uint64_t *base; uint64_t limit; uint32_t reg; int is_mmio; @@ -524,7 +527,7 @@ static int pci_program_bar(uint8_t bus, uint8_t dev, uint8_t fun, pci_config_write32(bus, dev, fun, bar_off, bar_value); if (*is_64bit) pci_config_write32(bus, dev, fun, bar_off + 4, 0x0); - *base = bar_value + length; + *base = (uint64_t)bar_value + length; PCI_DEBUG_PRINTF("PCI enum: %s bus: %x:%x.%x bar: %d [%x,%x] (0x%x %s %s)\r\n", (is_mmio ? "mm" : "io"), bus, dev, fun, bar_idx, bar_value, bar_value + length, length, (*is_64bit) ? "64bit" : "", @@ -617,14 +620,14 @@ static inline void pci_dump_bridge(uint8_t bus, uint8_t dev, uint8_t fun) static int pci_program_bridge(uint8_t bus, uint8_t dev, uint8_t fun, struct pci_enum_info *info) { - uint32_t prefetch_start; - uint32_t mem_start; - uint32_t io_start; + uint64_t prefetch_start; + uint64_t mem_start; + uint64_t io_start; uint32_t orig_cmd; uint8_t saved_bus; - uint32_t saved_mem; - uint32_t saved_pf; - uint32_t saved_io; + uint64_t saved_mem; + uint64_t saved_pf; + uint64_t saved_io; int ret; saved_bus = info->curr_bus_number; @@ -967,16 +970,17 @@ int pci_enum_do(void) ret = pci_enum_bus(0, &enum_info); PCI_DEBUG_PRINTF("PCI Memory Mapped I/O range [0x%x,0x%x] (0x%x)\r\n", - (uint32_t)PCI_MMIO32_BASE, enum_info.mem, - enum_info.mem - PCI_MMIO32_BASE); + (uint32_t)PCI_MMIO32_BASE, (uint32_t)enum_info.mem, + (uint32_t)(enum_info.mem - PCI_MMIO32_BASE)); PCI_DEBUG_PRINTF("PCI Memory Mapped I/O range (prefetch) [0x%x,0x%x] (0x%x)\r\n", - (uint32_t)PCI_MMIO32_PREFETCH_BASE, enum_info.mem_pf, - enum_info.mem_pf - PCI_MMIO32_PREFETCH_BASE); + (uint32_t)PCI_MMIO32_PREFETCH_BASE, + (uint32_t)enum_info.mem_pf, + (uint32_t)(enum_info.mem_pf - PCI_MMIO32_PREFETCH_BASE)); PCI_DEBUG_PRINTF("PCI I/O range [0x%x,0x%x] (0x%x)\r\n", - (uint32_t)PCI_IO32_BASE, enum_info.io, - enum_info.io - PCI_IO32_BASE); + (uint32_t)PCI_IO32_BASE, (uint32_t)enum_info.io, + (uint32_t)(enum_info.io - PCI_IO32_BASE)); return ret; } diff --git a/tools/unit-tests/unit-pci.c b/tools/unit-tests/unit-pci.c index 6dd2527e16..67d5dd39e8 100644 --- a/tools/unit-tests/unit-pci.c +++ b/tools/unit-tests/unit-pci.c @@ -59,6 +59,7 @@ struct test_pci_bar_info { uint32_t upper_mask; /* 64-bit BARs: upper half probe mask (0 = use default 0xFFFFFFFF) */ uint8_t has_raw_probe;/* 1=override probe readback with raw_probe (hostile/malformed BAR) */ uint32_t raw_probe; /* raw value returned on probe when has_raw_probe is set */ + uint32_t preset; /* initial BAR register value (previously programmed) */ }; struct test_pci_node { @@ -146,9 +147,19 @@ static void test_pci_dev_set_bar(struct test_pci_topology *t, int node_idx, b->is_prefetch = (type & TEST_PCI_BAR_PF) != 0; } +static void test_pci_dev_set_bar_preset(struct test_pci_topology *t, + int node_idx, int bar_idx, + uint32_t value) +{ + ck_assert(node_idx >= 0 && node_idx < t->count); + ck_assert(bar_idx >= 0 && bar_idx < TEST_PCI_MAX_BARS); + t->nodes[node_idx].bars[bar_idx].preset = value; +} + static void test_pci_commit(struct test_pci_topology *t) { int i; + int j; for (i = 0; i < t->count; i++) { struct test_pci_node *n = &t->nodes[i]; if (!n->in_use) @@ -163,6 +174,8 @@ static void test_pci_commit(struct test_pci_topology *t) n->cfg[PCI_CLASS_CODE_BYTE_OFFSET] = 0x06; n->cfg[PCI_SUBCLASS_BYTE_OFFSET] = 0x04; } + for (j = 0; j < TEST_PCI_MAX_BARS; j++) + memcpy(&n->cfg[PCI_BAR0_OFFSET + j * 4], &n->bars[j].preset, 4); } current_topology = t; } @@ -1740,22 +1753,33 @@ END_TEST START_TEST (test_pool_end_4gib) { struct test_pci_topology t; - int dev_node; + int dev_node, dev_next; uint32_t bar_val; int ret; test_pci_init(&t); dev_node = test_pci_add_dev(&t, 0, 0, 0x1234, 0x5678, TEST_PCI_ROOT_BUS); - test_pci_dev_set_bar(&t, dev_node, 0, 0x00100000, TEST_PCI_BAR_MMIO); + dev_next = test_pci_add_dev(&t, 1, 0, 0x9ABC, 0xDEF0, TEST_PCI_ROOT_BUS); + /* 1 GB BAR: exactly fills the [0xC0000000, 0x100000000) pool */ + test_pci_dev_set_bar(&t, dev_node, 0, 0x40000000, TEST_PCI_BAR_MMIO); + /* The next device's BAR was programmed by a previous boot */ + test_pci_dev_set_bar(&t, dev_next, 0, 0x00100000, TEST_PCI_BAR_MMIO); + test_pci_dev_set_bar_preset(&t, dev_next, 0, 0x40000000); test_pci_commit(&t); ret = pci_enum_do(); ck_assert_int_eq(ret, 0); - /* The BAR is allocated at the pool base */ + /* The 1 GB BAR is allocated at the pool base */ bar_val = pci_config_read32(0, 0, 0, PCI_BAR0_OFFSET); ck_assert_uint_eq(bar_val, 0xC0000000); + /* The pool is now exhausted exactly at 4 GiB. The allocation + * cursor must stay at the pool end, not wrap to 0 and program + * the next BAR over address 0: the second BAR is left untouched. */ + bar_val = pci_config_read32(0, 1, 0, PCI_BAR0_OFFSET); + ck_assert_uint_eq(bar_val, 0x40000000); + test_pci_cleanup(&t); } END_TEST @@ -1922,7 +1946,7 @@ END_TEST /* test_pci_align_check_up_overflow: edge cases for pci_align_check_up */ START_TEST(test_pci_align_check_up_overflow) { - uint32_t aligned; + uint64_t aligned; int ret; /* Normal case: already aligned */ From 42a42ec1c16327ebfd4bd30946e03f89e0cad105 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 26 Aug 2026 20:10:47 +0200 Subject: [PATCH 10/10] unit-tests: pin the update-trigger scrub payload and ordering The hal_flash_write stub discarded the written data, so the test only checked call counts and the final NVM_CACHE state. An implementation that scrubbed before the write - programming an all-zero flags sector and destroying the firmware key that persists in the trailer - would have satisfied every assertion. Capture the written sector in the stub at call time and assert the staged payload: the sector fill and key pattern intact, the fresh IMG_STATE_UPDATING byte and magic in place, with NVM_CACHE still required to be zero afterwards. Mutation-checked: a scrub-before- write variant that programs the zeroed sector fails the payload assertion. --- tools/unit-tests/unit-update-trigger-scrub.c | 31 +++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/tools/unit-tests/unit-update-trigger-scrub.c b/tools/unit-tests/unit-update-trigger-scrub.c index b69d5585ff..bbda007160 100644 --- a/tools/unit-tests/unit-update-trigger-scrub.c +++ b/tools/unit-tests/unit-update-trigger-scrub.c @@ -12,8 +12,12 @@ * * The real function is extracted by the Makefile (together with * nvm_cache_scrub()) and run with a test-owned NVM_CACHE, a staged - * sector carrying a key pattern and stubbed flash calls; the buffer - * must be zero after the call. + * sector carrying a key pattern and stubbed flash calls. The stub + * captures the written sector: it must carry the staged payload + * (key pattern plus the fresh flags), which pins the scrub-after- + * write ordering - a scrub that ran first would program an + * all-zero sector and destroy the key that persists in the trailer + * - and NVM_CACHE must be zero after the call. * Copyright (C) 2026 wolfSSL Inc. * * This file is part of wolfBoot. @@ -66,14 +70,18 @@ static uint8_t g_sector[NVM_CACHE_SIZE] * g_sector (the alignment above makes that true by construction). */ #define PART_UPDATE_ENDFLAGS ((uintptr_t)(g_sector + WOLFBOOT_SECTOR_SIZE)) -/* Stubbed flash layer: records calls. */ +/* Stubbed flash layer: records calls and captures the written + * sector at call time, before the function under test can scrub it. */ static int g_flash_writes; static int g_flash_erases; +static uint8_t g_written[WOLFBOOT_SECTOR_SIZE]; int hal_flash_write(uint32_t address, const uint8_t *data, int len) { - (void)address; (void)data; (void)len; + ck_assert_int_eq(len, WOLFBOOT_SECTOR_SIZE); + memcpy(g_written, data, WOLFBOOT_SECTOR_SIZE); g_flash_writes++; + (void)address; return 0; } @@ -151,6 +159,8 @@ static void teardown(void) * before rewriting the flags. Pre-fix the staged pattern remained. */ START_TEST(test_update_trigger_scrubs_cache) { + uint32_t magic = WOLFBOOT_MAGIC_TRAIL; + ck_assert_int_eq(cache_scrubbed(), 1); wolfBoot_update_trigger(); @@ -159,6 +169,19 @@ START_TEST(test_update_trigger_scrubs_cache) * both candidate sectors */ ck_assert_int_eq(g_flash_writes, 1); ck_assert_int_eq(g_flash_erases, 2); + + /* The written sector carries the staged payload: the sector fill + * and key pattern intact, the fresh state and magic in place. + * A scrub before the write would have programmed all zeros and + * destroyed the key that persists in the trailer. */ + ck_assert_int_eq(g_written[0], 0x11); + ck_assert_int_eq(g_written[KEY_OFF], 0xA5); + ck_assert_int_eq(g_written[KEY_OFF + KEY_LEN - 1], 0xA5); + ck_assert_int_eq(g_written[SECTOR_FLAGS_SIZE], IMG_STATE_UPDATING); + ck_assert_mem_eq(g_written + SECTOR_FLAGS_SIZE + 1, &magic, + sizeof(magic)); + + /* and the RAM copy of that sector is gone */ ck_assert_int_eq(cache_scrubbed(), 1); } END_TEST