linux-kernel - Re: [External] Re: [PATCH] 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: <CAEEQ3w=GnSF2Ka+nVM+HNZOmFxzcomPf9uDqOJZV=RU17OZijQ@mail.gmail.com>
Date: Thu, 3 Apr 2025 22:14:05 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, pmladek@...e.com, 
	arnd@...db.de, andriy.shevchenko@...ux.intel.com, namcao@...utronix.de, 
	benjamin.larsson@...exis.eu, schnelle@...ux.ibm.com, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [External] Re: [PATCH] serial: 8250: fix panic due to PSLVERR

Hi John,

On Thu, Apr 3, 2025 at 9:46 PM John Ogness <john.ogness@...utronix.de> wrote:
>
> On 2025-04-03, yunhui cui <cuiyunhui@...edance.com> wrote:
> >>> 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().
> >>
> >> Didn't this[0] patch resolve this exact issue?
> >>
> >> John Ogness
> >>
> >> [0] https://lore.kernel.org/lkml/20220713131722.2316829-1-vamshigajjela@google.com
> >
> > No, these are two separate issues. This[0] patch is necessary, as
> > expressed in this comment:
> >
> > /*
> > * With PSLVERR_RESP_EN parameter set to 1, the device generates an
> > * error response when an attempt to read an empty RBR with FIFO
> > * enabled.
> > */
> >
> > The current patch addresses the following scenario:
> >
> > cpuA is accessing the UART via printk(), causing the UART to be busy.
> > cpuB follows the CallTrace path:
> > -serial8250_do_startup()
> > --serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> > ---dw8250_serial_out32
> > ----dw8250_check_lcr
> > -----dw8250_force_idle (triggered by UART busy)
> > ------serial8250_clear_and_reinit_fifos
> > -------serial_out(p, UART_FCR, p->fcr); (enables FIFO here)
> > cpuB proceeds to the dont_test_tx_en label:
> >    ...
> >    serial_port_in(port, UART_RX); //FIFO is enabled, and the UART has
> > no data to read, causing the device to generate a PSLVERR error and
> > panic.
> >
> > Our solution:
> > Relevant serial_port_out operations should be placed in a critical section.
> > Before reading UART_RX, check if data is available (e.g., by verifying
> > the UART_LSR DR bit is set).
>
> OK, now I see. The problem is the explicit reads of UART_RX near the end
> of serial8250_do_startup().
>
> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> >> index 3f256e96c722..6909c81109db 100644
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -2264,13 +2264,16 @@ int serial8250_do_startup(struct uart_port *port)
> >>       * Clear the FIFO buffers and disable them.
> >>       * (they will be reenabled in set_termios())
> >>       */
> >> +    uart_port_lock_irqsave(port, &flags);
> >>      serial8250_clear_fifos(up);
> >> +    uart_port_unlock_irqrestore(port, flags);
>
> Can you clarify why serial8250_clear_fifos() needs to be in a critical
> section?

There are two aspects. Firstly, if the lock is not held, the following
situation may also occur when serial8250_clear_fifos() is called:
---dw8250_serial_out32
----dw8250_check_lcr
-----dw8250_force_idle (triggered by UART busy)
------serial8250_clear_and_reinit_fifos
-------serial_out(p, UART_FCR, p->fcr); (enables FIFO here)
This in itself goes against the semantics of clear_fifo.

Secondly, if a CPU is accessing the UART normally and the current CPU
suddenly clears the FIFO, it may cause problems.

>
> serial8250_do_shutdown() and do_set_rxtrig() also call
> serial8250_clear_fifos() without holding the port lock.
>
> >>>     /*
> >>>      * Clear the interrupt registers.
> >>>      */
> >>> -   serial_port_in(port, UART_LSR);
> >>> -   serial_port_in(port, UART_RX);
> >>> +   lsr = serial_port_in(port, UART_LSR);
> >>> +   if (lsr & UART_LSR_DR)
> >>> +           serial_port_in(port, UART_RX);
>
> Do we care about the unchecked UART_RX in serial8250_do_shutdown()?

I understand that it is required.

>
>         /*
>          * Read data port to reset things, and then unlink from
>          * the IRQ chain.
>          */
>         serial_port_in(port, UART_RX);
>         serial8250_rpm_put(up);
>
>         up->ops->release_irq(up);
> }
>
> Otherwise all other UART_RX reads are either checking UART_LSR_DR first
> or are under the port lock.

Agree

>
> John

Thanks,
Yunhui