Re: [PATCH] serial: sifive: fix off-by-one in console port bounds check

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Initialization Order / Uninitialized Lock and NULL Pointer Dereference
- [High] Divide by Zero / Race Condition
--

commit 1fa22c29aea26698105dc98632798b018a6fe1cf
Author: Zongmin Zhou <[email protected]>

serial: sifive: fix off-by-one in console port bounds check

sifive_serial_console_ports[] is sized SIFIVE_SERIAL_MAX_PORTS (8), so
valid indices are 0..7. The bounds check in sifive_serial_probe() uses
"id > SIFIVE_SERIAL_MAX_PORTS", which lets id == 8 pass and causes an
out-of-bounds write in __ssp_add_console_port() through
sifive_serial_console_ports[ssp->port.line].

Fix the check to use >= instead, matching the check already used in
sifive_serial_console_setup().

Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART")
Signed-off-by: Zongmin Zhou <[email protected]>

> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
> index 110d676131922..5d76f04736260 100644
> --- a/drivers/tty/serial/sifive.c
> +++ b/drivers/tty/serial/sifive.c
> @@ -999,7 +999,7 @@ static int sifive_serial_probe(struct platform_device *pdev)
>  	}
>  
>  #ifdef CONFIG_SERIAL_SIFIVE_CONSOLE
> -	if (id > SIFIVE_SERIAL_MAX_PORTS) {
> +	if (id >= SIFIVE_SERIAL_MAX_PORTS) {
>  		dev_err(&pdev->dev, "too many UARTs (%d)\n", id);
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but further down in sifive_serial_probe(),
clk_notifier_register() is called before ssp->baud_rate is initialized:

drivers/tty/serial/sifive.c:sifive_serial_probe() {
    ...
	r = clk_notifier_register(ssp->clk, &ssp->clk_notifier);
    ...
	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;
    ...
}

If a clock rate change is concurrently triggered right after registration but
before ssp->baud_rate is initialized, sifive_serial_clk_notifier() executes
and divides by zero:

drivers/tty/serial/sifive.c:sifive_serial_clk_notifier() {
    ...
		udelay(DIV_ROUND_UP(12 * 1000 * 1000, ssp->baud_rate));
    ...
}

Can this lead to a divide-by-zero panic during driver load?


[Severity: High]
This isn't a bug introduced by this patch, but towards the end of
sifive_serial_probe(), request_irq() is called before uart_add_one_port():

drivers/tty/serial/sifive.c:sifive_serial_probe() {
    ...
	r = request_irq(ssp->port.irq, sifive_serial_irq, ssp->port.irqflags,
			dev_name(&pdev->dev), ssp);
    ...
	r = uart_add_one_port(&sifive_serial_uart_driver, &ssp->port);
    ...
}

If the hardware has a pending interrupt (e.g., left active by the bootloader)
and interrupts are enabled, request_irq() allows sifive_serial_irq() to fire
immediately before the serial core has initialized the port state.

The interrupt handler would try to acquire the uninitialized port spinlock:

drivers/tty/serial/sifive.c:sifive_serial_irq() {
    ...
	uart_port_lock(&ssp->port);
    ...
}

And subsequently call __ssp_receive_chars(), which dereferences the
uninitialized ssp->port.state:

drivers/tty/serial/sifive.c:__ssp_receive_chars() {
    ...
	tty_flip_buffer_push(&ssp->port.state->port);
    ...
}

Since ssp->port.state is allocated later by uart_add_one_port(), it will be
NULL. Could this result in a lockdep warning and a NULL pointer dereference
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1