Skip to contentSkip to conversation

serial: core: fix NULL pointer dereference in serial_core_unregister_port()

PATCH In review Ruslan Valiyev 3 participants 3 messages Aug 26 – Aug 28

Subsystems (2)
Stable branch TTY layer and serial drivers
lore
Share & tools
lore

Ruslan Valiyev

author
#0 lore raw

PATCH v2 1/1 serial: core: fix NULL pointer dereference in serial_core_unregister_port()

port->port_dev is NULL when no port device is installed: it is cleared on teardown, and never set if registration failed before serial_core_port_device_add(). serial_core_unregister_port() passes it straight to serial_core_get_ctrl_dev(), which dereferences it:

  KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
  RIP: serial_core_unregister_port
  Call Trace:
   serial8250_unregister_port
   serial8250_remove
   unbind_store

Return early when there is no port device, and read port->port_dev under port_mutex.

Also clear port->port_dev on the serial_core_register_port() error path, where the port device has already been removed.

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]
Assisted-by Claude:claude-opus-5
Assisted-by Codex:gpt-5.6-sol
Signed-off-by Ruslan Valiyev <[email protected]>

---
Reproduced on 8d3ae59288f1 with syzbot's config under QEMU/KVM x86_64:
6/6 runs oops on stock, 0/6 patched. checkpatch clean, no new W=1 warnings.

The reproducer still does not run to completion on a patched kernel. It goes on to hit two pre-existing problems in the tty layer that this patch does not touch: tty_cdev_add() leaves driver->cdevs[index] pointing at a freed cdev when cdev_add() fails, and tty_unregister_device() deletes that entry unconditionally when it is NULL. Mentioning it so the remaining crashes are not mistaken for this fix failing.

v1: https://lore.kernel.org/all/[email protected]/
v2: trimmed the commit message and backtrace per Andy Shevchenko's
review, added the Assisted-by tags.

 drivers/tty/serial/serial_core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +15 -2
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

Signature · 1 line
2.43.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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

Pre-existing issues:
- [Critical] Missing cleanup on error paths in serial_core_add_one_port() leaves dangling pointers resulting in a Use-After-Free.
- [Critical] tty_cdev_add() leaves a dangling pointer on error, leading to a UAF or NULL-ptr dereference in tty_unregister_device(). --

commit a575b72dd88626a6d4f9468762be3ccbc1fb5e60
Author: Ruslan Valiyev <[email protected]>

serial: core: fix NULL pointer dereference in serial_core_unregister_port()

This commit fixes a NULL pointer dereference in serial_core_unregister_port() that happens when port->port_dev is NULL. It checks for a NULL port_dev early and clears port->port_dev on the registration error path. The patch does not touch two other pre-existing problems in the tty layer hit by the reproducer.

Link https://syzkaller.appspot.com/bug?extid=9f57c1b2792029198fcf
Link https://lore.kernel.org/all/[email protected]/ open here

drivers/tty/serial/serial_core.c int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) · line 3330

Quoted 9 lines
drivers/tty/serial/serial_core.c +1 -0
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;

[Severity: Critical]
This is a pre-existing issue, but does serial_core_add_one_port() leave a dangling pointer on its error paths?

If memory allocation fails in serial_core_add_one_port() (e.g., when allocating uport->name or uport->tty_groups), it returns -ENOMEM without clearing state->uart_port, which was set to uport earlier:

drivers/tty/serial/serial_core.c:serial_core_add_one_port() {
    ...
    state->uart_port = uport;
    ...
    uport->name = kasprintf(...);
    if (!uport->name)
        return -ENOMEM;
    ...
}

Could this lead to a Use-After-Free if a subsequent read of /proc/tty/driver/serial invokes uart_proc_show() -> uart_line_info(), retrieving the freed uport via uart_port_check()?

[ ... ]

Quoted 3 lines
drivers/tty/serial/serial_core.c +0 -0
@@ -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)
 {

[ ... ]

drivers/tty/serial/serial_core.c int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) · line 3365

Quoted 3 lines
drivers/tty/serial/serial_core.c +0 -0
 	port->flags |= UPF_DEAD;
 
 	serial_core_remove_one_port(drv, port);

[Severity: Critical]
This is a pre-existing issue, but does tty_cdev_add() leave a dangling pointer that causes a crash here during teardown?

In tty_cdev_add(), if cdev_add() fails, the cdev is freed but the array entry is not cleared:

drivers/tty/tty_io.c:tty_cdev_add() {
    ...
    if (err)
        kobject_put(&driver->cdevs[index]->kobj);
    return err;
}

When serial_core_remove_one_port() is called here during teardown, it unconditionally calls tty_unregister_device(). For drivers without dynamic allocation, this calls cdev_del(driver->cdevs[index]), which directly dereferences it:

fs/char_dev.c:cdev_del() {
    cdev_unmap(p->dev, p->count);
    ...
}

Could this cause a Use-After-Free or NULL pointer dereference when tearing down a device that previously failed registration?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v2 1/1
#2 lore raw
In reply to Ruslan Valiyev
Quoted 5 lines · Ruslan Valiyev

Return early when there is no port device, and read port->port_dev under port_mutex.

Also clear port->port_dev on the serial_core_register_port() error path, where the port device has already been removed.

These look like valid fixes to me:

Reviewed-by Tony Lindgren <[email protected]>