Re: [PATCH 4/4] serial: imx: clear imx_uart_ports[] entry on probe failure and removal

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lockless updates to the global imx_uart_ports[] array leave a Time-Of-Check to Time-Of-Use (TOCTOU) race condition during concurrent sibling probes and removals, allowing a stale pointer to be used for console registration and causing a deferred NULL pointer dereference.

Pre-existing issues:
- [High] Missing hrtimer cancellation for RS485 tx timers during device removal and shutdown leads to a Use-After-Free of the sport structure.
--

--- Patch [4]: [PATCH 4/4] serial: imx: clear imx_uart_ports[] entry on probe failure and removal ---
commit 85b874e619e770afd678fef4f09d60cd44a0c477
Author: Karl Mehltretter <kmehltretter@gmail.com>

serial: imx: clear imx_uart_ports[] entry on probe failure and removal

This commit clears the imx_uart_ports[] entry on probe failure and driver
removal. It prevents a stale pointer from being used by a sibling probe's
console setup during a devres free.

> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 251a50c8aa387..617c35772056c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2637,6 +2637,8 @@ static int imx_uart_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, sport);
>  
>  	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +	if (ret)
> +		imx_uart_ports[sport->port.line] = NULL;
>  
>  err_clk:

[ ... ]

> @@ -2647,8 +2649,10 @@ static int imx_uart_probe(struct platform_device *pdev)
>  static void imx_uart_remove(struct platform_device *pdev)
>  {
>  	struct imx_port *sport = platform_get_drvdata(pdev);
> +	unsigned int line = sport->port.line;
>  
>  	uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> +	imx_uart_ports[line] = NULL;

[Severity: High]
Does clearing imx_uart_ports[] after uart_remove_one_port() create a race
condition with concurrent sibling probes?

When uart_remove_one_port() returns, all locks (including console_list_lock
and port->mutex) have been dropped.

If Thread A unbinds the UART, it unregisters the console inside
uart_remove_one_port(), drops the locks, but has not yet executed the newly
added line to clear imx_uart_ports[line].

Concurrently, Thread B probing a sibling UART can see the console is
unregistered, and successfully register it. During registration, Thread B's
imx_uart_console_setup() locklessly reads the still-valid 
imx_uart_ports[0].

When Thread A resumes, it clears imx_uart_ports[0] = NULL, and devres frees
the port. A subsequent printk calls imx_uart_console_write_atomic(), which
reads the NULL pointer from the array and dereferences it.

>  }

[Severity: High]
This is a pre-existing issue, but does device removal fail to cancel RS485
tx timers, leading to a use-after-free?

During device unbind, imx_uart_shutdown() loops waiting for 
tx_state != OFF.

If the trigger_start_tx or trigger_stop_tx hrtimer is delayed or executing
concurrently, the loop times out, forcefully sets tx_state to OFF, and
returns without explicitly cancelling the timer using hrtimer_cancel().

Because imx_uart_remove() finishes without cancelling these driver-specific
hrtimers, devres will free the sport structure memory. If the delayed timer
subsequently fires, it will dereference the freed sport memory.

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