From 495f3bb5e181db562e92da082f10515bb58e663f Mon Sep 17 00:00:00 2001 From: Denis Suprunenko Date: Sun, 17 May 2026 13:49:55 +0200 Subject: [PATCH 1/2] fix(usb-ncm): eliminate NULL-deref race in xmit_setup_next_glue_ntb and tud_network_xmit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both functions read ncm_interface.xmit_glue_ntb twice: once for a NULL check, then again to assign a local pointer. A concurrent SET_INTERFACE or DMA-done callback (USB task) can zero xmit_glue_ntb between the two reads via ncm_flush_data_paths() or xmit_start_if_possible(), causing the second read to return NULL and the subsequent field write to address 0. Crash site: ncm_device.c:509, pc=0x08101690, r3=0 → SecureFault → TF-M SYSRESETREQ → nboot ISP loop (T-145). Fix: capture the pointer once into a local variable, NULL-check the local, and publish to the global only after the NTB is fully initialised (xmit_setup_next_glue_ntb). In tud_network_xmit, snapshot the global before the NULL check so the same pointer is used throughout. Co-Authored-By: Claude Sonnet 4.6 --- src/class/net/ncm_device.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/class/net/ncm_device.c b/src/class/net/ncm_device.c index 79b14a0ec..5fbd19481 100644 --- a/src/class/net/ncm_device.c +++ b/src/class/net/ncm_device.c @@ -495,17 +495,19 @@ static bool xmit_setup_next_glue_ntb(void) { xmit_put_ntb_into_ready_list(ncm_interface.xmit_glue_ntb); } - ncm_interface.xmit_glue_ntb = xmit_get_free_ntb();// get next buffer (if any) - if (ncm_interface.xmit_glue_ntb == NULL) { + // Capture into local: a concurrent SET_INTERFACE or DMA-done callback can + // zero ncm_interface.xmit_glue_ntb between a NULL check and the next read. + // Using a local guarantees the pointer we check is the pointer we dereference. + xmit_ntb_t *ntb = xmit_get_free_ntb(); + if (ntb == NULL) { TU_LOG_DRV(" xmit_setup_next_glue_ntb - nothing free\n");// should happen rarely + ncm_interface.xmit_glue_ntb = NULL; return false; } ncm_interface.xmit_glue_ntb_datagram_ndx = 0; - xmit_ntb_t *ntb = ncm_interface.xmit_glue_ntb; - - // Fill in NTB header + // Fill in NTB header via local — no re-read of the global ntb->nth.dwSignature = NTH16_SIGNATURE; ntb->nth.wHeaderLength = sizeof(ntb->nth); ntb->nth.wSequence = ncm_interface.xmit_sequence++; @@ -518,6 +520,9 @@ static bool xmit_setup_next_glue_ntb(void) { ntb->ndp.wNextNdpIndex = 0; memset(ntb->ndp_datagram, 0, sizeof(ntb->ndp_datagram)); + + // Publish to global only after NTB is fully initialised + ncm_interface.xmit_glue_ntb = ntb; return true; } // xmit_setup_next_glue_ntb @@ -795,13 +800,14 @@ bool tud_network_can_xmit(uint16_t size) { void tud_network_xmit(void *ref, uint16_t arg) { TU_LOG_DRV("tud_network_xmit(%p, %d)\n", ref, arg); - if (ncm_interface.xmit_glue_ntb == NULL) { + // Capture into local to avoid re-reading the global after the NULL check + // (a concurrent SET_INTERFACE can zero xmit_glue_ntb between the two reads). + xmit_ntb_t *ntb = ncm_interface.xmit_glue_ntb; + if (ntb == NULL) { TU_LOG_DRV("(EE) tud_network_xmit: no buffer\n");// must not happen (really) return; } - xmit_ntb_t *ntb = ncm_interface.xmit_glue_ntb; - // copy new datagram to the end of the current NTB uint16_t size = tud_network_xmit_cb(ntb->data + ntb->nth.wBlockLength, ref, arg); From 7c7d8e65a9b8bfaaf312ecf4dde9a6472629c97c Mon Sep 17 00:00:00 2001 From: Denis Suprunenko Date: Sun, 17 May 2026 13:52:54 +0200 Subject: [PATCH 2/2] fix(usb-ncm): add osal_spin_lock critical sections to xmit_glue_ntb state machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lwIP task (tud_network_can_xmit / tud_network_xmit) and the USB task (tud_task → ncm_flush_data_paths / xmit_start_if_possible) both mutate ncm_interface.xmit_glue_ntb and xmit_glue_ntb_datagram_ndx without synchronisation, creating data races on a preemptive FreeRTOS scheduler. The T-145.1 local-variable fix prevents the immediate NULL-deref crash; this commit closes the residual race window completely. Protected regions (single file-static osal_spinlock_t s_xmit_glue_lock): ncm_flush_data_paths() — zero xmit_glue_ntb + datagram_ndx (USB task) xmit_start_if_possible() — guard check + steal of xmit_glue_ntb (USB task) xmit_setup_next_glue_ntb() — snapshot old ptr, publish new ptr + ndx (lwIP task) tud_network_xmit() — snapshot ptr, reserve datagram slot (lwIP task) RW612 is single-core (TUP_MCU_MULTIPLE_CORE=0); osal_spin_lock(false) maps to taskENTER_CRITICAL() which masks interrupts up to configMAX_SYSCALL_ INTERRUPT_PRIORITY — includes the USB IRQ at configLIBRARY_LOWEST_ INTERRUPT_PRIORITY-1. All protected sections are short and non-blocking. Co-Authored-By: Claude Sonnet 4.6 --- src/class/net/ncm_device.c | 57 ++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/src/class/net/ncm_device.c b/src/class/net/ncm_device.c index 5fbd19481..21b1ff7de 100644 --- a/src/class/net/ncm_device.c +++ b/src/class/net/ncm_device.c @@ -138,6 +138,10 @@ typedef struct { static ncm_interface_t ncm_interface; CFG_TUD_MEM_SECTION static ncm_epbuf_t ncm_epbuf; +// Protects concurrent access to xmit_glue_ntb / xmit_glue_ntb_datagram_ndx +// between the lwIP task (tud_network_can_xmit / tud_network_xmit) and the +// USB task (tud_task → ncm_flush_data_paths / xmit_start_if_possible). +static osal_spinlock_t s_xmit_glue_lock; static void recv_put_ntb_into_free_list(recv_ntb_t *free_ntb); static bool ncm_host_strictly_configured_for_tx(void) { @@ -356,11 +360,13 @@ static void ncm_flush_data_paths(void) { ncm_interface.xmit_tinyusb_ntb = NULL; } + osal_spin_lock(&s_xmit_glue_lock, false); if (ncm_interface.xmit_glue_ntb != NULL) { xmit_put_ntb_into_free_list(ncm_interface.xmit_glue_ntb); ncm_interface.xmit_glue_ntb = NULL; } ncm_interface.xmit_glue_ntb_datagram_ndx = 0; + osal_spin_unlock(&s_xmit_glue_lock, false); for (int i = 0; i < XMIT_NTB_N; ++i) { if (ncm_interface.xmit_ready_ntb[i] != NULL) { @@ -443,12 +449,17 @@ static void xmit_start_if_possible(uint8_t rhport) { ncm_interface.xmit_tinyusb_ntb = xmit_get_next_ready_ntb(); if (ncm_interface.xmit_tinyusb_ntb == NULL) { + // Guard and steal of xmit_glue_ntb must be atomic with respect to the + // lwIP task that publishes / reads xmit_glue_ntb and datagram_ndx. + osal_spin_lock(&s_xmit_glue_lock, false); if (ncm_interface.xmit_glue_ntb == NULL || ncm_interface.xmit_glue_ntb_datagram_ndx == 0) { // -> really nothing is waiting + osal_spin_unlock(&s_xmit_glue_lock, false); return; } ncm_interface.xmit_tinyusb_ntb = ncm_interface.xmit_glue_ntb; ncm_interface.xmit_glue_ntb = NULL; + osal_spin_unlock(&s_xmit_glue_lock, false); } #if CFG_TUD_NCM_LOG_LEVEL >= 3 @@ -490,39 +501,44 @@ static bool xmit_requested_datagram_fits_into_current_ntb(uint16_t datagram_size static bool xmit_setup_next_glue_ntb(void) { TU_LOG_DRV("xmit_setup_next_glue_ntb - %p\n", ncm_interface.xmit_glue_ntb); - if (ncm_interface.xmit_glue_ntb != NULL) { + // Snapshot and clear the old glue NTB under lock so the USB task cannot + // race on xmit_glue_ntb while we decide what to do with the old buffer. + osal_spin_lock(&s_xmit_glue_lock, false); + xmit_ntb_t *old_ntb = ncm_interface.xmit_glue_ntb; + ncm_interface.xmit_glue_ntb = NULL; + osal_spin_unlock(&s_xmit_glue_lock, false); + + if (old_ntb != NULL) { // put NTB into waiting list (the new datagram did not fit in) - xmit_put_ntb_into_ready_list(ncm_interface.xmit_glue_ntb); + xmit_put_ntb_into_ready_list(old_ntb); } - // Capture into local: a concurrent SET_INTERFACE or DMA-done callback can - // zero ncm_interface.xmit_glue_ntb between a NULL check and the next read. - // Using a local guarantees the pointer we check is the pointer we dereference. + // Allocate next free NTB — non-blocking array scan, no lock needed here. xmit_ntb_t *ntb = xmit_get_free_ntb(); if (ntb == NULL) { TU_LOG_DRV(" xmit_setup_next_glue_ntb - nothing free\n");// should happen rarely - ncm_interface.xmit_glue_ntb = NULL; return false; } - ncm_interface.xmit_glue_ntb_datagram_ndx = 0; - - // Fill in NTB header via local — no re-read of the global + // Fill in NTB and NDP16 headers via local variable — no shared-state access. ntb->nth.dwSignature = NTH16_SIGNATURE; ntb->nth.wHeaderLength = sizeof(ntb->nth); ntb->nth.wSequence = ncm_interface.xmit_sequence++; ntb->nth.wBlockLength = sizeof(ntb->nth) + sizeof(ntb->ndp) + sizeof(ntb->ndp_datagram); ntb->nth.wNdpIndex = sizeof(ntb->nth); - // Fill in NDP16 header and terminator ntb->ndp.dwSignature = NDP16_SIGNATURE_NCM0; ntb->ndp.wLength = sizeof(ntb->ndp) + sizeof(ntb->ndp_datagram); ntb->ndp.wNextNdpIndex = 0; memset(ntb->ndp_datagram, 0, sizeof(ntb->ndp_datagram)); - // Publish to global only after NTB is fully initialised + // Publish: set datagram_ndx and xmit_glue_ntb together under lock so the + // USB task always sees a consistent (ptr, ndx) pair. + osal_spin_lock(&s_xmit_glue_lock, false); + ncm_interface.xmit_glue_ntb_datagram_ndx = 0; ncm_interface.xmit_glue_ntb = ntb; + osal_spin_unlock(&s_xmit_glue_lock, false); return true; } // xmit_setup_next_glue_ntb @@ -800,21 +816,26 @@ bool tud_network_can_xmit(uint16_t size) { void tud_network_xmit(void *ref, uint16_t arg) { TU_LOG_DRV("tud_network_xmit(%p, %d)\n", ref, arg); - // Capture into local to avoid re-reading the global after the NULL check - // (a concurrent SET_INTERFACE can zero xmit_glue_ntb between the two reads). + // Snapshot xmit_glue_ntb and reserve the datagram slot atomically so that + // a concurrent SET_INTERFACE (ncm_flush_data_paths) cannot zero the pointer + // or re-use the slot between our NULL check and our NTB write. + osal_spin_lock(&s_xmit_glue_lock, false); xmit_ntb_t *ntb = ncm_interface.xmit_glue_ntb; if (ntb == NULL) { + osal_spin_unlock(&s_xmit_glue_lock, false); TU_LOG_DRV("(EE) tud_network_xmit: no buffer\n");// must not happen (really) return; } + uint16_t ndx = ncm_interface.xmit_glue_ntb_datagram_ndx; + ncm_interface.xmit_glue_ntb_datagram_ndx = (uint16_t)(ndx + 1); + osal_spin_unlock(&s_xmit_glue_lock, false); - // copy new datagram to the end of the current NTB + // copy new datagram to the end of the current NTB (ntb and ndx exclusively ours) uint16_t size = tud_network_xmit_cb(ntb->data + ntb->nth.wBlockLength, ref, arg); - // correct NTB internals - ntb->ndp_datagram[ncm_interface.xmit_glue_ntb_datagram_ndx].wDatagramIndex = ntb->nth.wBlockLength; - ntb->ndp_datagram[ncm_interface.xmit_glue_ntb_datagram_ndx].wDatagramLength = size; - ncm_interface.xmit_glue_ntb_datagram_ndx += 1; + // correct NTB internals using captured slot index + ntb->ndp_datagram[ndx].wDatagramIndex = ntb->nth.wBlockLength; + ntb->ndp_datagram[ndx].wDatagramLength = size; ntb->nth.wBlockLength += (uint16_t) (size + XMIT_ALIGN_OFFSET(size));