[PATCH] serial: core: fix NULL pointer dereference in serial_core_unregister_port()

serial_core_unregister_port() dereferences port->port_dev before it has
been checked:

	struct serial_port_device *port_dev = port->port_dev;
	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);

serial_core_get_ctrl_dev() takes &port_dev->dev and reads dev->parent
straight away, so a NULL port_dev faults at offset 0x40.

port_dev is NULL whenever no port device is installed:
serial_core_remove_one_port() clears it on teardown, and it is never
set if registration failed before serial_core_port_device_add().

serial8250_unregister_port() reaches that state. It calls
uart_remove_one_port(), which clears port_dev, and then re-adds the
port with uart_add_one_port() without checking the return value. When
that re-add fails, port_dev stays NULL while port.dev still points at
the ISA platform device, so unbinding that device once more calls
serial8250_unregister_port() again and oopses:

  Oops: general protection fault, probably for non-canonical address
  KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
  RIP: 0010:serial_core_unregister_port+0xef/0x990
  Call Trace:
   serial8250_unregister_port+0x1e4/0x8a0
   serial8250_remove+0x8c/0xb0
   platform_remove+0x5f/0x80
   device_release_driver_internal+0x46b/0x640
   unbind_store+0xf8/0x110
   sysfs_kf_write+0xf2/0x150
   vfs_write+0x6ac/0x1050

Return early when there is no port device to remove, and read
port->port_dev under port_mutex, since every other update of that
field is serialised by it.

Also clear port->port_dev on the serial_core_register_port() error
path. serial_base_port_device_remove() frees the port device but left
the pointer behind, so unregistering after a failed registration read
freed memory instead. That is the use-after-free variant of the same
crash, and matches the title syzbot first reported this under.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=9f57c1b2792029198fcf
Cc: [email protected]
Signed-off-by: Ruslan Valiyev <[email protected]>
---
Reproduced and verified on 8d3ae59288f1 (Linux 7.2) with syzbot's config,
under QEMU/KVM x86_64. Over six runs of the reproducer:

  stock:   6/6 oops at serial_core_unregister_port+0xef, with the same
           Code: bytes and RDI=0x40 as the syzbot report
  patched: 0/6 oops at serial_core_unregister_port

checkpatch.pl clean, W=1 build of serial_core.o produces no new warnings,
and the patch applies cleanly to current mainline.

Please note the reproducer does not run to completion on a patched kernel.
It goes on to hit two further problems. Both look pre-existing and neither
is addressed here; I am describing them so the remaining crashes are not
mistaken for this patch failing.

1) tty_cdev_add() drops the last reference to the cdev when cdev_add()
   fails, but leaves driver->cdevs[index] pointing at it, and
   tty_unregister_device() then calls cdev_del() on the freed object:

     WARNING: lib/refcount.c:28 at refcount_warn_saturate
     Call Trace:
      kobject_put+0x26f/0x6f0
      tty_unregister_device+0x118/0x1c0
      tty_port_unregister_device+0x60/0x70
      serial_core_unregister_port+0x333/0x9a0

   tty_unregister_device() also calls cdev_del(driver->cdevs[index])
   unconditionally, and that entry is NULL when registration failed
   before tty_cdev_add() ran:

     KASAN: null-ptr-deref in range [0x60-0x67]
     RIP: 0010:cdev_del+0x26/0xa0

   serial_core_add_one_port() reaches both: it treats a failed tty
   registration as non-fatal, flagging the port dead and returning
   success, so the port is still unregistered later.

2) Registration is not failure-atomic. serial_core_add_one_port() links
   state->uart_port before the kasprintf() and tty_groups allocations, so
   a failure there leaves the port half registered. The state is never
   released, and because serial_core_add_one_port() starts with

           if (state->uart_port)
                   return -EINVAL;

   that line can then never be registered again. Unwinding it properly
   means undoing uart_configure_port(), which claims resources and can
   register a console, so it did not look like something to bolt onto a
   crash fix.

While here I also noticed serial8250_unregister_port() ignores the return
value of the uart_add_one_port() call that re-adds the port to the ISA
device, which is what produces the NULL port_dev this patch guards
against.
 drivers/tty/serial/serial_core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a530ad372b434..5bf71d7bbd223 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3327,6 +3327,7 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 
 err_unregister_port_dev:
 	serial_base_port_device_remove(port->port_dev);
+	port->port_dev = NULL;
 
 err_unregister_ctrl_dev:
 	serial_base_ctrl_device_remove(new_ctrl_dev);
@@ -3341,12 +3342,24 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
 {
 	struct device *phys_dev = port->dev;
-	struct serial_port_device *port_dev = port->port_dev;
-	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+	struct serial_port_device *port_dev;
+	struct serial_ctrl_device *ctrl_dev;
 	int ctrl_id = port->ctrl_id;
 
 	guard(mutex)(&port_mutex);
 
+	/*
+	 * A NULL port device means there is no registered port device to
+	 * remove: serial_core_remove_one_port() clears port_dev on
+	 * teardown, and it is never set if registration failed before
+	 * serial_core_port_device_add().
+	 */
+	port_dev = port->port_dev;
+	if (!port_dev)
+		return;
+
+	ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+
 	port->flags |= UPF_DEAD;
 
 	serial_core_remove_one_port(drv, port);

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.43.0