On 2024-01-12, Gui-Dong Han 2045gemini@gmail.com wrote:
In uart_tiocmget(): result = uport->mctrl; uart_port_lock_irq(uport); result |= uport->ops->get_mctrl(uport); uart_port_unlock_irq(uport); ... return result;
In uart_update_mctrl(): uart_port_lock_irqsave(port, &flags); ... port->mctrl = (old & ~clear) | set; ... uart_port_unlock_irqrestore(port, flags);
An atomicity violation is identified due to the concurrent execution of uart_tiocmget() and uart_update_mctrl(). After assigning result = uport->mctrl, the mctrl value may change in uart_update_mctrl(), leading to a mismatch between the value returned by uport->ops->get_mctrl(uport) and the mctrl value previously read. This can result in uart_tiocmget() returning an incorrect value.
This possible bug is found by an experimental static analysis tool developed by our team, BassCheck[1]. This tool analyzes the locking APIs to extract function pairs that can be concurrently executed, and then analyzes the instructions in the paired functions to identify possible concurrency bugs including data races and atomicity violations. The above possible bug is reported when our tool analyzes the source code of Linux 5.17.
To address this issue, it is suggested to move the line result = uport->mctrl inside the uart_port_lock block to ensure atomicity and prevent the mctrl value from being altered during the execution of uart_tiocmget(). With this patch applied, our tool no longer reports the bug, with the kernel configuration allyesconfig for x86_64. Due to the absence of the requisite hardware, we are unable to conduct runtime testing of the patch. Therefore, our verification is solely based on code logic analysis.
[1] https://sites.google.com/view/basscheck/
Fixes: 559c7ff4e324 ("serial: core: Use port lock wrappers")
It fixes c5f4644e6c8b ("[PATCH] Serial: Adjust serial locking").
Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han 2045gemini@gmail.com
drivers/tty/serial/serial_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 80085b151b34..a9e39416d877 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1085,8 +1085,8 @@ static int uart_tiocmget(struct tty_struct *tty) goto out; if (!tty_io_error(tty)) {
result = uport->mctrl;
result = uport->mctrl;
Looking over the RMW accesses to @mctrl, I expect you will also need this hunk:
@@ -2242,6 +2242,7 @@ uart_set_options(struct uart_port *port, struct console *co, { struct ktermios termios; static struct ktermios dummy; + unsigned long flags;
/* * Ensure that the serial-console lock is initialised early. @@ -2279,7 +2280,9 @@ uart_set_options(struct uart_port *port, struct console *co, * some uarts on other side don't support no flow control. * So we set * DTR in host uart to make them happy */ + uart_port_lock_irqsave(port, &flags); port->mctrl |= TIOCM_DTR; + uart_port_unlock_irqrestore(port, flags);
port->ops->set_termios(port, &termios, &dummy); /*
FWIW, Reviewed-by: John Ogness john.ogness@linutronix.de