serial: fix ioctl hangup race
Subsystems (2)
PATCH 1/1 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.
---
I've just sent a fix for USB serial here:
https://lore.kernel.org/r/[email protected]
and will take closer look at the other TTY drivers tomorrow.
Johan
drivers/tty/serial/serial_core.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/tty/serial/serial_core.c +9 -5
@@ -896,7 +896,7 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,upf_t old_flags, new_flags;int retval; -if (!uport) +if (!uport ||tty_io_error (tty))return -EIO ; new_port = new_info->port;
@@ -1119,7 +1119,7 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)guard (mutex)(&port->mutex); uport =uart_port_check (state); -if (!uport) +if (!uport ||tty_io_error (tty))return -EIO ;if (uport->type !=PORT_UNKNOWN && uport->ops->break_ctl)
@@ -1144,7 +1144,7 @@ static int uart_do_autoconfig(struct tty_struct *tty, struct uart_state *state)*/ scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->mutex) { uport =uart_port_check (state); -if (!uport) +if (!uport ||tty_io_error (tty))return -EIO ;if (tty_port_users (port) !=1 )
@@ -1646,7 +1646,7 @@ static void uart_set_termios(struct tty_struct *tty,guard (mutex)(&state->port.mutex); uport =uart_port_check (state); -if (!uport) +if (!uport ||tty_io_error (tty))return ;/*
@@ -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 ; + }msleep_interruptible (jiffies_to_msecs (char_time));if (signal_pending (current))break ;
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 11 lines
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?
[ ... ]
Quoted 4 lines
drivers/tty/serial/serial_core.c +0
-0
[ ... ]
drivers/tty/serial/serial_core.c static void uart_wait_until_sent(struct tty_struct *tty, int timeout) · line 1804
Quoted 9 lines
drivers/tty/serial/serial_core.c +4
-1@@ -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?
Quoted 4 lines
drivers/tty/serial/serial_core.c +1
-0+ }msleep_interruptible (jiffies_to_msecs (char_time));if (signal_pending (current))break ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 6 lines · On Thu, Sep 03, 2026 at 04:45:31PM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Using abreakstatement insidescoped_guard()incorrectly exits the macro's internal loop instead of the intended outerfor (;;)loop, leading to an infinite wait or severe latency.
Bah. Another reason to dislike guards.
Quoted 4 lines
Pre-existing issues:
- [High] The TIOCMIWAIT ioctl is still vulnerable to the hangup race becauseuart_wait_modem_status()accesses hardware registers viauart_enable_ms(), despite the commit message claiming otherwise.
I missed that call. Will revisit.
Johan