linux-kernel - Re: [PATCH v3] serial: 8250: fix panic due to PSLVERR

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: <Z_zLqH1Moavhi52x@smile.fi.intel.com>
Date: Mon, 14 Apr 2025 11:47:36 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Yunhui Cui <cuiyunhui@...edance.com>
Cc: ilpo.jarvinen@...ux.intel.com, gregkh@...uxfoundation.org,
	jirislaby@...nel.org, john.ogness@...utronix.de, pmladek@...e.com,
	arnd@...db.de, namcao@...utronix.de, benjamin.larsson@...exis.eu,
	schnelle@...ux.ibm.com, heikki.krogerus@...ux.intel.com,
	markus.mayer@...aro.org, tim.kryger@...aro.org,
	matt.porter@...aro.org, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH v3] serial: 8250: fix panic due to PSLVERR

On Mon, Apr 14, 2025 at 11:14:50AM +0800, Yunhui Cui wrote:

You forgot to rebase against latest tty-next or, if there is something
in the latter (but I don't see right now), even tty-testing.

> When the PSLVERR_RESP_EN parameter is set to 1, the device generates
> an error response if an attempt is made to read an empty RBR (Receive
> Buffer Register) while the FIFO is enabled.
> 
> In serial8250_do_startup(), calling serial_port_out(port, UART_LCR,
> UART_LCR_WLEN8) triggers dw8250_check_lcr(), which invokes
> dw8250_force_idle() and serial8250_clear_and_reinit_fifos(). The latter
> function enables the FIFO via serial_out(p, UART_FCR, p->fcr).
> Execution proceeds to the dont_test_tx_en label:
> ...
> serial_port_in(port, UART_RX);
> This satisfies the PSLVERR trigger condition.
> 
> Because another CPU(e.g., using printk()) is accessing the UART (UART
> is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) ==
> (lcr & ~UART_LCR_SPAR), causing it to enter dw8250_force_idle().
> 
> To fix this, all calls to serial_out(UART_LCR) and serial_in(UART_RX)
> should be executed under port->lock. Additionally, checking the readiness
> via UART_LSR should also be done under port->lock.
> 
> Panic backtrace:
> [    0.442336] Oops - unknown exception [#1]
> [    0.442343] epc : dw8250_serial_in32+0x1e/0x4a
> [    0.442351]  ra : serial8250_do_startup+0x2c8/0x88e
> ...
> [    0.442416] console_on_rootfs+0x26/0x70

This patch seems need split to three. See below.

...

First of all, while everything looks better now, there is a chance in the
future to miss the same issue again. In order to avoid that I suggest to
introduce a new helper where you made this check _and_ add a comment why.

(Note that currently you have a mixture of serial_in()/serial_port_in() in
 some cases.)

static inline unsigned int serial8250_discard_data(struct uart_8250_port *up)
{
	u16 lsr;

	lsr = serial_in(up, UART_LSR);
	if (lsr & UART_LSR_DR)
		return serial_in(up, UART_RX);

	return 0;
}

And this can be one patch (patch 2).

...

> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

Changes here deserve the separate patch (patch 1).

...

> +	/*
> +	 * Serial_in(p, UART_RX) should be under port->lock, but we can't add

serial_in()

> +	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
> +	 * is under port->lock.
> +	 */
> +	lockdep_assert_held_once(&p->lock);

...

> +	uart_port_lock_irqsave(port, &flags);

And one patch (patch 3) about locking.

-- 
With Best Regards,
Andy Shevchenko