linux-kernel - [PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart

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

Message-Id: <20200925082412.12960-1-matthias.schiffer@ew.tq-group.com>
Date:   Fri, 25 Sep 2020 10:24:12 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>
Cc:     Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>
Subject: [PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart_shutdown() to avoid IRQ storm

The IPG clock is disabled at the end of imx_uart_shutdown(); we really
don't want to run any IRQ handlers after this point.

At least on i.MX8MN, the UART will happily continue to generate interrupts
even with its clocks disabled, but in this state, all register writes are
ignored (which will cause the shadow registers to differ from the actual
register values, resulting in all kinds of weirdness).

In a transfer without DMA, this could lead to the following sequence of
events:

- The UART finishes its transmission while imx_uart_shutdown() is run,
  triggering the TXDC interrupt (we can trigger this fairly reliably by
  writing a single byte to the TTY and closing it right away)
- imx_uart_shutdown() finishes, disabling the UART clocks
- imx_uart_int() -> imx_uart_transmit_buffer() -> imx_uart_stop_tx()

imx_uart_stop_tx() should now clear UCR4_TCEN to disable the TXDC
interrupt, but this register write is ineffective. This results in an
interrupt storm.

To disable all interrupts in the same place, and to avoid setting UCR4
twice, clearing UCR4_OREN is moved below del_timer_sync() as well; this
should be harmless.

Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
---

While debugging this, I found one more instance of register writes with
disabled clock: The IPG clock is disabled before calling
uart_add_one_port() at the end of imx_uart_probe(). This results in the
following call stack:

    imx_uart_writel+0x168/0x188
    imx_uart_set_mctrl+0x3c/0xb8
    uart_add_one_port+0x394/0x4c8
    imx_uart_probe+0x530/0x810

Fortunately, in this case the register already matches the value that is
written, so no inconsistent state results. I assume we'll have to do
something about the way we handle the clocks in this driver to fix
this...

 drivers/tty/serial/imx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4e6ead1f650e..1731d9728865 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1552,10 +1552,6 @@ static void imx_uart_shutdown(struct uart_port *port)
 	ucr2 = imx_uart_readl(sport, UCR2);
 	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
 	imx_uart_writel(sport, ucr2, UCR2);
-
-	ucr4 = imx_uart_readl(sport, UCR4);
-	ucr4 &= ~UCR4_OREN;
-	imx_uart_writel(sport, ucr4, UCR4);
 	spin_unlock_irqrestore(&sport->port.lock, flags);

 	/*
@@ -1568,10 +1564,15 @@ static void imx_uart_shutdown(struct uart_port *port)
 	 */

 	spin_lock_irqsave(&sport->port.lock, flags);
+
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
-
 	imx_uart_writel(sport, ucr1, UCR1);
+
+	ucr4 = imx_uart_readl(sport, UCR4);
+	ucr4 &= ~(UCR4_OREN | UCR4_TCEN);
+	imx_uart_writel(sport, ucr4, UCR4);
+
 	spin_unlock_irqrestore(&sport->port.lock, flags);

 	clk_disable_unprepare(sport->clk_per);
-- 
2.17.1