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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Mar 2023 08:09:14 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Ilpo Järvinen' 
        <ilpo.jarvinen@...ux.intel.com>,
        "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>
CC:     "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH 2/2] serial: 8250: Fix serial8250_tx_empty() race with DMA
 Tx

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

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)