* Re: [PATCH] tty: timbuart: kill tasklet after freeing IRQ
2026-10-19 13:31 ` [PATCH] tty: timbuart: kill tasklet after freeing IRQ LI xiao
@ 2026-10-19 13:45 ` Greg KH
2026-10-19 14:16 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: Greg KH @ 2026-10-19 13:45 UTC (permalink / raw)
To: LI xiao; +Cc: jirislaby, linux-kernel, linux-serial
On Mon, Oct 19, 2026 at 09:31:55PM +0800, LI xiao wrote:
> The timbuart driver calls tasklet_kill() before uart_remove_one_port().
> An interrupt can schedule the tasklet again after tasklet_kill()
> returns but before timbuart_shutdown() frees the IRQ. The subsequent
> free_irq() synchronizes the IRQ handler, but does not drain a tasklet
> that the handler has already queued.
>
> Move tasklet_kill() to timbuart_shutdown(), immediately after
> free_irq(), so that any tasklet queued by the IRQ handler is drained
> before uart_remove_one_port() releases the port resources.
>
> I tested the intended synchronization ordering with a KASAN-enabled
> kernel in a QEMU guest using a test module. The module queued
> the tasklet, waited for the simulated IRQ handler to exit, called
> tasklet_kill(), and then freed the UART object. The tasklet completed
> without a KASAN report or scheduling warning.
>
> This was a controlled test of the synchronization ordering. It did
> not exercise timbuart's actual IRQ handler or Timberdale UART hardware,
> which is not modeled by QEMU.
Ah, that might be tricky, I think you need to test this on real hardware
as you are changing the logic here.
> Signed-off-by: LI Xiao <lixiao0@hdu.edu.cn>
No fixes: tag? Cc: stable?
>
> ---
> drivers/tty/serial/timbuart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
> index 7a0326e71..c561ca702 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);
Are you sure you can flush after the tasklet is killed? Nothing
requires it to still be running at this point in time?
And why not stop it before the irq is freed as that's what it is
handling? Note, I could be totally wrong, I'm just guessing here...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tty: timbuart: kill tasklet after freeing IRQ
2026-10-19 13:31 ` [PATCH] tty: timbuart: kill tasklet after freeing IRQ LI xiao
2026-10-19 13:45 ` Greg KH
@ 2026-10-19 14:16 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-19 14:16 UTC (permalink / raw)
To: LI xiao; +Cc: linux-serial
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
^ permalink raw reply [flat|nested] 4+ messages in thread