| 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
| ||
|
Message-ID: <34f3c38c-b224-8a4d-3235-c1df04ac1d04@linux.intel.com>
Date: Mon, 12 May 2025 15:02:40 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: yunhui cui <cuiyunhui@...edance.com>
cc: arnd@...db.de, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
benjamin.larsson@...exis.eu,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
heikki.krogerus@...ux.intel.com, Jiri Slaby <jirislaby@...nel.org>,
jkeeping@...usicbrands.com, john.ogness@...utronix.de,
LKML <linux-kernel@...r.kernel.org>,
linux-serial <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 v5 2/4] serial: 8250: avoid potential
PSLVERR issue
On Mon, 12 May 2025, yunhui cui wrote:
> Hi Ilpo,
>
> On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen
> <ilpo.jarvinen@...ux.intel.com> wrote:
> >
> > On Mon, 12 May 2025, yunhui cui wrote:
> >
> > > Hi Ilpo,
> > >
> > > On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> > > <ilpo.jarvinen@...ux.intel.com> wrote:
> > > >
> > > > On Tue, 6 May 2025, Yunhui Cui wrote:
> > > >
> > > > > Failure to check the UART_LSR_DR before reading UART_RX, or the
> > > > > non-atomic nature of clearing the FIFO and reading UART_RX, poses
> > > > > potential risks that could lead to PSLVERR.
> > > >
> > > > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > > > it's worded out in the other patch but also explain it here.
> > > >
> > > > You're only explaining problem and missing what this patch does to solve
> > > > the problem.
> > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@...edance.com>
> > > > > ---
> > > > > drivers/tty/serial/8250/8250.h | 13 +++++++++
> > > > > drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > > > > 2 files changed, 35 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > > > index b861585ca02a..6f97ff3a197d 100644
> > > > > --- a/drivers/tty/serial/8250/8250.h
> > > > > +++ b/drivers/tty/serial/8250/8250.h
> > > > > @@ -162,6 +162,19 @@ 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 void serial8250_discard_data(struct uart_8250_port *up)
> > > > > +{
> > > > > + u16 lsr;
> > > > > +
> > > > > + lsr = serial_in(up, UART_LSR);
> > > > > + if (lsr & UART_LSR_DR)
> > > > > + serial_in(up, UART_RX);
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * For the 16C950
> > > > > */
> > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > > > index a913135d5217..1666b965f6a0 100644
> > > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > > @@ -1357,9 +1357,8 @@ static void autoconfig_irq(struct uart_8250_port *up)
> > > > > /* Synchronize UART_IER access against the console. */
> > > > > uart_port_lock_irq(port);
> > > > > serial_out(up, UART_IER, UART_IER_ALL_INTR);
> > > > > + serial8250_discard_data(up);
> > > > > uart_port_unlock_irq(port);
> > > > > - serial_in(up, UART_LSR);
> > > > > - serial_in(up, UART_RX);
> > > > > serial_in(up, UART_IIR);
> > > > > serial_in(up, UART_MSR);
> > > > > serial_out(up, UART_TX, 0xFF);
> > > > > @@ -2137,25 +2136,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);
> > > > >
> > > > > - if (!(lsr & UART_LSR_DR)) {
> > > > > - status = NO_POLL_CHAR;
> > > > > - goto out;
> > > > > - }
> > > > > -
> > > > > - status = serial_port_in(port, UART_RX);
> > > > > -out:
> > > > > serial8250_rpm_put(up);
> > > > > return status;
> > > >
> > > > Not a problem that originates from you, but IMO calling this variable
> > > > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > > > is no character is present).
> > > >
> > > > > }
> > > > >
> > > > > -
> > > > > static void serial8250_put_poll_char(struct uart_port *port,
> > > > > unsigned char c)
> > > > > {
> > > > > @@ -2264,13 +2260,17 @@ 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);
> > > > >
> > > > > /*
> > > > > - * Clear the interrupt registers.
> > > > > + * Read UART_RX to clear interrupts (e.g., Character Timeout).
> > > > > + * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > > > + * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> > > >
> > > > I don't understand what the last two lines mean and I don't see the
> > > > connection to the code that is below the comment either, could you try to
> > > > rephrase the comment.
> > >
> > > The original intention was to check UART_LSR_DR first when reading
> > > UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> > > to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.
> >
> > I understood the first sentence in the comment but the rest of it is very
> > cryptic and has many grammar issues too. Also, the extent of passive voice
> > there makes it hard to know who does what (UART / kernel).
> >
> > > The logic for clearing the interrupt in the interrupt handling
> > > function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> > > to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> > > needs to be disabled.
> >
> > The grammar is so broken, it failed to convey that message.
>
> The purpose of serial_port_in(port, UART_RX) is to clear interrupts
> such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX)
> is called when the LSR does not have the UART_LSR_DR bit set.
>
> To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX)
> should be called only when the LSR has the UART_LSR_DR bit set.
>
> These two logics are clearly contradictory. Therefore, both
> serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed
> under the protection of port->lock.
>
> If you believe this is not a potential issue, that's fine. I can
> remove this patch in the next patchset version.
No, my goal is not to get this removed from the patch series.
I meant that the comment wording needs to be fixed for the next version
such that it is understandable for those that are not deeply familiar with
what is related to PSLVERR. Currently even I struggle to follow what's
written into that comment (unless I read heavily between lines and base
guesses on the extra knowledge I've about how this entire patchset relates
to PSLVERR).
> > > Therefore, we should put serial8250_clear_fifos() and the execution of
> > > serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> > > port->lock.
> > >
> > > >
> > > > > */
> > > > > serial_port_in(port, UART_LSR);
> > > > > serial_port_in(port, UART_RX);
> > > > > + uart_port_unlock_irqrestore(port, flags);
> > > > > serial_port_in(port, UART_IIR);
> > > > > serial_port_in(port, UART_MSR);
> > > > >
> > > > > @@ -2429,15 +2429,14 @@ int serial8250_do_startup(struct uart_port *port)
> > > > > }
> > > > >
> > > > > dont_test_tx_en:
> > > > > - uart_port_unlock_irqrestore(port, flags);
> > > > >
> > > > > /*
> > > > > * Clear the interrupt registers again for luck, and clear the
> > > > > * saved flags to avoid getting false values from polling
> > > > > * routines or the previous session.
> > > > > */
> > > > > - serial_port_in(port, UART_LSR);
> > > > > - serial_port_in(port, UART_RX);
> > > > > + serial8250_discard_data(up);
> > > > > + uart_port_unlock_irqrestore(port, flags);
> > > > > serial_port_in(port, UART_IIR);
> > > > > serial_port_in(port, UART_MSR);
> > > > > up->lsr_saved_flags = 0;
> > > > > @@ -2519,7 +2518,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
> > > > > @@ -2527,6 +2525,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
> > > > > /*
> > > > > @@ -2535,11 +2541,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);
> > > > > serial8250_rpm_put(up);
> > > > >
> > > > > up->ops->release_irq(up);
> > > > >
> > > >
> > > > --
> > > > i.
> > > >
> > >
> > > Thanks,
> > > Yunhui
> > >
> >
> > --
> > i.
>
> Thanks,
> Yunhui
>
--
i.
Powered by blists - more mailing lists