[PATCH 1/2] serial: sifive: fix NULL pointer dereference in probe IRQ window

[Sebastian Alba Vives <sebasjosue84@gmail.com>]

From: Sebastian Alba <sebasjosue84@estudiantec.cr>

sifive_serial_probe() installs the interrupt handler with request_irq()
before uart_add_one_port() has run.  uart_add_one_port() is what assigns
ssp->port.state, and ssp is allocated with devm_kzalloc(), so
ssp->port.state is NULL for the whole window between the two calls.

The handler dereferences that pointer unconditionally:

  sifive_serial_irq()
    __ssp_receive_chars()
      uart_insert_char(&ssp->port, ...)        /* port->state->port */
      tty_flip_buffer_push(&ssp->port.state->port);

so an RX interrupt taken inside the window oopses the kernel.

The window is not purely theoretical.  probe() programs TXCTRL and
RXCTRL but never writes the interrupt enable register, so IE keeps
whatever value the previous stage left in it.  If RX watermark
interrupts are still enabled there, or an interrupt is already pending
at the PLIC, the handler runs as soon as request_irq() unmasks the
line, with no timing requirement at all.

On a HiFive Unmatched (FU740-C000) the vendor firmware leaves IE
masked, so a cold boot on that board does not hit this.  The realistic
trigger is a handover where the previous stage leaves IE enabled, such
as kexec into a new kernel while the UART is in use.

Move request_irq() after uart_add_one_port() so the handler can never
observe a half-initialised port, and explicitly mask the interrupt
enable register during probe instead of inheriting whatever value was
left there.  Unwinding is adjusted accordingly: uart_remove_one_port()
now balances the added port on the request_irq() failure path.

Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART")
Signed-off-by: Sebastian Alba <sebasjosue84@estudiantec.cr>
---
 drivers/tty/serial/sifive.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
index f615701fc..4a445d285 100644
--- a/drivers/tty/serial/sifive.c
+++ b/drivers/tty/serial/sifive.c
@@ -1046,27 +1046,30 @@ static int sifive_serial_probe(struct platform_device *pdev)
 		     SIFIVE_SERIAL_RXCTRL_RXEN_MASK,
 		     SIFIVE_SERIAL_RXCTRL_OFFS, ssp);
 
-	r = request_irq(ssp->port.irq, sifive_serial_irq, ssp->port.irqflags,
-			dev_name(&pdev->dev), ssp);
-	if (r) {
-		dev_err(&pdev->dev, "could not attach interrupt: %d\n", r);
-		goto probe_out2;
-	}
+	/* Mask all interrupts; the bootloader may have left some enabled */
+	__ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp);
 
 	__ssp_add_console_port(ssp);
 
 	r = uart_add_one_port(&sifive_serial_uart_driver, &ssp->port);
 	if (r != 0) {
 		dev_err(&pdev->dev, "could not add uart: %d\n", r);
+		goto probe_out2;
+	}
+
+	r = request_irq(ssp->port.irq, sifive_serial_irq, ssp->port.irqflags,
+			dev_name(&pdev->dev), ssp);
+	if (r) {
+		dev_err(&pdev->dev, "could not attach interrupt: %d\n", r);
 		goto probe_out3;
 	}
 
 	return 0;
 
 probe_out3:
-	__ssp_remove_console_port(ssp);
-	free_irq(ssp->port.irq, ssp);
+	uart_remove_one_port(&sifive_serial_uart_driver, &ssp->port);
 probe_out2:
+	__ssp_remove_console_port(ssp);
 	clk_notifier_unregister(ssp->clk, &ssp->clk_notifier);
 probe_out1:
 	return r;
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv