Re: [PATCH] tty: timbuart: kill tasklet after freeing IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] tty: timbuart: tasklet rescheduled via timbuart_start_tx() after shutdown causes UAF
--

commit 2fa5108233918f24d98748eb13c80825cd7a576d
Author: LI xiao <lixiao0@hdu.edu.cn>

tty: timbuart: kill tasklet after freeing IRQ

This commit moves tasklet_kill() from timbuart_remove() to
timbuart_shutdown() immediately after free_irq() so that any tasklet
scheduled by the interrupt handler is drained before port resources are
released.

> diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
> index 7a0326e7102a2..c561ca7021901 100644
> --- a/drivers/tty/serial/timbuart.c
> +++ b/drivers/tty/serial/timbuart.c
> @@ -254,6 +254,7 @@ static void timbuart_shutdown(struct uart_port *port)
>  		container_of(port, struct timbuart_port, port);
>  	dev_dbg(port->dev, "%s\n", __func__);
>  	free_irq(port->irq, uart);
> +	tasklet_kill(&uart->tasklet);
>  	iowrite32(0, port->membase + TIMBUART_IER);
>  
>  	timbuart_flush_buffer(port);
> @@ -474,7 +475,6 @@ static void timbuart_remove(struct platform_device *dev)
>  {
>  	struct timbuart_port *uart = platform_get_drvdata(dev);
>  
> -	tasklet_kill(&uart->tasklet);
>  	uart_remove_one_port(&timbuart_driver, &uart->port);
>  	uart_unregister_driver(&timbuart_driver);
>  	kfree(uart);

[Severity: Critical]
Can uart->tasklet still be scheduled via timbuart_start_tx() after
timbuart_shutdown() or when tty_port_initialized() is false, leading to a
use-after-free of struct timbuart_port and a NULL or unmapped dereference of
uart->port.membase in timbuart_tasklet()?

In addition to the IRQ handler, timbuart_start_tx() unconditionally schedules
the tasklet:

drivers/tty/serial/timbuart.c:timbuart_start_tx() {
	struct timbuart_port *uart =
		container_of(port, struct timbuart_port, port);

	/* do not transfer anything here -> fire off the tasklet */
	tasklet_schedule(&uart->tasklet);
}

In drivers/tty/serial/serial_core.c, neither __uart_start() nor
uart_send_xchar() checks tty_port_initialized(port) or port_dev->tx_enabled
before calling port->ops->start_tx(port):

drivers/tty/serial/serial_core.c:__uart_start() {
	if (!pm_runtime_enabled(port->dev) || pm_runtime_active(&port_dev->dev))
		port->ops->start_tx(port);
}

drivers/tty/serial/serial_core.c:uart_send_xchar() {
	...
	else {
		guard(uart_port_lock_irqsave)(port);
		port->x_char = ch;
		if (ch)
			port->ops->start_tx(port);
	}
}

During a normal close in tty_port_close(), tty_port_shutdown() clears
tty_port_initialized(port) and calls:

  uart_tty_port_shutdown()
    -> uart_port_shutdown()
      -> timbuart_shutdown()

When tasklet_kill(&uart->tasklet) in timbuart_shutdown() drains an in-flight
timbuart_tasklet(), timbuart_handle_rx_port() -> timbuart_rx_chars() calls
tty_flip_buffer_push(tport), which queues tport->buf.work (flush_to_ldisc).
Because port->itty and tty->ldisc remain active until tty_release_struct()
after tty_port_close() returns, flush_to_ldisc() can run on another CPU after
timbuart_shutdown() returns and before tty_ldisc_flush() in
tty_port_close_end() acquires buf->lock. Inside n_tty_receive_buf_common()
(even while tty->closing == 1), receiving STOP_CHAR followed by START_CHAR
under default IXON termios (n_tty_receive_buf_closing() ->
n_tty_receive_char_closing() -> n_tty_receive_char_flow_ctrl() -> start_tty()
-> uart_start() -> __uart_start()), reaching the throttle threshold under
IXOFF (n_tty_check_throttle() -> tty_throttle_safe() -> uart_throttle() ->
uart_send_xchar()), or having EXTPROC set (__receive_buf() ->
uart_flush_chars() -> uart_start() -> __uart_start()) calls
timbuart_start_tx() and schedules uart->tasklet after timbuart_shutdown() has
completed.

Similarly, if timbuart_startup() fails in request_irq() for a CAP_SYS_ADMIN
opener, uart_port_startup() returns 1 with state->port.xmit_buf allocated,
tty_port_open() returns 1 with tty_port_initialized(port) still false, and
uart_open() returns 0. A subsequent open() in tty_port_open() executes
clear_bit(TTY_IO_ERROR, &tty->flags) before port->ops->activate(), allowing a
concurrent write() or ioctl(TCXONC) on the open descriptor (or
ioctl(TIOCSETD) -> tty_set_ldisc() -> n_tty_open() -> tty_unthrottle() ->
uart_unthrottle() -> uart_send_xchar()) to invoke timbuart_start_tx() and
schedule uart->tasklet while tty_port_initialized(port) remains false.

When timbuart_tasklet() runs after timbuart_shutdown(), it unconditionally
writes RXBAF | RXBF | RXTT | CTS_DELTA to TIMBUART_IER:

drivers/tty/serial/timbuart.c:timbuart_tasklet() {
	...
	iowrite32(ier, uart->port.membase + TIMBUART_IER);
}

which re-enables hardware interrupts after timbuart_shutdown() cleared
TIMBUART_IER. Furthermore, because tty_port_initialized(port) is false in
these cases, serial_core_remove_one_port() skips uart_shutdown() during
timbuart_remove():

drivers/tty/serial/serial_core.c:serial_core_remove_one_port() {
		if (tty_port_initialized(port))
			uart_shutdown(NULL, state);
	}

	/*
	 * Free the port IO and memory resources, if any.
	 */
	if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
		uport->ops->release_port(uport);
}

With tasklet_kill(&uart->tasklet) removed from timbuart_remove(),
timbuart_release_port() unmaps and zeroes port->membase and timbuart_remove()
frees uart via kfree(uart) while uart->tasklet is still scheduled or running.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/AMoAIAAtLCMmxKHlfyhDJarK.3.1792416715880.Hmail.252270051@hdu.edu.cn?part=1