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

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

[Fan Wu]

brcmuart_handle_irq() starts priv->hrt to work around a receive-timeout
quirk of the 8250 core when hardware flow control is enabled; the timer
callback brcmuart_hrtimer_func() dereferences priv.

brcmuart_remove() cancels the hrtimer before it unregisters the port, so
a receive-timeout interrupt that arrives in between re-arms the timer.
After devm frees priv, the callback runs on freed memory.

Unregister the port before cancelling the hrtimer.
serial8250_unregister_port() shuts the port down and frees its IRQ, so
brcmuart_handle_irq() can no longer restart the timer, and the final
hrtimer_cancel() drains any timer that was armed before or during the
unregister.

This issue was found by an in-house static analysis tool.

Fixes: 41a469482de2 ("serial: 8250: Add new 8250-core based Broadcom STB driver")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/8250/8250_bcm7271.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
index 742004d63c6f..60c7d5c9100c 100644
--- a/drivers/tty/serial/8250/8250_bcm7271.c
+++ b/drivers/tty/serial/8250/8250_bcm7271.c
@@ -1123,8 +1123,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);
 	brcmuart_free_bufs(&pdev->dev, priv);
 	if (priv->dma_enabled)
 		brcmuart_arbitration(priv, 0);

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

[sashiko-bot]

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

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

[Florian Fainelli]

On 9/9/26 01:40, Fan Wu wrote:
> brcmuart_handle_irq() starts priv->hrt to work around a receive-timeout
> quirk of the 8250 core when hardware flow control is enabled; the timer
> callback brcmuart_hrtimer_func() dereferences priv.
> 
> brcmuart_remove() cancels the hrtimer before it unregisters the port, so
> a receive-timeout interrupt that arrives in between re-arms the timer.
> After devm frees priv, the callback runs on freed memory.
> 
> Unregister the port before cancelling the hrtimer.
> serial8250_unregister_port() shuts the port down and frees its IRQ, so
> brcmuart_handle_irq() can no longer restart the timer, and the final
> hrtimer_cancel() drains any timer that was armed before or during the
> unregister.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 41a469482de2 ("serial: 8250: Add new 8250-core based Broadcom STB driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Co-developed-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian