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

[sashiko-bot@kernel.org]

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