jerpelea opened a new pull request, #16466:
URL: https://github.com/apache/nuttx/pull/16466
## Summary
While going through the code in drivers/serial/serial.c, I noticed this
comment:
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.
This is what happens for (at least) AVR architecture. These are 8bit
microcontrollers and as such, they will fetch the 16-bit value in two
8-bit load instructions; interrupt routine may execute between those and
change the value being read. This will result in corrupted read (one
byte of the value will be from pre-interrupt state and the other from
post-interrupt state.)
This patch introduces CONFIG_ARCH_LD_16BIT_NOT_ATOMIC configuration
option, which is automatically selected for architectures known to be
unable to load 16bit values in one instruction. (It is currently only
set for AVR. I presume it might be also useful for Z80 but I do not have
any experience with that architecture so I did no changes there.) When
this configuration option is set, reads of recv.head and xmit.tail are
enclosed with enter_critical_section and leave_critical_section calls to
prevent interrupt handlers from running, if needed. Not all reads need
to be protected this way - some are already in existing critical
sections and some happen with the UART-specific interrupt disabled.
If the configuration option is not set, the code simply loads the value
into a local variable. Subsequent direct uses of the unprotected
volatile variable are replaced with the local variables for both cases.
There is also a related change that only applies when
CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS is set. Aside from the protection
selected by CONFIG_ARCH_LD_16BIT_NOT_ATOMIC, this patch also fixes
calculation of the nbuffered value. This calculation is not running with
interrupts disabled and value of rxbuf->head can change between the
condition and actual computation. Even if the load itself is atomic,
this leads to TOCTOU error.
## Impact
this change should not impact architectures that do not benefit
from this change at all unless CONFIG_SERIAL_IFLOWCONTROL is set. If
CONFIG_SERIAL_IFLOWCONTROL is set, the only change that remains in
effect is the fix of the TOCTOU error.
## Testing
Patch was tested with rv-virt:nsh - disassembly of functions
from drivers/serial/serial.c was compared and did not change after the
patch was applied - with one exception. The exception was an address of
a global variable, I assume it was caused by change of g_version length
(which notes that the tree is dirty) and is therefore inconsequential.
CONFIG_SERIAL_IFLOWCONTROL was not set in this test.
Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by building
it for AVR DA/DB. Configuration with CONFIG_SERIAL_IFLOWCONTROL unset
was tested by custom echo application running on AVR128DA28 for a few
hours.
I also ran CI test: 1044 passed, 10 skipped, 14 deselected, 3 warnings
in 2463.23s (0:41:03)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
