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.

---
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(-)

index a530ad372b434..5bf71d7bbd223 100644
@@ -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

It's an invalid syzbot reproducer, if root tells the kernel to unbind from a device when it is being used, it gets to keep the pieces when things break :(

Let me go polish off my "taint the kernel if bind/unbind runs" patch to keep this from happening...

Did you forget an Assisted-by: tag?

thanks,

greg k-h

Understood, thanks for looking at it so quickly. I'll drop the patch.

Yes. The patch was AI-assisted and should have included

I worked from submitting-patches.rst for the tag order and did not read coding-assistants.rst. My mistake, and it will be on anything I send in future.

Thanks,
Ruslan

It's not an invalid change, just fix it up to be sane and we can take it.

Your LLM should have read that file and added it automatically. If not, you're "holding it wrong" when using those tools as there's lots of text in there that it should be reading.

thanks,

greg k-h

At least these two lines are noise in the backtrace in the commit message. Submitting Patches recommends to leave only significantly important lines.

Overall, try to re-read and simplify the text. This looks like an AI puke.

With Best Regards,
Andy Shevchenko