Re: [PATCH] serial: 8250_bcm7271: fix use-after-free in brcmuart_remove()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition and lock corruption introduced by reordering hrtimer_cancel.

Pre-existing issues:
- [High] Missing hrtimer_cancel on the brcmuart_probe error path leads to a use-after-free.
- [High] brcmuart_free_bufs() frees DMA buffers before the devm-managed DMA IRQ is deactivated, leading to a use-after-free.
- [High] Data race on hardware registers and state flags between set_termios and DMA ISR due to missing locks.
- [High] Race condition between port registration and priv->up initialization causes NULL pointer dereference in the timer callback.
--

commit bdfef65a3f8fdbe1033b7dfafac9c8386120580f
Author: Fan Wu <fanwu01@zju.edu.cn>

serial: 8250_bcm7271: fix use-after-free in brcmuart_remove()
    
This commit moves hrtimer_cancel() after serial8250_unregister_port() in
brcmuart_remove() to prevent a use-after-free. The goal is to shut down the
port and free its IRQ so the hrtimer cannot be re-armed before cancelling it.

> diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
> --- a/drivers/tty/serial/8250/8250_bcm7271.c
> +++ b/drivers/tty/serial/8250/8250_bcm7271.c
> @@ -1121,8 +1121,8 @@ static void brcmuart_remove(struct platform_device *pdev)
>  	struct brcmuart_priv *priv = platform_get_drvdata(pdev);
>  
>  	debugfs_remove_recursive(priv->debugfs_dir);
> -	hrtimer_cancel(&priv->hrt);
>  	serial8250_unregister_port(priv->line);
> +	hrtimer_cancel(&priv->hrt);

[Severity: High]
Does moving hrtimer_cancel() after serial8250_unregister_port() introduce a
race condition?

A pending hrtimer can start executing on another CPU, read priv->shutdown
locklessly as false, and block waiting for the port lock. Concurrently,
serial8250_unregister_port() calls serial8250_init_port(), which reinitializes
the port's spinlock using spin_lock_init(). 

If the timer callback attempts to acquire the spinlock while it is being
reinitialized, could this lead to lock corruption or deadlocks?

>  	brcmuart_free_bufs(&pdev->dev, priv);

[Severity: High]
This is a pre-existing issue, but could freeing the DMA buffers here cause a
use-after-free if the devm-managed DMA IRQ handler is still executing?

The driver requests the DMA interrupt via devm_request_irq() with IRQF_SHARED.
Devres defers freeing the IRQ until after brcmuart_remove() returns. If a
shared interrupt fires concurrently, brcmuart_isr() could process pending
status and dereference priv->rx_bufs via brcmuart_rx_buf_done_isr() after
brcmuart_free_bufs() has freed it.

>  	if (priv->dma_enabled)
>  		brcmuart_arbitration(priv, 0);
>  }


While reviewing this patch, I noticed a few pre-existing issues in related
code.

[Severity: High]
This is a pre-existing issue, but on the error path in brcmuart_probe():

drivers/tty/serial/8250/8250_bcm7271.c:brcmuart_probe() {
    ...
err1:
    serial8250_unregister_port(priv->line);
err:
    return ret;
}

If an RX timeout interrupt triggers brcmuart_handle_irq() and arms priv->hrt
after the port is registered, would failing a subsequent probe step (like a DMA
IRQ request) leave the hrtimer armed? 

The error path unregisters the port but misses hrtimer_cancel(), which could
result in the timer callback executing on freed memory once devres frees priv.


[Severity: High]
This is a pre-existing issue, but could brcmstb_set_termios() race with the
DMA ISR?

drivers/tty/serial/8250/8250_bcm7271.c:brcmstb_set_termios() {
    ...
    if (priv->dma_enabled)
        stop_rx_dma(p8250);
    ...
    if (priv->dma_enabled)
        start_rx_dma(p8250);
    ...
}

These functions modify hardware registers and update priv->rx_running without
holding the UART port spinlock. Can a concurrent DMA interrupt execute
brcmuart_isr() on another CPU and modify these same states, corrupting the
DMA engine state?


[Severity: High]
This is a pre-existing issue, but is there a window where priv->up is used
before it is initialized?

drivers/tty/serial/8250/8250_bcm7271.c:brcmuart_probe() {
    ...
    ret = serial8250_register_8250_port(&up);
    ...
    priv->line = ret;
    new_port = serial8250_get_port(ret);
    priv->up = &new_port->port;
    ...
}

If a UART IRQ fires immediately after serial8250_register_8250_port() but
before priv->up is set, brcmuart_handle_irq() can arm the hrtimer. 

If brcmuart_hrtimer_func() runs, it dereferences priv->up which is still NULL:

drivers/tty/serial/8250/8250_bcm7271.c:brcmuart_hrtimer_func() {
    ...
    struct uart_port *p = priv->up;
    ...
    uart_port_lock_irqsave(p, &flags);
    ...
}

Could this cause a NULL pointer dereference crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909084002.686911-1-fanwu01@zju.edu.cn?part=1