This is an automated email from the ASF dual-hosted git repository.
acassis pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
commit a3f8b55143ad8ade1da2df0d848b5adb3a15cd9d
Author: Kerogit <kr....@kerogit.eu>
AuthorDate: Wed Jun 18 20:14:45 2025 +0200
drivers/serial/serial: prevent race conditions on 8-bit architectures
Some code paths in drivers/serial/serial.c load head and tail values
of receive and transmit circular buffers with interrupts enabled,
making it possible that the interrupt handler changes the value.
As noted in the code, this is safe as long as the load itself is atomic.
That is not true for 8bit architectures which fetch the 16-bit values
using two load instructions. If interrupt handler runs between those
two instructions and changes the value, the read returns corrupted data.
This patch introduces CONFIG_ARCH_LDST_16BIT_NOT_ATOMIC configuration
option which is automatically selected for AVR architecture. Based
on this option, head and tail values are reduced to 8-bit length
so the read remains atomic.
Patch was tested by building on rv-virt:nsh - disassembly of functions
from serial.c showed no difference which is correct as Risc-V does
not need to protect reads of these values. There should be no impact
for architectures that do not set the new configuration option.
It was also tested by by custom echo application running on AVR128DA28.
Signed-off-by: Kerogit <kr....@kerogit.eu>
---
arch/Kconfig | 17 ++++++
arch/avr/Kconfig | 1 +
drivers/serial/Kconfig | 9 ++++
drivers/serial/Kconfig-usart | 120 +++++++++++++++++++++++++++++++++++-------
drivers/serial/serial.c | 14 +++--
include/nuttx/serial/serial.h | 24 +++++++--
6 files changed, 155 insertions(+), 30 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index af0fd00f6a..8ce7ca2edf 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -492,6 +492,23 @@ config ARCH_HAVE_RESET
bool
default n
+config ARCH_LDST_16BIT_NOT_ATOMIC
+ bool
+ default n
+ select ARCH_HAVE_8BIT_SERIAL_BUFSIZE
+ ---help---
+ This configuration option denotes architecture incapable of loading
+ 16-bit values atomically, ie. in a single instruction. If this is set,
+ it means that the architecture needs two (or more) instructions
+ to perform such load AND that a interrupt service routine may run
+ between their execution. The same applies for stores.
+
+ If set, any code running in non-interrupt context that reads 16-bit
+ variable that may be written in interrupt context must disable
+ interrupts or use another method that makes the read atomic.
+ (The same applies for non-interrupt context write and interrupt
+ context read.)
+
config ARCH_HAVE_TESTSET
bool
default n
diff --git a/arch/avr/Kconfig b/arch/avr/Kconfig
index 2c391c4a97..394194e792 100644
--- a/arch/avr/Kconfig
+++ b/arch/avr/Kconfig
@@ -49,6 +49,7 @@ config ARCH_FAMILY_AVR
bool
default n
select ARCH_HAVE_STACKCHECK
+ select ARCH_LDST_16BIT_NOT_ATOMIC
config ARCH_FAMILY_AVR32
bool
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 43bf6c8e0e..f6b9100a15 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -7,6 +7,15 @@ config ARCH_HAVE_SERIAL_TERMIOS
bool
default n
+config ARCH_HAVE_8BIT_SERIAL_BUFSIZE
+ bool
+ default n
+ ---help---
+ This is set by ARCH_LDST_16BIT_NOT_ATOMIC if the architecture does
+ not support atomic 16-bit load/store instructions. Receive and transmit
+ buffer sizes are stored in uint8_t (instead of int16_t) variable
+ in such case.
+
menuconfig SERIAL
bool "Serial Driver Support"
default y
diff --git a/drivers/serial/Kconfig-usart b/drivers/serial/Kconfig-usart
index e11045ad46..b14fb20029 100644
--- a/drivers/serial/Kconfig-usart
+++ b/drivers/serial/Kconfig-usart
@@ -58,18 +58,26 @@ menu "USART0 Configuration"
config USART0_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART0_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART0_BAUD
int "BAUD rate"
default 115200
@@ -130,18 +138,26 @@ menu "USART1 Configuration"
config USART1_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART1_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART1_BAUD
int "BAUD rate"
default 115200
@@ -202,18 +218,26 @@ menu "USART2 Configuration"
config USART2_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART2_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART2_BAUD
int "BAUD rate"
default 115200
@@ -273,18 +297,26 @@ menu "USART3 Configuration"
config USART3_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART3_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART3_BAUD
int "BAUD rate"
default 115200
@@ -345,18 +377,26 @@ menu "USART4 Configuration"
config USART4_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART4_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART4_BAUD
int "BAUD rate"
default 115200
@@ -417,18 +457,26 @@ menu "USART5 Configuration"
config USART5_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART5_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART5_BAUD
int "BAUD rate"
default 115200
@@ -489,18 +537,26 @@ menu "USART6 Configuration"
config USART6_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART6_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART6_BAUD
int "BAUD rate"
default 115200
@@ -561,18 +617,26 @@ menu "USART7 Configuration"
config USART7_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART7_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART7_BAUD
int "BAUD rate"
default 115200
@@ -633,18 +697,26 @@ menu "USART8 Configuration"
config USART8_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART8_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART8_BAUD
int "BAUD rate"
default 115200
@@ -705,18 +777,26 @@ menu "USART9 Configuration"
config USART9_RXBUFSIZE
int "Receive buffer size"
- default 256
+ default 252
---help---
Characters are buffered as they are received. This specifies
the size of the receive buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART9_TXBUFSIZE
int "Transmit buffer size"
- default 256
+ default 252
---help---
Characters are buffered before being sent. This specifies
the size of the transmit buffer.
+ Note that on architectures that are unable to load 16-bit
+ values atomically without eg. disabling interrupts, the size
+ of the buffer is stored in uint8_t variable.
+
config USART9_BAUD
int "BAUD rate"
default 115200
diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 9f25b2a04c..f5aaec1953 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -901,13 +901,13 @@ static ssize_t uart_readv(FAR struct file *filep, FAR
struct uio *uio)
#ifdef CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS
unsigned int nbuffered;
unsigned int watermark;
- int16_t head;
+ sbuf_size_t head;
#endif
irqstate_t flags;
ssize_t recvd = 0;
ssize_t buflen;
bool echoed = false;
- int16_t tail;
+ sbuf_size_t tail;
char ch;
int ret;
@@ -954,9 +954,13 @@ static ssize_t uart_readv(FAR struct file *filep, FAR
struct uio *uio)
* index is only modified in this function. Therefore, no
* special handshaking is required here.
*
- * The head and tail pointers are 16-bit values. The only time that
- * the following could be unsafe is if the CPU made two non-atomic
- * 8-bit accesses to obtain the 16-bit head index.
+ * The head and tail pointers values are sized based
+ * on the architecture. If the architecture reads 16-bit values
+ * atomically by nature, they are 16-bit values. On architectures
+ * where 16-bit access is split into two non-atomic 8-bit accesses,
+ * the pointers are 8-bit.
+ *
+ * The following code is therefore safe even with interrupts enabled.
*/
tail = rxbuf->tail;
diff --git a/include/nuttx/serial/serial.h b/include/nuttx/serial/serial.h
index 8e916ecab1..57eb81492f 100644
--- a/include/nuttx/serial/serial.h
+++ b/include/nuttx/serial/serial.h
@@ -123,15 +123,29 @@
/* This structure defines one serial I/O buffer.
* The serial infrastructure will initialize the 'sem' field but all other
* fields must be initialized by the caller of uart_register().
+ *
+ * Maximum buffer size is reduced to 8 bits on architectures where 16bit
+ * load takes two instructions and is therefore not atomic. This prevents
+ * corrupted read if the value is changed in an interrupt handler while
+ * being loaded in non-interrupt code.
*/
+#ifndef CONFIG_ARCH_LDST_16BIT_NOT_ATOMIC
+typedef int16_t sbuf_size_t;
+#else
+typedef uint8_t sbuf_size_t;
+#endif
+
struct uart_buffer_s
{
- mutex_t lock; /* Used to control exclusive access to the buffer */
- volatile int16_t head; /* Index to the head [IN] index in the buffer */
- volatile int16_t tail; /* Index to the tail [OUT] index in the buffer */
- int16_t size; /* The allocated size of the buffer */
- FAR char *buffer; /* Pointer to the allocated buffer memory */
+ mutex_t lock; /* Used to control exclusive access
+ * to the buffer */
+ volatile sbuf_size_t head; /* Index to the head [IN] index
+ * in the buffer */
+ volatile sbuf_size_t tail; /* Index to the tail [OUT] index
+ * in the buffer */
+ sbuf_size_t size; /* The allocated size of the buffer */
+ FAR char *buffer; /* Pointer to the allocated buffer memory */
};
#if defined(CONFIG_SERIAL_RXDMA) || defined(CONFIG_SERIAL_TXDMA)
