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

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

[Changyul Lee]

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@gmail.com>
---

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.

 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

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

[sashiko-bot]

> 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
> [ ... ]
> 
> Remove the entry on both error paths.
> 
> Fixes: 9f25e07b882b ("serial: mps2-uart: move to dynamic port allocation")
> Signed-off-by: Changyul Lee <lcy8047@gmail.com>

Sashiko has reviewed this patch and found no issues.

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