serial: fix ioctl hangup race

PATCH Superseded Johan Hovold 2 participants 2 messages Sep 4

Subsystems (2)

Stable branch TTY layer and serial drivers

lore

Share & tools

lore

Johan Hovold

author

2026-09-04 11:45 UTC #0 lore raw

PATCH v2 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 do not access hardware and are therefore not affected by the race.

Fixes 04f378b198da ("tty: BKL pushdown")

Cc [email protected] # 2.6.26

Signed-off-by Johan Hovold <[email protected]>

---

I botched the scoped_guard() conversion as Sashiko pointed out so here's a v2.

Johan

Changes in v2
- include TIOCMIWAIT which also access hardware
- replace broken scoped_guard() construct in wait loop

 drivers/tty/serial/serial_core.c | 36 +++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 10 deletions(-)

drivers/tty/serial/serial_core.c +26 ~~-10~~

index 95774b0f1484..b0d3902fe4fc 100644
@@ -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)

@@ -1199,7 +1199,7 @@ static void uart_enable_ms(struct uart_port *uport)
  * FIXME: This wants extracting into a common all driver implementation
  * of TIOCMWAIT using tty_port.
  */
-static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
+static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *state, unsigned long arg)
 {
 	struct uart_port *uport;
 	struct tty_port *port = &state->port;

@@ -1213,9 +1213,17 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	uport = uart_port_ref(state);
 	if (!uport)
 		return -EIO;
-	scoped_guard(uart_port_lock_irq, uport) {
-		memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
-		uart_enable_ms(uport);
+
+	scoped_guard(mutex, &port->mutex) {
+		if (tty_io_error(tty)) {
+			ret = -EIO;
+			goto out_deref;
+		}
+
+		scoped_guard(uart_port_lock_irq, uport) {
+			memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
+			uart_enable_ms(uport);
+		}
 	}
 
 	add_wait_queue(&port->delta_msr_wait, &wait);

@@ -1245,6 +1253,7 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	}
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&port->delta_msr_wait, &wait);
+out_deref:
 	uart_port_deref(uport);
 
 	return ret;

@@ -1567,7 +1576,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 
 	/* This should only be used when the hardware is present. */
 	if (cmd == TIOCMIWAIT)
-		return uart_wait_modem_status(state, arg);
+		return uart_wait_modem_status(tty, state, arg);
 
 	/* rs485_config requires more locking than others */
 	if (cmd == TIOCSRS485)

@@ -1646,7 +1655,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 +1807,14 @@ 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 (;;) {
+		mutex_lock(&state->port.mutex);
+		if (tty_io_error(tty) || port->ops->tx_empty(port)) {
+			mutex_unlock(&state->port.mutex);
+			break;
+		}
+		mutex_unlock(&state->port.mutex);
+
 		msleep_interruptible(jiffies_to_msecs(char_time));
 		if (signal_pending(current))
 			break;

Signature · 1 line

2.55.0

[email protected]

AI review

2026-09-04 11:55 UTC #1 lore raw

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

New issues:
- [Medium] Mixing goto-based error handling with scope-based cleanup (scoped_guard) in the same function.

Pre-existing issues:
- [High] Missing hangup check (tty_io_error) inside the TIOCMIWAIT wait loop causes indefinite hangs on device removal.
- [High] Task state is set to TASK_INTERRUPTIBLE after evaluating the wait condition, causing potential missed wakeups. --

commit 971660f9118e5ac27dcc58fb0e95856b75c73a98
Author: Johan Hovold <[email protected]>
serial: fix ioctl hangup race

This patch adds missing checks to make sure the tty port has not been hung up before accessing the hardware in several serial ioctls. This helps avoid issues like kernel panics due to unclocked accesses on devices that are already gone or powered down.

Quoted 4 lines

drivers/tty/serial/serial_core.c +0 -0
index 95774b0f1484b..b0d3902fe4fca 100644

[ ... ]

drivers/tty/serial/serial_core.c static int uart_wait_modem_status(struct uart_state *state, unsigned long arg) · line 1221

Quoted 13 lines

drivers/tty/serial/serial_core.c +6 -3

@@ -1213,9 +1213,17 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	uport = uart_port_ref(state);
 	if (!uport)
 		return -EIO;
-	scoped_guard(uart_port_lock_irq, uport) {
-		memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
-		uart_enable_ms(uport);
+
+	scoped_guard(mutex, &port->mutex) {
+		if (tty_io_error(tty)) {
+			ret = -EIO;
+			goto out_deref;
+		}

[Severity: Medium]
Does this mix goto-based error handling with scope-based cleanup blocks?

According to the kernel cleanup subsystem guidelines in include/linux/cleanup.h, mixing goto-based cleanup targets (like out_deref below) with scoped_guard() in the same function can lead to confusing ownership semantics. Should this function fully convert its resources to scope-based cleanup, or stick entirely to manual locking?

[ ... ]

drivers/tty/serial/serial_core.c static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)

Quoted 4 lines

drivers/tty/serial/serial_core.c +0 -0

 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
 		scoped_guard(uart_port_lock_irq, uport)
 			memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));

[Severity: High]
This is a pre-existing issue, but does this loop lack a hangup check (tty_io_error(tty)) upon waking?

If a userspace process calls TIOCMIWAIT and blocks in schedule(), and the device is hot-unplugged or hung up, tty_port_hangup() wakes the queue. If the process ignores SIGHUP, signal_pending() is false. Without a check for device removal here, could the task resume, observe no changes to the hardware counters, and go back to sleep indefinitely?

Quoted 2 lines

set_current_state(TASK_INTERRUPTIBLE);

[Severity: High]
This is also a pre-existing issue, but does setting the task state to TASK_INTERRUPTIBLE after reading the wait conditions create a race window?

If a modem status interrupt fires immediately after the lock is released (when memcpy finishes) but before set_current_state() is called, the interrupt handler will wake the queue and set the task to TASK_RUNNING. Could the task then incorrectly overwrite its state to TASK_INTERRUPTIBLE, see the stale counters, and call schedule(), potentially losing the wakeup?

[ ... ]

Quoted 7 lines

drivers/tty/serial/serial_core.c +1 -0

 	}
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&port->delta_msr_wait, &wait);
+out_deref:
 	uart_port_deref(uport);
 
 	return ret;

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1