linux-kernel - Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

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:   Thu, 22 Apr 2021 09:34:55 +0000
From:   Valentin CARON - foss <valentin.caron@...s.st.com>
To:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jiri Slaby <jirislaby@...nel.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre TORGUE - foss <alexandre.torgue@...s.st.com>,
        "dillon.minfei@...il.com" <dillon.minfei@...il.com>,
        Erwan LE-RAY - foss <erwan.leray@...s.st.com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        Gerald BAEZA <gerald.baeza@...com>
Subject: Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

Hi, Johan

On 4/16/21 4:05 PM, Johan Hovold wrote:
> When DMA is enabled the receive handler runs in a threaded handler, but
> the primary handler up until very recently neither disabled interrupts
> in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
> interrupt comes in while the threaded receive handler is running under
> the port lock.
>
> Commit ad7676812437 ("serial: stm32: fix a deadlock condition with
> wakeup event") claimed to fix an unrelated deadlock, but unfortunately
> also disabled interrupts in the threaded handler. While this prevents
> the deadlock mentioned in the previous paragraph it also defeats the
> purpose of using a threaded handler in the first place.
>
> Fix this by making the interrupt one-shot and not disabling interrupts
> in the threaded handler.
>
> Note that (receive) DMA must not be used for a console port as the
> threaded handler could be interrupted while holding the port lock,
> something which could lead to a deadlock in case an interrupt handler
> ends up calling printk.
>
> Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
> Fixes: 3489187204eb ("serial: stm32: adding dma support")
> Cc: stable@...r.kernel.org      # 4.9
> Cc: Alexandre TORGUE <alexandre.torgue@...com>
> Cc: Gerald Baeza <gerald.baeza@...com>
> Signed-off-by: Johan Hovold <johan@...nel.org>

Reviewed-by: Valentin Caron<valentin.caron@...s.st.com>

> ---
>   drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 4d277804c63e..3524ed2c0c73 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   	struct tty_port *tport = &port->state->port;
>   	struct stm32_port *stm32_port = to_stm32_port(port);
>   	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> -	unsigned long c, flags;
> +	unsigned long c;
>   	u32 sr;
>   	char flag;
>   
> -	if (threaded)
> -		spin_lock_irqsave(&port->lock, flags);
> -	else
> -		spin_lock(&port->lock);
> +	spin_lock(&port->lock);
>   
>   	while (stm32_usart_pending_rx(port, &sr, &stm32_port->last_res,
>   				      threaded)) {
> @@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   		uart_insert_char(port, sr, USART_SR_ORE, c, flag);
>   	}
>   
> -	if (threaded)
> -		spin_unlock_irqrestore(&port->lock, flags);
> -	else
> -		spin_unlock(&port->lock);
> +	spin_unlock(&port->lock);
>   
>   	tty_flip_buffer_push(tport);
>   }
> @@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port)
>   
>   	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
>   				   stm32_usart_threaded_interrupt,
> -				   IRQF_NO_SUSPEND, name, port);
> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> +				   name, port);
>   	if (ret)
>   		return ret;
>   
> @@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,
>   	struct dma_async_tx_descriptor *desc = NULL;
>   	int ret;
>   
> +	/*
> +	 * Using DMA and threaded handler for the console could lead to
> +	 * deadlocks.
> +	 */
> +	if (uart_console(port))
> +		return -ENODEV;
> +
>   	/* Request DMA RX channel */
>   	stm32port->rx_ch = dma_request_slave_channel(dev, "rx");
>   	if (!stm32port->rx_ch) {