[PATCH 0/2] serial: sifive: fix two init ordering bugs in probe

[PATCH 0/2] serial: sifive: fix two init ordering bugs in probe

[Sebastian Alba Vives]

From: Sebastian Alba <sebasjosue84@estudiantec.cr>

Hi all,

I found two ordering bugs in the SiFive UART probe path while working on my
undergrad thesis project. The project uses a HiFive Unmatched board with an
FU740-C000, and I spent a lot of time reading this driver to understand how
the console comes up, which is how I ran into both of these.

Both have the same shape. A resource gets turned on before the state it
depends on is set up, so there is a window during probe where a callback can
run against a half initialized port.

Patch 1 is the one I think matters. request_irq() runs before
uart_add_one_port(), so the handler is live while ssp->port.state is still
NULL from devm_kzalloc(). If a byte arrives in that window, the handler walks
into uart_insert_char() and dereferences the NULL state pointer. The patch
moves request_irq() after uart_add_one_port(), masks the interrupt enable
register before adding the port in case the bootloader left something on, and
adds the missing error label so a failing request_irq() unwinds
uart_add_one_port().

Patch 2 registers the clock notifier before ssp->baud_rate and
ssp->port.uartclk are initialized, so a rate change in that window divides by
zero. This is worse on RISC-V than on most other arches, because the ISA does
not trap on integer division by zero. divu by zero returns all bits set, so
DIV_ROUND_UP(12000000, 0) comes back as 0xFFFFFFFFFFFFFFFF and the udelay()
in the notifier becomes a busy wait that never ends. Instead of a clean oops
you get a hung core. The patch sets up the divider before registering the
notifier and adds a cheap guard inside the notifier.

I reproduced patch 1 under QEMU. The machine is qemu-system-riscv64 -M
sifive_u, which reports itself as HiFive Unleashed A00 and models the FU540,
not the FU740 I have on my desk, but it is the same UART IP block and the
same driver. I added six lines of instrumentation right after the
request_irq() block to print port.state, unmask RXWM and hold the window open
with mdelay(), then fed bytes into the UART while it was open. With panic=-1
the kernel reboots and retries on its own.

Without the fix I got 318 windows and 318 NULL dereferences, so every single
attempt crashed, and port.state printed as 0000000000000000 every time. The
trace is always the same.

  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000080
  epc : uart_insert_char+0x66/0x154
   ra : sifive_serial_irq+0x94/0x1f0

With the fix, same instrumentation at the same point and same input, I got 18
windows and 0 NULL dereferences, and port.state printed as (____ptrval____)
every time. That is what vsprintf prints for a real pointer before the RNG is
ready, since ptr_to_id() special cases NULL and prints the literal value
instead.

One honest note on those numbers. The window counts are not equal, because
without the fix the kernel crashes early and panic=-1 restarts it right away,
so many short cycles fit into the same 180 seconds, while with the fix nothing
crashes and every window burns its full mdelay. The count is not the argument.
With a 100 percent baseline failure rate, 0 out of 18 is not luck, and the non
NULL pointer is the cause while the crash is only the symptom.

I did not reproduce patch 2 the same way. It needs a clock rate change inside
a very narrow window and I could not drive one reliably under QEMU, so that
one comes from reading the code. Happy to be corrected on it.

Both were pointed at during review of the off by one fix back in August, but
as far as I can tell nobody sent patches, and both are still in master today.

This is my first series to this subsystem, so please tell me if I got the
format or the split wrong.

Thanks,
Sebastian


Sebastian Alba (2):
  serial: sifive: fix NULL pointer dereference in probe IRQ window
  serial: sifive: avoid division by zero in the clock notifier

 drivers/tty/serial/sifive.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

-- 
2.43.0

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

[Sebastian Alba Vives]

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

[PATCH 2/2] serial: sifive: avoid division by zero in the clock notifier

[Sebastian Alba Vives]

From: Sebastian Alba <sebasjosue84@estudiantec.cr>

sifive_serial_probe() registers the clock rate-change notifier before
it initialises the two fields that notifier divides by.  ssp comes from
devm_kzalloc(), so between clk_notifier_register() and the assignments
a few lines below, both ssp->baud_rate and ssp->port.uartclk are zero.

A rate change taken in that window reaches:

  PRE_RATE_CHANGE:  udelay(DIV_ROUND_UP(12 * 1000 * 1000, ssp->baud_rate))
  POST_RATE_CHANGE: __ssp_update_div() ->
                      DIV_ROUND_UP(ssp->port.uartclk, ssp->baud_rate) - 1

On RISC-V an integer division by zero does not trap; the ISA defines the
result as all bits set.  DIV_ROUND_UP() therefore yields ULONG_MAX and
udelay() busy-waits essentially forever, hanging the CPU, rather than
producing a clean oops.  The POST_RATE_CHANGE path additionally programs
a bogus divisor into the hardware.

On the FU740 the UART clock is derived from the CPU clock through a
synchronous divide-by-two, as the notifier's own comment describes, so
any CPU rate change during probe can hit this.

Initialise uartclk and baud_rate before registering the notifier, and
bail out early if the notifier is ever called with no baud rate set.

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

diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
index 4a445d285..cd2925b6a 100644
--- a/drivers/tty/serial/sifive.c
+++ b/drivers/tty/serial/sifive.c
@@ -604,6 +604,9 @@ static int sifive_serial_clk_notifier(struct notifier_block *nb,
 	struct clk_notifier_data *cnd = data;
 	struct sifive_serial_port *ssp = notifier_to_sifive_serial_port(nb);
 
+	if (!ssp->baud_rate)
+		return NOTIFY_OK;
+
 	if (event == PRE_RATE_CHANGE) {
 		/*
 		 * The TX watermark is always set to 1 by this driver, which
@@ -1022,6 +1025,11 @@ static int sifive_serial_probe(struct platform_device *pdev)
 	ssp->clk = clk;
 	ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier;
 
+	/* Set up clock divider before the notifier can observe this port */
+	ssp->port.uartclk = clk_get_rate(ssp->clk);
+	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;
+	__ssp_update_div(ssp);
+
 	r = clk_notifier_register(ssp->clk, &ssp->clk_notifier);
 	if (r) {
 		dev_err(&pdev->dev, "could not register clock notifier: %d\n",
@@ -1029,11 +1037,6 @@ static int sifive_serial_probe(struct platform_device *pdev)
 		goto probe_out1;
 	}
 
-	/* Set up clock divider */
-	ssp->port.uartclk = clk_get_rate(ssp->clk);
-	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;
-	__ssp_update_div(ssp);
-
 	platform_set_drvdata(pdev, ssp);
 
 	/* Enable transmits and set the watermark level to 1 */
-- 
2.43.0