linux-kernel - RE: [PATCH 2/2] serial: 8250: Fix serial8250_tx

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: <ce143371-3ad-6c8-a37c-f8bcbc9382a@linux.intel.com>
Date:   Fri, 17 Mar 2023 12:51:35 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     David Laight <David.Laight@...LAB.COM>
cc:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH 2/2] serial: 8250: Fix serial8250_tx_empty() race with
 DMA Tx

On Fri, 17 Mar 2023, David Laight wrote:

> From: Ilpo Järvinen
> > Sent: 16 March 2023 13:25
> > 
> > There's a potential race before THRE/TEMT deasserts when DMA Tx is
> > starting up (or the next batch of continuous Tx is being submitted).
> > This can lead to misdetecting Tx empty condition.
> > 
> > It is entirely normal for THRE/TEMT to be set for some time after the
> > DMA Tx had been setup in serial8250_tx_dma(). As Tx side is definitely
> > not empty at that point, it seems incorrect for serial8250_tx_empty()
> > claim Tx is empty.
> > 
> > Fix the race by also checking in serial8250_tx_empty() whether there's
> > DMA Tx active.
> > 
> > Note: This fix only addresses in-kernel race mainly to make using
> > TCSADRAIN/FLUSH robust. Userspace can still cause other races but they
> > seem userspace concurrency control problems.
> > 
> > Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > ---
> >  drivers/tty/serial/8250/8250.h      | 12 ++++++++++++
> >  drivers/tty/serial/8250/8250_port.c |  7 ++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> ...
> >  static inline int ns16550a_goto_highspeed(struct uart_8250_port *up)
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index fa43df05342b..4954c4f15fb2 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2006,17 +2006,22 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
> >  {
> >  	struct uart_8250_port *up = up_to_u8250p(port);
> >  	unsigned long flags;
> > +	bool dma_tx_running;
> >  	u16 lsr;
> > 
> >  	serial8250_rpm_get(up);
> > 
> >  	spin_lock_irqsave(&port->lock, flags);
> >  	lsr = serial_lsr_in(up);
> > +	dma_tx_running = serial8250_tx_dma_running(up);
> >  	spin_unlock_irqrestore(&port->lock, flags);
> > 
> >  	serial8250_rpm_put(up);
> > 
> > -	return uart_lsr_tx_empty(lsr) ? TIOCSER_TEMT : 0;
> > +	if (uart_lsr_tx_empty(lsr) && !dma_tx_running)
> > +		return TIOCSER_TEMT;
> > +
> > +	return 0;
> >  }
> 
> Since checking whether dma is active is fast but reading
> the lsr is slow it is probably better to generate the return
> value inside the lock and test for dma first.
> Something like:
> 	spin_lock()
> 	result = 0;
> 	if (!serial8250_tx_dma_running(up) &&
> 	    uart_lsr_tx_empty(serial_lsr_in(up)))
> 		result = TIOCSER_TEMT;
> 	}
> 	spin_unlock()
> 	...
> 	return result;
> 
> 	David

Thanks, I'll change it to something along those lines.

-- 
 i.