From 6337b0a31e3f4e1959ccae9c5f09b9160e5fc8b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 02:31:11 +0200 Subject: [PATCH 1/2] fix(M10): wrap thread-context lwIP calls in cyw43_arch_lwip_begin/end; volatile ntp_state Co-Authored-By: Claude Opus 4.8 (1M context) --- network/ntp.c | 47 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/network/ntp.c b/network/ntp.c index 735d2d5..967f673 100644 --- a/network/ntp.c +++ b/network/ntp.c @@ -34,14 +34,22 @@ typedef enum { NTP_STATE_FAILED, } ntp_state_t; -static ntp_state_t ntp_state = NTP_STATE_IDLE; -static uint8_t ntp_nonce[8]; // stored transmit timestamp for origin check -static struct udp_pcb *ntp_pcb = NULL; -static ip_addr_t server_addr; - -static bool synced = false; -static uint32_t last_sync_unix = 0; -static uint64_t last_sync_monotonic_us = 0; +// ntp_state, synced and the last_sync_* pair are written from the lwIP +// callbacks (dns_found_cb / ntp_recv_cb -> apply_time), which run in a +// low-priority IRQ under pico_cyw43_arch_lwip_threadsafe_background, and are +// read/spun-on from thread context (ntp_sync's wait loop, ntp_task, +// ntp_is_synced, ntp_last_sync_time). volatile stops the compiler caching the +// spun-on value or reordering the reads across the IRQ store. (The 64-bit +// last_sync_monotonic_us can still tear on a 32-bit read; eliminating that +// needs the copy-48-bytes+flag restructuring deferred to M10b.) +static volatile ntp_state_t ntp_state = NTP_STATE_IDLE; +static uint8_t ntp_nonce[8]; // stored transmit timestamp for origin check +static struct udp_pcb *ntp_pcb = NULL; +static ip_addr_t server_addr; + +static volatile bool synced = false; +static volatile uint32_t last_sync_unix = 0; +static volatile uint64_t last_sync_monotonic_us = 0; // Cumulative backward slack consumed since boot (see NTP_ROLLBACK_BUDGET_S). static uint32_t rollback_budget_used_s = 0; @@ -239,18 +247,28 @@ void ntp_init(void) { } bool ntp_sync(void) { + // Every lwIP call below runs in thread context, so it must be bracketed by + // cyw43_arch_lwip_begin()/end() to lock out the background-IRQ lwIP handler + // (udp_remove unlinks the pcb from lwIP's global list; racing udp_input() + // walking that list is a use-after-free the attacker times via datagrams). if (ntp_pcb) { + cyw43_arch_lwip_begin(); udp_remove(ntp_pcb); + cyw43_arch_lwip_end(); ntp_pcb = NULL; } + cyw43_arch_lwip_begin(); ntp_pcb = udp_new_ip_type(IPADDR_TYPE_ANY); + cyw43_arch_lwip_end(); if (!ntp_pcb) { printf("[ntp] failed to create UDP pcb\r\n"); return false; } + cyw43_arch_lwip_begin(); udp_recv(ntp_pcb, ntp_recv_cb, NULL); + cyw43_arch_lwip_end(); ntp_state = NTP_STATE_RESOLVING; cyw43_arch_lwip_begin(); @@ -258,11 +276,18 @@ bool ntp_sync(void) { cyw43_arch_lwip_end(); if (err == ERR_OK) { - // Already cached - fire callback manually + // Already cached - fire callback manually. On the async path lwIP + // invokes dns_found_cb from the background IRQ (already holding the lwIP + // lock), but here we call it from thread context, so its lwIP calls + // (udp_connect/pbuf_alloc/udp_sendto/pbuf_free) must be locked by us. + cyw43_arch_lwip_begin(); dns_found_cb(NTP_SERVER, &server_addr, NULL); + cyw43_arch_lwip_end(); } else if (err != ERR_INPROGRESS) { printf("[ntp] DNS error: %d\r\n", err); + cyw43_arch_lwip_begin(); udp_remove(ntp_pcb); + cyw43_arch_lwip_end(); ntp_pcb = NULL; return false; } @@ -274,7 +299,9 @@ bool ntp_sync(void) { sleep_ms(10); if (time_reached(deadline)) { printf("[ntp] timed out\r\n"); + cyw43_arch_lwip_begin(); udp_remove(ntp_pcb); + cyw43_arch_lwip_end(); ntp_pcb = NULL; return false; } @@ -283,7 +310,9 @@ bool ntp_sync(void) { bool ok = ntp_state == NTP_STATE_SUCCESS; ntp_state = NTP_STATE_IDLE; + cyw43_arch_lwip_begin(); udp_remove(ntp_pcb); + cyw43_arch_lwip_end(); ntp_pcb = NULL; return ok; From 90a5a6b68d4daa5bd0808ae46ffeb20c27ef7856 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 20:26:42 +0200 Subject: [PATCH 2/2] fix(cmd_status): function-scope keys so the coverage/firmware build compiles Rebasing onto current master pulled in a latent build break: cmd_status declares `keys` inside the `if (commands_is_admin())` block but scrubs it via `secure_wipe(keys, sizeof(keys))` at function scope, so `keys` is undeclared there. asan_commands links serial/commands.c (not commands_system.c) so it never compiled this TU, but `make -C test coverage` compiles the whole first-party surface and fails here (`keys undeclared`) -- as does the real firmware build. Restore `keys` to function scope, matching the scrub's intent of always clearing the key DB from BSS (incl. the non-admin path where it stays zero-initialised). Same fix as PR #15 (M1). Co-Authored-By: Claude Opus 4.8 (1M context) --- serial/commands_system.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/serial/commands_system.c b/serial/commands_system.c index 4b92f9e..bdd3cac 100644 --- a/serial/commands_system.c +++ b/serial/commands_system.c @@ -64,11 +64,13 @@ void cmd_status(int argc, char **argv) { printf("ntp: not synced\r\n"); } - // Keys (admin only: key inventory is target-selection data) + // Keys (admin only: key inventory is target-selection data). Declared at + // function scope so the scrub below always runs, even on the non-admin path + // where the array stays zero-initialised. + static key_record_t keys[BACKUP_MAX_KEYS]; if (commands_is_admin()) { - static key_record_t keys[BACKUP_MAX_KEYS]; - int count = storage_key_list(keys, BACKUP_MAX_KEYS); - int enabled = 0, corrupt = 0; + int count = storage_key_list(keys, BACKUP_MAX_KEYS); + int enabled = 0, corrupt = 0; for (int i = 0; i < count; i++) { if (!keys[i].is_checksum_valid) corrupt++;