Re: [PATCH] serial: mps2-uart: Remove the ports_idr entry when probe fails

[Vladimir Murzin]

On 11/6/26 01:06, Changyul Lee wrote:
> mps2_of_get_port() adds mps_port to ports_idr, but mps2_serial_probe()
> does not remove the entry when mps2_init_port() or uart_add_one_port()
> fails. mps_port is allocated with devm_kzalloc(), so devres frees it
> when probe fails, while ports_idr still points to it. When another port
> later registers the console, mps2_uart_console_setup() looks up the
> console index in ports_idr and uses the freed mps_port:
> 
>   BUG: KASAN: slab-use-after-free in uart_set_options+0x650/0x7a4
>   Read of size 8 at addr ffff0000018c41a0 by task swapper/0/1
>   Call trace:
>    uart_set_options
>    mps2_uart_console_setup
>    try_enable_preferred_console
>    register_console
>    serial_core_register_port
>    serial_ctrl_register_port
>    uart_add_one_port
>    mps2_serial_probe
>   ...
>   Freed by task 1:
>    kfree
>    dr_node_free
>    devres_release_all
>    device_unbind_cleanup
>    really_probe
> 
> The stale entry also keeps the ID in use. A re-probe of the port (e.g.
> after -EPROBE_DEFER) then gets a different ID than its alias.
> 
> Remove the entry on both error paths.
> 
> Fixes: 9f25e07b882b ("serial: mps2-uart: move to dynamic port allocation")
> Signed-off-by: Changyul Lee <lcy8047@xxxxxxxxx>
> ---
> 
> Notes:
>     Found with a static analysis tool we are developing.
>     
>     Reproduced in QEMU (arm64 virt, CONFIG_COMPILE_TEST, KASAN) with two
>     arm,mps2-uart nodes added to the device tree: serial0 without a
>     "clocks" property, so devm_clk_get() fails after the ID is allocated,
>     and serial1 with a fixed clock, booted with console=ttyMPS0. Without
>     this patch KASAN reports the use-after-free above. With it, there is no
>     KASAN report and the boot goes on until it looks for a root filesystem
>     (the test has none).
>     Not tested on MPS2 hardware.

Before getting into the review, I'd like to clarify a few things:

- This driver is only used on a NOMMU platform, where any code
  can write to any memory location, so the security impact of
  this issue is likely minimal

- The only platform that uses this driver is deprecated and
  scheduled for removal [1], with the driver to be removed
  afterwards

@Arnd, could you please advise on how to proceed? Should we
review the patch, leave things as they are, or take a different
approach?

[1] https://lore.kernel.org/linux-arm-kernel/20260908152808.3928630-12-arnd@xxxxxxxxxx/

> 
>  drivers/tty/serial/mps2-uart.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
> index 2a4c09f3a834..c39aa534b822 100644
> --- a/drivers/tty/serial/mps2-uart.c
> +++ b/drivers/tty/serial/mps2-uart.c
> @@ -590,15 +590,19 @@ static int mps2_serial_probe(struct platform_device *pdev)
>  
>  	ret = mps2_init_port(pdev, mps_port);
>  	if (ret)
> -		return ret;
> +		goto err_remove_id;
>  
>  	ret = uart_add_one_port(&mps2_uart_driver, &mps_port->port);
>  	if (ret)
> -		return ret;
> +		goto err_remove_id;
>  
>  	platform_set_drvdata(pdev, mps_port);
>  
>  	return 0;
> +
> +err_remove_id:
> +	idr_remove(&ports_idr, mps_port->port.line);
> +	return ret;
>  }
>  
>  #ifdef CONFIG_OF
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> -- 2.43.0
>