linux-kernel - Re: [External] Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard

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: <CAEEQ3wnizUrMctT17krZNYUszT0n7mgW5cBHkMzOUF+j+ecMJA@mail.gmail.com>
Date: Tue, 6 May 2025 18:38:40 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: Jiri Slaby <jirislaby@...nel.org>
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, 
	jkeeping@...usicbrands.com, john.ogness@...utronix.de, 
	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 v4 2/4] serial: 8250: introduce serial8250_discard_data()

Hi js,

On Fri, Apr 25, 2025 at 2:49 PM Jiri Slaby <jirislaby@...nel.org> wrote:
>
> On 25. 04. 25, 8:24, Yunhui Cui wrote:
> > To prevent triggering PSLVERR, it is necessary to check whether the
> > UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
> > Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
> > under port->lock.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@...edance.com>
> > ---
> >   drivers/tty/serial/8250/8250.h      | 15 +++++++++++
> >   drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
> >   2 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index b861585ca02a..5a106cf88207 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -162,6 +162,21 @@ static inline u16 serial_lsr_in(struct uart_8250_port *up)
> >       return lsr;
> >   }
> >
> > +/*
> > + * To avoid PSLVERR, check UART_LSR_DR in UART_LSR before
> > + * reading UART_RX.
> > + */
> > +static inline unsigned int serial8250_discard_data(struct uart_8250_port *up)
> > +{
> > +     u16 lsr;
>
> Is lsr really 16-bit?

The data book I have shows a 32 - bit width, but actually only the
lower 8 bits are valid. Using u16 is fine, and the current code all
uses u16 lsr.

>
> > +
> > +     lsr = serial_in(up, UART_LSR);
> > +     if (lsr & UART_LSR_DR)
> > +             return serial_in(up, UART_RX);
>
> Why does a discard function return a value at all?

Update in next version.

>
> > +
> > +     return 0;
> > +}
> > +
> >   /*
> >    * For the 16C950
> >    */
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index a913135d5217..802ac50357c0 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> ...
> > @@ -2137,25 +2136,21 @@ 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;
> >
> >       serial8250_rpm_get(up);
> >
> > +     uart_port_lock_irqsave(port, &flags);
> >       lsr = serial_port_in(port, UART_LSR);
> > -
> > -     if (!(lsr & UART_LSR_DR)) {
> > -             status = NO_POLL_CHAR;
> > -             goto out;
> > -     }
> > -
> > -     status = serial_port_in(port, UART_RX);
> > +     if ((lsr & UART_LSR_DR))
>
> Too many parentheses.

Okay.

>
> > +             status = serial_port_in(port, UART_RX);
> > +     uart_port_unlock_irqrestore(port, flags);
>
> thanks,
> --
> js
> suse labs

Thanks,
Yunhui