Re: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime

[sashiko-bot@kernel.org]

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

New issues:
- [High] Clobbering of an active port entry in `imx_uart_ports` during a duplicate-line probe leads to a NULL pointer dereference.

Pre-existing issues:
- [High] Missing explicit cancellation of hrtimers during device removal leads to Use-After-Free.
- [High] imx_uart_probe() requests interrupts before sport->port.state is initialized, without fully clearing all hardware interrupt enable bits.
--

--- Patch [4]: [PATCH v2 4/4] serial: imx: serialize imx_uart_ports[] lifetime ---
commit 7a4c9e19ce793ebaff93921ddc3c70f324874180
Author: Karl Mehltretter <kmehltretter@gmail.com>

serial: imx: serialize imx_uart_ports[] lifetime

imx_uart_probe() publishes the port in imx_uart_ports[] before
uart_add_one_port(), because console setup during that call uses the
table. Use a driver-wide mutex to serialize table publication, port
addition and rollback with port removal and table clearing.

Link: https://lore.kernel.org/all/20260719162850.043B41F000E9@smtp.kernel.org

> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 251a50c8aa387..def874f9cd000 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
[ ... ]
> @@ -2632,11 +2642,14 @@ static int imx_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	imx_uart_ports[sport->port.line] = sport;
> -
>  	platform_set_drvdata(pdev, sport);
>  
> +	mutex_lock(&imx_uart_ports_lock);
> +	imx_uart_ports[sport->port.line] = sport;
> +	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +	if (ret)
> +		imx_uart_ports[sport->port.line] = NULL;

[Severity: High]
If a duplicate UART alias is probed, such as from a misconfigured device
tree, does this unconditionally overwrite an already active port's entry
in imx_uart_ports?

When uart_add_one_port() subsequently fails for the duplicate, this error
path sets the entry to NULL. The original port remains active, but will
console callbacks that rely on imx_uart_ports[] encounter a NULL pointer
dereference and crash the system?

[Severity: High]
This is a pre-existing issue, but I noticed that just before this code in
imx_uart_probe(), devm_request_irq() is called before sport->port.state is
initialized by uart_add_one_port().

Since UCR4 interrupt flags (like UCR4_TCEN) are not fully cleared, could an
early spurious interrupt fire immediately if left enabled by the bootloader
or a previous crash kernel?

If the interrupt fires, the ISR would call:

imx_uart_transmit_buffer()
    struct tty_port *tport = &sport->port.state->port;

Would this dereference the uninitialized sport->port.state and cause a
kernel panic?

> +	mutex_unlock(&imx_uart_ports_lock);
>  
>  err_clk:
>  	clk_disable_unprepare(sport->clk_ipg);
>  
>  	return ret;
>  }
>  
>  static void imx_uart_remove(struct platform_device *pdev)
>  {
>  	struct imx_port *sport = platform_get_drvdata(pdev);
> +	unsigned int line = sport->port.line;
>  
> +	mutex_lock(&imx_uart_ports_lock);
>  	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> +	imx_uart_ports[line] = NULL;
> +	mutex_unlock(&imx_uart_ports_lock);

[Severity: High]
This is a pre-existing issue, but does imx_uart_remove() need to explicitly
cancel any pending hrtimers before returning?

During imx_uart_shutdown(), if the TX state is SEND (e.g., in RS485 mode)
and the hrtimer fires late, the shutdown loop forces the state to OFF upon
timeout but does not cancel the timer:

imx_uart_shutdown()
    if (sport->tx_state != OFF) {
        dev_warn(sport->port.dev, "unexpected tx_state\n");
        ...
        sport->tx_state = OFF;
    }

Since sport is devm-allocated, it is freed immediately after
imx_uart_remove() returns. Could the pending trigger_stop_tx hrtimer then
fire and cause a use-after-free on the sport structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719221014.44354-1-kmehltretter@gmail.com?part=4