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

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]/

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

[ ... ]

> @@ -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)
>  {
[ ... ]
>  	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?

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