Skip to content

fix(usb-ncm): NULL deref race in xmit_setup_next_glue_ntb + osal_spin_lock protection (T-145) #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chipsoft merged 2 commits into from
May 23, 2026
Merged

fix(usb-ncm): NULL deref race in xmit_setup_next_glue_ntb + osal_spin_lock protection (T-145) #4

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
495f3bb
fix(usb-ncm): eliminate NULL-deref race in xmit_setup_next_glue_ntb a…
chipsoft May 17, 2026
7c7d8e6
fix(usb-ncm): add osal_spin_lock critical sections to xmit_glue_ntb s…
chipsoft May 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 45 additions & 18 deletions src/class/net/ncm_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -490,34 +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);
}

ncm_interface.xmit_glue_ntb = xmit_get_free_ntb();// get next buffer (if any)
if (ncm_interface.xmit_glue_ntb == NULL) {
// 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
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 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: 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

Expand Down Expand Up @@ -795,20 +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);

if (ncm_interface.xmit_glue_ntb == NULL) {
// 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);

xmit_ntb_t *ntb = ncm_interface.xmit_glue_ntb;

// 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));

Expand Down
Loading