linux-kernel - Re: [PATCH v6 2/4] serial: 8250: avoid potential PSLVERR issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84y0uq9z0o.fsf@jogness.linutronix.de>
Date: Wed, 21 May 2025 10:03:43 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Yunhui Cui <cuiyunhui@...edance.com>, arnd@...db.de,
 andriy.shevchenko@...ux.intel.com, benjamin.larsson@...exis.eu,
 cuiyunhui@...edance.com, gregkh@...uxfoundation.org,
 heikki.krogerus@...ux.intel.com, ilpo.jarvinen@...ux.intel.com,
 jirislaby@...nel.org, jkeeping@...usicbrands.com,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
 markus.mayer@...aro.org, matt.porter@...aro.org, namcao@...utronix.de,
 paulmck@...nel.org, pmladek@...e.com, schnelle@...ux.ibm.com,
 sunilvl@...tanamicro.com, tim.kryger@...aro.org
Subject: Re: [PATCH v6 2/4] serial: 8250: avoid potential PSLVERR issue

On 2025-05-13, Yunhui Cui <cuiyunhui@...edance.com> wrote:
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 07fe818dffa34..9a04f24b0c762 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2133,25 +2132,22 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>  static int serial8250_get_poll_char(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> -	int status;
> +	int status = NO_POLL_CHAR;
>  	u16 lsr;
> +	unsigned long flags;
>  
>  	serial8250_rpm_get(up);
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	lsr = serial_port_in(port, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		status = serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);

I realize I previously made a comment saying it was OK to add the spin
locking here. But I have changed my mind. Please remove this spin
locking. It is not necessary because with kgdb all the other CPUs are
quiesced, so there is no need to synchronize with the console. Also, it
will deadlock if kgdb took over while the port was locked.

> @@ -2513,7 +2514,6 @@ void serial8250_do_shutdown(struct uart_port *port)
>  		port->mctrl &= ~TIOCM_OUT2;
>  
>  	serial8250_set_mctrl(port, port->mctrl);
> -	uart_port_unlock_irqrestore(port, flags);
>  
>  	/*
>  	 * Disable break condition and FIFOs
> @@ -2521,6 +2521,14 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	serial_port_out(port, UART_LCR,
>  			serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
>  	serial8250_clear_fifos(up);
> +	/*
> +	 * Read data port to reset things, and then unlink from
> +	 * the IRQ chain.
> +	 * Since reading UART_RX clears interrupts, doing so with
> +	 * FIFO disabled won't trigger PSLVERR.
> +	 */
> +	serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
>  
>  #ifdef CONFIG_SERIAL_8250_RSA
>  	/*
> @@ -2529,11 +2537,6 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	disable_rsa(up);
>  #endif
>  
> -	/*
> -	 * Read data port to reset things, and then unlink from
> -	 * the IRQ chain.
> -	 */
> -	serial_port_in(port, UART_RX);

I am thinking you should keep the read here and instead move the unlock
below the read. This would mean the lock/unlock in disable_rsa() need to
be removed. (The function comments for disable_rsa() aready say that the
caller needs to hold the port lock.)

I am thinking something like the below (untested) diff instead of the
above 2 hunks.

John Ogness

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 22b3f7a193070..51467383aaf5a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -781,8 +781,6 @@ static void disable_rsa(struct uart_8250_port *up)
 
 	if (up->port.type == PORT_RSA &&
 	    up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
-		uart_port_lock_irq(&up->port);
-
 		mode = serial_in(up, UART_RSA_MSR);
 		result = !(mode & UART_RSA_MSR_FIFO);
 
@@ -794,7 +792,6 @@ static void disable_rsa(struct uart_8250_port *up)
 
 		if (result)
 			up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
-		uart_port_unlock_irq(&up->port);
 	}
 }
 #endif /* CONFIG_SERIAL_8250_RSA */
@@ -2536,7 +2533,6 @@ void serial8250_do_shutdown(struct uart_port *port)
 		port->mctrl &= ~TIOCM_OUT2;
 
 	serial8250_set_mctrl(port, port->mctrl);
-	uart_port_unlock_irqrestore(port, flags);
 
 	/*
 	 * Disable break condition and FIFOs
@@ -2555,8 +2551,12 @@ void serial8250_do_shutdown(struct uart_port *port)
 	/*
 	 * Read data port to reset things, and then unlink from
 	 * the IRQ chain.
+	 *
+	 * Since reading UART_RX clears interrupts, doing so with
+	 * FIFO disabled won't trigger PSLVERR.
 	 */
 	serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 	serial8250_rpm_put(up);
 
 	up->ops->release_irq(up);