Re: [PATCH] serial core: fix use-after-free after driver module unload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] serial: core: use-after-free and early module_put() in
  uart_cleanup()
- [Low] serial: core: inaccurate Fixes trailer in commit message
--

commit 6a2bc2d8089d9c95546b231a0390b02e8dc6cd97
Author: Johan Hovold <johan@kernel.org>

> serial core: fix use-after-free after driver module unload
>
> Deregistering a uart driver frees the driver uart state including the
> tty ports themselves.
>
> Take a reference to the uart driver module at install() so that the
> uart state and resources like the tty driver name do not go away while a
> tty is still open.
>
> This specifically avoids use-after-free when accessing the tty port and
> crashes when accessing the tty driver name (e.g. in the tty printk
> macros) when a tty is closed after its driver module has been unloaded.
>
> It also makes sure that the uart state is still valid when waking up any
> waiters after releasing the final uart port reference in
> uart_port_deref().
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

[Severity: Low]
1da177e4c3f4 ("Linux-2.6.12-rc2") cannot be resolved in the git history,
and uart_register_driver() originally set normal->owner = drv->owner until
it was removed by 2f16669d322e ("TTY: remove re-assignments to tty_driver
members").  Should the Fixes tag reference that commit instead?

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index b06d2649be343..2160e72780b45 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1935,10 +1935,25 @@ static int uart_install(struct tty_driver *driver, struct tty_struct *tty)
>  {
>  	struct uart_driver *drv = driver->driver_state;
>  	struct uart_state *state = drv->state + tty->index;
> +	int ret;
> +
> +	if (!try_module_get(drv->owner))
> +		return -ENODEV;
>  
>  	tty->driver_data = state;
>  
> -	return tty_standard_install(driver, tty);
> +	ret = tty_standard_install(driver, tty);
> +	if (ret)
> +		module_put(drv->owner);
> +
> +	return ret;
> +}
> +
> +static void uart_cleanup(struct tty_struct *tty)
> +{
> +	struct uart_driver *drv = tty->driver->driver_state;
> +
> +	module_put(drv->owner);
>  }

[Severity: High]
Can uart_cleanup() dereference freed memory via tty->driver->driver_state
when a device is unbound while a TTY is open?

Drivers such as sccnxp and tegra-tcu embed struct uart_driver inside
devm-allocated device state and call uart_unregister_driver() from their
.remove() callback.  Holding a reference to drv->owner does not prevent a
sysfs unbind, which unregisters the driver and frees the struct containing
struct uart_driver while the TTY remains open:

CPU0                                  CPU1
sccnxp_remove()
  uart_unregister_driver(&s->uart)
devres_release_all()
  kfree(s)
                                      release_one_tty()
                                        uart_cleanup()
                                          drv = tty->driver->driver_state;
                                          module_put(drv->owner);  <-- use-after-free

In addition, release_one_tty() runs tty->ops->cleanup(tty) before
tty_driver_kref_put(driver):

drivers/tty/tty_io.c:release_one_tty() {
	struct tty_driver *driver = tty->driver;
	struct module *owner = driver->owner;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty_driver_kref_put(driver);
	module_put(owner);
}

Dropping drv->owner in uart_cleanup() allows a concurrent rmmod to finish
uart_unregister_driver() and unmap the module before release_one_tty()
calls tty_driver_kref_put(driver) -> destruct_tty_driver(), leaving
/proc/tty/driver/<driver_name> registered while drv and drv->state are
already freed.  Leaving normal->owner unset also leaves cdev->owner unset,
so an open() racing with module unload is not blocked in chrdev_open() and
can call tty_driver_kref_put() in tty_open_by_driver() after the module has
been unloaded.

Setting normal->owner = drv->owner (or passing drv->owner to
__tty_alloc_driver()) in uart_register_driver() lets the TTY core manage
the module reference via struct tty_driver without dereferencing
tty->driver->driver_state in cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112100541.348390-1-johan@kernel.org?part=1