Skip to contentSkip to conversation

serial: fix ioctl hangup race

PATCH Superseded Johan Hovold 3 participants 13 messages Sep 7

Subsystems (2)
Stable branch TTY layer and serial drivers

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v3 cover 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.

Included in v3 are also two related fixes for issues highlighted by Sashiko.

Johan

Changes in v3
- revert guards in uart_wait_modem_status()
- fix race in TIOCMIWAIT (new)
- abort TIOCMIWAIT on hangup (new)

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

Johan Hovold (4):
serial: revert guards in uart_wait_modem_status()
serial: fix ioctl hangup race
serial: fix TIOCMIWAIT race
serial: abort TIOCMIWAIT on hangup

 drivers/tty/serial/serial_core.c | 50 +++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 13 deletions(-)
Signature · 1 line
2.55.0
author
#1 lore raw

PATCH v3 1/4 serial: revert guards in uart_wait_modem_status()

Mixing scope-based and regular cleanup is discouraged and uart_port_deref() used by uart_wait_modem_status() falls in the latter category.

Revert the premature guard conversion in preparation for fixing a hangup race.

Fixes 56609c050051 ("serial: serial_core: use guard()s")
Cc [email protected] # 6.18
Cc Jiri Slaby (SUSE) <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serial/serial_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/tty/serial/serial_core.c +7 -6
index 95774b0f1484..1553bc6cbe7b 100644
@@ -1213,15 +1213,16 @@ 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);
-	}
+	uart_port_lock_irq(uport);
+	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
+	uart_enable_ms(uport);
+	uart_port_unlock_irq(uport);
 
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
-		scoped_guard(uart_port_lock_irq, uport)
-			memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_lock_irq(uport);
+		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_unlock_irq(uport);
 
 		set_current_state(TASK_INTERRUPTIBLE);
Signature · 1 line
2.55.0
maintainer
#2 lore raw

Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()

Quoted 6 lines · Johan Hovold

Mixing scope-based and regular cleanup is discouraged and uart_port_deref() used by uart_wait_modem_status() falls in the latter category.

Revert the premature guard conversion in preparation for fixing a hangup race.

It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.

Quoted 35 lines
Fixes 56609c050051 ("serial: serial_core: use guard()s")
Cc [email protected] # 6.18
Cc Jiri Slaby (SUSE) <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
  drivers/tty/serial/serial_core.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
drivers/tty/serial/serial_core.c +7 -6
index 95774b0f1484..1553bc6cbe7b 100644
@@ -1213,15 +1213,16 @@ 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);
-	}
+	uart_port_lock_irq(uport);
+	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
+	uart_enable_ms(uport);
+	uart_port_unlock_irq(uport);
 
  	add_wait_queue(&port->delta_msr_wait, &wait);
  	for (;;) {
-		scoped_guard(uart_port_lock_irq, uport)
-			memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_lock_irq(uport);
+		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_unlock_irq(uport);
 
  		set_current_state(TASK_INTERRUPTIBLE);
Signature · 2 lines
js
suse labs
maintainer
#3 lore raw

Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()

Quoted 9 lines · Jiri Slaby
Quoted 6 lines · Johan Hovold

Mixing scope-based and regular cleanup is discouraged and uart_port_deref() used by uart_wait_modem_status() falls in the latter category.

Revert the premature guard conversion in preparation for fixing a hangup race.

It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.

(Or don't use gotos and I will.)

Signature · 2 lines
js
suse labs
author
#4 lore raw

Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()

Quoted 6 lines · Jiri Slaby
Quoted 5 lines · Jiri Slaby
Quoted 4 lines · Johan Hovold

Mixing scope-based and regular cleanup is discouraged and uart_port_deref() used by uart_wait_modem_status() falls in the latter category.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Revert the premature guard conversion in preparation for fixing a hangup race.

It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.

(Or don't use gotos and I will.)

I guess you meant "don't use guards" here. :)

Gotos don't suck. Obscure driver-specific guard constructs do, though.

So I'm with netdev here, who discourage its use. [1]

I haven't missed your fondness of them, though, and if you want to continue that path I think it's best if you do it as a follow-on change (e.g. to facilitate backporting and as it is a larger change to the driver, unrelated to the fix).

Johan

[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

AI review
#5 lore raw

Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()

In reply to Johan Hovold
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 11 lines

Mixing scope-based and regular cleanup is discouraged and uart_port_deref() used by uart_wait_modem_status() falls in the latter category.

Revert the premature guard conversion in preparation for fixing a hangup race.

Fixes 56609c050051 ("serial: serial_core: use guard()s")
Cc [email protected] # 6.18
Cc Jiri Slaby (SUSE) <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author
#6 lore raw

PATCH v3 2/4 serial: fix ioctl hangup race

In reply to Johan Hovold

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]>
---
 drivers/tty/serial/serial_core.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)
drivers/tty/serial/serial_core.c +25 -7
index 1553bc6cbe7b..4213fa3dc988 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,11 +1213,21 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	uport = uart_port_ref(state);
 	if (!uport)
 		return -EIO;
+
+	mutex_lock(&port->mutex);
+	if (tty_io_error(tty)) {
+		mutex_unlock(&port->mutex);
+		ret = -EIO;
+		goto out_deref;
+	}
+
 	uart_port_lock_irq(uport);
 	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
 	uart_enable_ms(uport);
 	uart_port_unlock_irq(uport);
 
+	mutex_unlock(&port->mutex);
+
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
 		uart_port_lock_irq(uport);

@@ -1246,6 +1256,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;

@@ -1568,7 +1579,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)

@@ -1647,7 +1658,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;
 
 	/*

@@ -1799,7 +1810,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
AI review
#7 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

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

Pre-existing issues:
- [High] uart_set_ldisc misses the tty_io_error(tty) check while holding port->mutex, allowing hardware access after the port has been shut down. --

commit 3a1dbe6dd850d3b9aa0ddbed8997abbaa897c8af
Author: Johan Hovold <[email protected]>

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.

Quoted 4 lines
drivers/tty/serial/serial_core.c +0 -0
index 1553bc6cbe7ba..4213fa3dc988b 100644

[ ... ]

drivers/tty/serial/serial_core.c static void uart_set_termios(struct tty_struct *tty, · line 1664

Quoted 9 lines
drivers/tty/serial/serial_core.c +1 -1
@@ -1647,7 +1658,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;
 
 	/*

[Severity: High]
This isn't a bug introduced by this patch, but should a similar check be added to uart_set_ldisc?

uart_set_ldisc() also acquires port->mutex and checks if the port is valid, but it does not check tty_io_error(tty) before calling uport->ops->set_ldisc:

static void uart_set_ldisc(struct tty_struct *tty)

{
        ...
        guard(mutex)(&state->port.mutex);
        uport = uart_port_check(state);
        if (uport && uport->ops->set_ldisc)
                uport->ops->set_ldisc(uport, &tty->termios);
}

If a privileged user executes TIOCSERCONFIG concurrently with a TIOCSETD ioctl, uart_do_autoconfig() can acquire the mutex, call uart_shutdown() to shut down the hardware, and set TTY_IO_ERROR. If uart_set_ldisc() then acquires the mutex, could it invoke the uport->ops->set_ldisc hardware callback on a powered-down device since it doesn't verify tty_io_error(tty)?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author
#8 lore raw
Quoted 4 lines · On Mon, Sep 07, 2026 at 07:00:34AM +0000, [email protected] wrote:

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

Pre-existing issues:
- [High] uart_set_ldisc misses the tty_io_error(tty) check while holding port->mutex, allowing hardware access after the port has been shut down.

This looks valid so I'll include uart_set_ldisc() in a v4 in a couple of days.

Johan

author
#9 lore raw

PATCH v3 3/4 serial: fix TIOCMIWAIT race

In reply to Johan Hovold

The task state must be updated before checking the wakeup condition to avoid missing a racing modem status update.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +2 -2
index 4213fa3dc988..c2c2c8fe1a4c 100644
@@ -1230,12 +1230,12 @@ static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *sta
 
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
 		uart_port_lock_irq(uport);
 		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
 		uart_port_unlock_irq(uport);
 
-		set_current_state(TASK_INTERRUPTIBLE);
-
 		if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
 		    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
 		    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
Signature · 1 line
2.55.0
AI review
#10 lore raw

Re: [PATCH v3 3/4] serial: fix TIOCMIWAIT race

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines

The task state must be updated before checking the wakeup condition to avoid missing a racing modem status update.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
author
#11 lore raw

PATCH v3 4/4 serial: abort TIOCMIWAIT on hangup

In reply to Johan Hovold

Add the missing hangup check to the TIOCMIWAIT wait loop to avoid going back to sleep (indefinitely) after being woken on hangup.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serial/serial_core.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/tty/serial/serial_core.c +5 -0
index c2c2c8fe1a4c..45d160743ea0 100644
@@ -1244,6 +1244,11 @@ static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *sta
 			break;
 		}
 
+		if (tty_io_error(tty)) {
+			ret = -EIO;
+			break;
+		}
+
 		schedule();
 
 		/* see if a signal did it */
Signature · 1 line
2.55.0
AI review
#12 lore raw

Re: [PATCH v3 4/4] serial: abort TIOCMIWAIT on hangup

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines

Add the missing hangup check to the TIOCMIWAIT wait loop to avoid going back to sleep (indefinitely) after being woken on hangup.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

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