Re: [PATCH] serial: fix ioctl hangup race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Using a `break` statement inside `scoped_guard()` incorrectly exits the macro's internal loop instead of the intended outer `for (;;)` loop, leading to an infinite wait or severe latency.

Pre-existing issues:
- [High] The TIOCMIWAIT ioctl is still vulnerable to the hangup race because `uart_wait_modem_status()` accesses hardware registers via `uart_enable_ms()`, despite the commit message claiming otherwise.
--

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

serial: fix ioctl hangup race

This patch attempts to prevent tty ioctls from racing with hangup by
checking if the port has been hung up before accessing the hardware. It
adds missing checks for tty_io_error() across several ioctl paths.

Link: https://lore.kernel.org/r/20260903163146.1498497-1-johan@kernel.org

> serial: fix ioctl hangup race
>
> The tty ioctls can race with hangup and end up calling into a tty
> driver for a device that is already gone or powered down.
>
> Add the missing checks to make sure the port has not been hung up before
> accessing the hardware to avoid issues like kernel panic due to
> unclocked accesses.
>
> Note that TIOCGSERIAL, TIOCGICOUNT and TIOCMIWAIT do not access hardware
> and are therefore not affected by the race.

[Severity: High]
This is a pre-existing issue, but does TIOCMIWAIT actually avoid hardware
access?

If a userspace process calls the TIOCMIWAIT ioctl, it routes through
uart_ioctl() to uart_wait_modem_status(). In uart_wait_modem_status(), we
directly call uart_enable_ms(), which invokes the enable_ms() hardware
callback to write to registers.

If a concurrent hangup powers down the port before uart_enable_ms()
executes, could this lead to an unclocked hardware access on a
powered-down port?

[ ... ]

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 95774b0f1484b..aac12be4ceac6 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c

[ ... ]

> @@ -1798,7 +1798,11 @@ static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
>  	 * 'timeout' / 'expire' give us the maximum amount of time
>  	 * we wait.
>  	 */
> -	while (!port->ops->tx_empty(port)) {
> +	for (;;) {
> +		scoped_guard(mutex, &state->port.mutex) {
> +			if (tty_io_error(tty) || port->ops->tx_empty(port))
> +				break;

[Severity: High]
Does this break statement exit the intended outer loop?

Since scoped_guard() is implemented as a hidden for loop in
include/linux/cleanup.h, the break statement will only exit the
scoped_guard macro's internal loop. The outer for (;;) loop in
uart_wait_until_sent() will continue executing and unconditionally sleep
via msleep_interruptible().

If timeout is 0, won't this result in an infinite wait loop unless a
signal is pending?

> +		}
>  		msleep_interruptible(jiffies_to_msecs(char_time));
>  		if (signal_pending(current))
>  			break;

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