[PATCH] tty: serial: lpuart: Add port lock for registers read in lpuart32_set_termios()

[PATCH] tty: serial: lpuart: Add port lock for registers read in lpuart32_set_termios()

[Sherry Sun (OSS)]

From: Sherry Sun <sherry.sun@nxp.com>

lpuart32_set_termios() reads UARTCTRL at function entry without holding
the port lock. lpuart32_console_write() can run concurrently on another
CPU and temporarily clears the RIE (Receiver Interrupt Enable) bit while
it polls characters out. If the unlocked read in set_termios lands inside
that window, the stale snapshot is written back, permanently clearing
RIE. Once lost, no normal code path re-enables them, so the UART stops
receiving characters and becomes unresponsive.

Add port lock for the registers read in lpuart32_set_termios() to avoid
the unlocked snapshot captured.

Please note that this only fix the normal printk path, in the
oops_in_progress path of lpuart32_console_write(), when trylock fails,
RIE is cleared and old_cr restored without the port lock held, there is
still a risk of stale snapshot, but it's acceptable for the post-oops
state and the trylock-fail window.

Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index cd2575a3aef7..7c249b9371ae 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2241,9 +2241,11 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned int  baud;
 	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
 
+	uart_port_lock_irqsave(port, &flags);
 	ctrl = old_ctrl = lpuart32_read(port, UARTCTRL);
 	bd = lpuart32_read(port, UARTBAUD);
 	modem = lpuart32_read(port, UARTMODIR);
+	uart_port_unlock_irqrestore(port, flags);
 	sport->is_cs7 = false;
 	/*
 	 * only support CS8 and CS7

---
base-commit: 793807ca13bf3072fc6d16ce6d90e9ab9beff2a5
change-id: 20261111-lpuart_port_lock-01f7deab451a

Best regards,
-- 
Sherry Sun <sherry.sun@nxp.com>

Re: [PATCH] tty: serial: lpuart: Add port lock for registers read in lpuart32_set_termios()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] tty: serial: lpuart: split port->lock race in
  lpuart32_set_termios()
--

commit c6586e620953933a073532d069075858d2abff60
Author: Sherry Sun <sherry.sun@nxp.com>

tty: serial: lpuart: Add port lock for registers read in lpuart32_set_termios()

This commit wraps the initial register reads of UARTCTRL, UARTBAUD, and
UARTMODIR in lpuart32_set_termios() with uart_port_lock_irqsave() to avoid
reading a stale UARTCTRL value while lpuart32_console_write() temporarily
clears RIE.

> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index cd2575a3aef74..7c249b9371ae8 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2241,9 +2241,11 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
>  	unsigned int  baud;
>  	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	ctrl = old_ctrl = lpuart32_read(port, UARTCTRL);
>  	bd = lpuart32_read(port, UARTBAUD);
>  	modem = lpuart32_read(port, UARTMODIR);
> +	uart_port_unlock_irqrestore(port, flags);
>  	sport->is_cs7 = false;
>  	/*
>  	 * only support CS8 and CS7

[Severity: Medium]
Does releasing port->lock right after reading UARTCTRL, UARTBAUD, and
UARTMODIR still leave a window where ctrl and old_ctrl become stale before
they are written back to hardware?

CPU0                                  CPU1
lpuart32_set_termios()
  uart_port_lock_irqsave()
  ctrl = old_ctrl =
    lpuart32_read(UARTCTRL)
    (UARTCTRL_TIE is 0)
  uart_port_unlock_irqrestore()
                                      uart_write()
                                        lpuart32_start_tx()
                                          uart_port_lock_irqsave()
                                          lpuart32_write(ctrl | UARTCTRL_TIE,
                                                         UARTCTRL)
                                          uart_port_unlock_irqrestore()
  uart_get_baud_rate()
  lpuart_dma_rx_free()
  uart_port_lock_irqsave()
  ...
  lpuart32_write(ctrl, UARTCTRL)  <-- clears UARTCTRL_TIE, stalling TX

Callers such as lpuart32_start_tx(), lpuart32_stop_tx(),
lpuart32_stop_rx(), and lpuart32_set_mctrl() update UARTCTRL under
port->lock without holding tty_port->mutex.  Any modification made during
the unlocked window is overwritten when lpuart32_set_termios() re-acquires
port->lock and writes old_ctrl and ctrl back to UARTCTRL.  Clearing
sport->is_cs7 outside port->lock also races with lpuart32_rxint() and
lpuart_copy_rx_to_tty().

Since neither uart_get_baud_rate() nor lpuart_dma_rx_free() uses ctrl,
old_ctrl, bd, or modem, the register reads and bit manipulations could be
moved into the existing uart_port_lock_irqsave() critical section after
lpuart_dma_rx_free() so the read-modify-write stays under a single lock
acquisition.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-lpuart_port_lock-v1-1-5d332c51edfe@nxp.com?part=1