linux-kernel - Re: [External] 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] [day] [month] [year] [list]
Message-ID: <CAEEQ3wn6tY7QQppTScBMQTeAZDTXdEY1CJYxvGHhE9FNm_4a7g@mail.gmail.com>
Date: Wed, 28 May 2025 11:29:55 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: arnd@...db.de, andriy.shevchenko@...ux.intel.com, 
	benjamin.larsson@...exis.eu, 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: [External] Re: [PATCH v6 2/4] serial: 8250: avoid potential
 PSLVERR issue

Hi John Ogness,

On Wed, May 21, 2025 at 3:57 PM John Ogness <john.ogness@...utronix.de> wrote:
>
> 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.
>

Okay.


> > @@ -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.

Reasonable. disable_rsa() has only one caller currently, to be updated
in the next version.

>
> 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);

Thanks,
Yunhui