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

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

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


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