Re: [PATCH 03/24] serial: 8250: don't wait for the em485 timers locked

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: em485 UAF and leak in serial8250_unregister_port()
--

commit 6643854d8da29baca2d8df19bd5e802596031b66
Author: Josef Bacik <josef@toxicpanda.com>

serial: 8250: don't wait for the em485 timers locked

This commit avoids a deadlock when disabling RS485 emulation by using
hrtimer_try_to_cancel() in serial8250_em485_destroy() while holding the port
lock and retaining the struct uart_8250_em485 instance in em485_idle. A new
serial8250_em485_free() helper is added to synchronously cancel the timers
without the port lock and free the structure during port unregistration.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 42319843cb5b0..0951d3e42e1f3 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -947,10 +947,7 @@ void serial8250_unregister_port(struct uart_8250_port *uart)
>  {
>  	guard(mutex)(&serial_mutex);
>  
> -	if (uart->em485) {
> -		guard(uart_port_lock_irqsave)(&uart->port);
> -		serial8250_em485_destroy(uart);
> -	}
> +	serial8250_em485_free(uart);
>  
>  	uart_remove_one_port(&serial8250_reg, &uart->port);

[Severity: High]
Can calling serial8250_em485_free(uart) before uart_remove_one_port() cause a
use-after-free or leak struct uart_8250_em485?

In serial8250_unregister_port(), only serial_mutex is held when
serial8250_em485_free() runs. Inside serial8250_em485_free(), uart->port.lock
is acquired to move uart->em485 / uart->em485_idle into a local variable and
then released before hrtimer_cancel() and kfree(em485):

drivers/tty/serial/8250/8250_port.c:serial8250_em485_free() {
	scoped_guard(uart_port_lock_irqsave, &p->port) {
		serial8250_em485_destroy(p);
		em485 = p->em485_idle;
		p->em485_idle = NULL;
	}
	...
	hrtimer_cancel(&em485->start_tx_timer);
	hrtimer_cancel(&em485->stop_tx_timer);
	kfree(em485);
}

Because uart_remove_one_port() has not yet detached the port, hung up the
tty, or unregistered the console, two races appear possible:

1. A concurrent TIOCSRS485 ioctl (uart_ioctl() -> uart_set_rs485_config()) or
PM resume (uart_resume_port() -> uart_rs485_config()) serializes on
state->port.mutex and uart->port.lock rather than serial_mutex. Once
serial8250_em485_free() drops uart->port.lock,->rs485_config() can call
serial8250_em485_config() -> serial8250_em485_init(), see both p->em485 and
p->em485_idle as NULL, and allocate a fresh struct uart_8250_em485. Neither
uart_remove_one_port() nor the rest of serial8250_unregister_port() calls
serial8250_em485_free() again, leaking the new allocation, reinitializing
uart->port.lock in serial8250_init_port() when serial8250_isa_devs is set, and
potentially leaving start_tx_timer or stop_tx_timer armed after the port is
unregistered.

2. If the port is used as a console, unregister_console() is not called until
uart_remove_one_port() -> serial_core_remove_one_port(). A concurrent
serial8250_console_write() caches up->em485 at entry and releases the nbcon
unsafe state across mdelay() before reading em485->tx_stopped:

drivers/tty/serial/8250/8250_port.c:serial8250_console_write() {
	struct uart_8250_em485 *em485 = up->em485;
	...
	if (em485) {
		mdelay(port->rs485.delay_rts_after_send);

		/* Toggle unsafe after possibly long delay */
		nbcon_exit_unsafe(wctxt);
		while (!nbcon_enter_unsafe(wctxt))
			nbcon_reacquire_nobuf(wctxt);

		if (em485->tx_stopped)
			up->rs485_stop_tx(up, false);
	}
}

If serial8250_em485_free() runs before uart_remove_one_port() unregisters the
console, kfree(em485) can free the structure while serial8250_console_write()
still holds the cached pointer and subsequently dereferences
em485->tx_stopped.

Should serial8250_em485_free(uart) be called after uart_remove_one_port()
instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-tty-llm-v1-0-99b0202ae546@linuxfoundation.org?part=3