From e6ae322946a4d4b451510d8a188f536d837d60f6 Mon Sep 17 00:00:00 2001 From: raiden00pl Date: Mon, 7 Sep 2026 20:31:47 +0200 Subject: [PATCH 1/2] drivers/serial: sample xmit.head once per iteration in uart_xmitchars() uart_putxmitchar() advances xmit.head from thread context without holding the critical section, so on SMP the head index can move, and wrap around, while uart_xmitchars() runs in the TX interrupt on another CPU. Since commit b319c27f03e ("serial: Added APIs for receiving and sending multiple chars") the sendbuf path of uart_xmitchars() reads xmit.head twice: once to decide whether the pending data is contiguous and again to compute its length. If the producer wraps the index in between, the computed length goes negative, is passed to sendbuf() as a huge size_t and the driver transmits memory far beyond the ring buffer. The per-byte path reads the index only once and is not affected, which is why this went unnoticed: the batch path is only used by drivers that implement sendbuf, and the 16550 driver gained it in commit 45c38d8592b ("drivers/serial/16550: add polling mode support for serial drivers"). qemu-intel64 with SMP is the first configuration combining a sendbuf driver with a producer running on another CPU. On qemu-intel64 SMP this shows up as an endless stream of NUL bytes on the console (captured with gdb: head = 1, tail = 8, size = 16, and u16550_sendbuf() called with size = (size_t)-7), which makes the ntfc test harness fail every test that runs while the flood lasts. Read the head index once per loop iteration and use that snapshot for both the contiguity test and the length. The producer only ever moves the index forward, so a stale snapshot merely sends less now. Assisted-by: Claude Code Signed-off-by: raiden00pl --- drivers/serial/serial_io.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/serial/serial_io.c b/drivers/serial/serial_io.c index 0f8bc6b270869..22ee415901709 100644 --- a/drivers/serial/serial_io.c +++ b/drivers/serial/serial_io.c @@ -57,14 +57,22 @@ void uart_xmitchars(FAR uart_dev_t *dev) { uint16_t nbytes = 0; + sbuf_size_t head; #ifdef CONFIG_SMP irqstate_t flags = enter_critical_section(); #endif - /* Send while we still have data in the TX buffer & room in the fifo */ + /* Send while we still have data in the TX buffer & room in the fifo. + * + * uart_putxmitchar() advances xmit.head from thread context without + * holding the critical section, so on SMP it can move (and wrap) while + * we are in here. Sample it once per iteration: a stale value only + * makes us send less now, whereas reading it twice can turn the batch + * length negative and send from far beyond the buffer. + */ - while (dev->xmit.head != dev->xmit.tail && uart_txready(dev)) + while ((head = dev->xmit.head) != dev->xmit.tail && uart_txready(dev)) { /* Send the next byte */ @@ -72,9 +80,9 @@ void uart_xmitchars(FAR uart_dev_t *dev) { ssize_t sent; - if (dev->xmit.tail < dev->xmit.head) + if (dev->xmit.tail < head) { - sent = dev->xmit.head - dev->xmit.tail; + sent = head - dev->xmit.tail; } else { From 58c12f29ba2b3a3bb1f7132770f94ae8c83599a6 Mon Sep 17 00:00:00 2001 From: raiden00pl Date: Tue, 8 Sep 2026 09:40:31 +0200 Subject: [PATCH 2/2] drivers/serial: read the consumer index once in uart_recvchars() Same issue as the previous commit, on the receive side: uart_read() advances recv.tail from thread context without holding the critical section, but the recvbuf batch path of uart_recvchars() reads recv.tail several times (the full check, the watermark count and the free-space computation). If uart_read() moves and wraps the index in between, the computed free space goes negative and is passed to recvbuf() as a huge size_t, which lets the driver store past the end of the ring buffer. Read recv.tail once per loop iteration and derive everything from that snapshot. The consumer only ever moves the index forward, so a stale snapshot merely stores less now. Assisted-by: Claude Code Signed-off-by: raiden00pl --- drivers/serial/serial_io.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/serial/serial_io.c b/drivers/serial/serial_io.c index 22ee415901709..3361c11f3f416 100644 --- a/drivers/serial/serial_io.c +++ b/drivers/serial/serial_io.c @@ -172,8 +172,16 @@ void uart_recvchars(FAR uart_dev_t *dev) while (uart_rxavailable(dev)) { + /* uart_read() advances recv.tail from thread context without holding + * the critical section, so on SMP it can move (and wrap) while we are + * in here. Sample it once per iteration and derive the free space + * from that snapshot: a stale value only makes us store less now, + * whereas reading it twice can turn the batch length negative. + */ + int nexthead = rxbuf->head + 1 < rxbuf->size ? rxbuf->head + 1 : 0; - bool is_full = (nexthead == rxbuf->tail); + sbuf_size_t tail = rxbuf->tail; + bool is_full = (nexthead == tail); FAR char *pbuf = NULL; char ch; @@ -182,13 +190,13 @@ void uart_recvchars(FAR uart_dev_t *dev) /* How many bytes are buffered */ - if (rxbuf->head >= rxbuf->tail) + if (rxbuf->head >= tail) { - nbuffered = rxbuf->head - rxbuf->tail; + nbuffered = rxbuf->head - tail; } else { - nbuffered = rxbuf->size - rxbuf->tail + rxbuf->head; + nbuffered = rxbuf->size - tail + rxbuf->head; } /* Is the level now above the watermark level that we need to report? */ @@ -231,11 +239,11 @@ void uart_recvchars(FAR uart_dev_t *dev) if (!is_full) { - if (rxbuf->tail > rxbuf->head) + if (tail > rxbuf->head) { - nbytes = rxbuf->tail - rxbuf->head - 1; + nbytes = tail - rxbuf->head - 1; } - else if (rxbuf->tail) + else if (tail) { nbytes = rxbuf->size - rxbuf->head; }