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

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

Pre-existing issues:
- [High] `tty_unregister_device()` unconditionally deregisters a `cdev`, causing UAF or NULL pointer dereference if `tty_cdev_add()` previously failed.
- [High] `uart_resume_port()` dereferences a NULL `tty_dev` in `device_may_wakeup()` if port registration failed and the driver kept the port active.
- [High] Concurrent read-modify-write of `port->flags` without proper locking causes `UPF_DEAD` to be lost, undermining port teardown safety.
--

commit a9b5deb58407eb5cb3253b4b3646215bfcb86ab0
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 occurs when unregistering a port with no port device installed. It adds
an early return if port_dev is NULL and explicitly clears port->port_dev
on the error path of serial_core_register_port().

> 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;

[Severity: High]
This is a pre-existing issue, but does uart_resume_port() dereference a
NULL tty_dev in device_may_wakeup() if port registration failed and the
driver kept the port active?

If serial_core_register_port() fails, port_dev is set to NULL. If this port
is configured as a console, during system suspend uart_suspend_port() will
return early (leaving uport->suspended = 0).

On system resume, uart_resume_port() calls device_find_child(NULL, ...):

drivers/tty/serial/serial_core.c:uart_resume_port() {
	tty_dev = device_find_child(&uport->port_dev->dev, &match, serial_match_port);
	if (!uport->suspended && device_may_wakeup(tty_dev)) {
		...
	}
}

It safely returns NULL for tty_dev. Since uport->suspended is 0, the
short-circuit && does not prevent device_may_wakeup(tty_dev) from being
called.

device_may_wakeup(NULL) unconditionally dereferences NULL->power.can_wakeup:

include/linux/pm_wakeup.h:device_may_wakeup() {
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

>  
>  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;

[Severity: High]
This is a pre-existing issue, but can a concurrent read-modify-write of
port->flags without proper locking cause UPF_DEAD to be lost, undermining
port teardown safety?

serial_core_unregister_port() sets the UPF_DEAD bit in port->flags while
holding the global port_mutex, but without holding the TTY &port->mutex.

Concurrently, uart_set_info() modifies port->flags using a read-modify-write
operation while holding the TTY &port->mutex:

drivers/tty/serial/serial_core.c:uart_set_info() {
	uport->flags = (uport->flags & ~UPF_CHANGE_MASK) |
				 (new_flags & UPF_CHANGE_MASK);
}

If the threads interleave, uart_set_info() can overwrite and clear the
UPF_DEAD bit set by the teardown path. If UPF_DEAD is lost,
uart_port_activate() will incorrectly allow an unbound/removed port to be
opened and activated.

>  
>  	serial_core_remove_one_port(drv, port);

[Severity: High]
This is a pre-existing issue, but does tty_unregister_device() unconditionally
deregister a cdev, causing UAF or NULL pointer dereference if tty_cdev_add()
previously failed?

If cdev_alloc() or cdev_add() fails in tty_cdev_add(), it leaves
driver->cdevs[index] pointing to NULL or freed memory:

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

Because serial_core_add_one_port() ignores this error, treating TTY
registration failure as non-fatal and returning success, the port gets
registered:

drivers/tty/serial/serial_core.c:serial_core_add_one_port() {
	if (!IS_ERR(tty_dev)) {
		device_set_wakeup_capable(tty_dev, 1);
	} else {
		uport->flags |= UPF_DEAD;
		dev_err(uport->dev, "Cannot register tty device on line %u\n",
		       uport->line);
	}
	return 0;
}

On teardown, serial_core_remove_one_port() calls
tty_port_unregister_device() which invokes tty_unregister_device().
tty_unregister_device() calls cdev_del(driver->cdevs[index]) unconditionally
since serial_core does not use the TTY_DRIVER_DYNAMIC_ALLOC flag:

drivers/tty/tty_io.c:tty_unregister_device() {
	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
		cdev_del(driver->cdevs[index]);
		driver->cdevs[index] = NULL;
	}
}

cdev_del() does not check for NULL and dereferences the pointer, causing a
direct kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1