[PATCH v4 0/4] serial: fix ioctl hangup race

[PATCH v4 0/4] serial: fix ioctl hangup race

[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.

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

Johan


Changes in v4
 - include TIOCSETD which also access hardware

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 | 59 ++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 18 deletions(-)

-- 
2.55.0

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

[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 guard conversion in preparation for fixing a hangup race.

Fixes: 56609c050051 ("serial: serial_core: use guard()s")
Cc: stable@vger.kernel.org	# 6.18
Cc: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 95774b0f1484..1553bc6cbe7b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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);
 
-- 
2.55.0

[PATCH v4 2/4] serial: fix ioctl hangup race

[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.

Also note that the initialised check in uart_set_ldisc() added by commit
2f70e49ed860 ("serial_core: Check for port state when tty is in error
state") should have been done under the port lock to avoid racing with
uart_do_autoconfig().

Fixes: 04f378b198da ("tty: BKL pushdown")
Cc: stable@vger.kernel.org	# 2.6.26
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 41 ++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1553bc6cbe7b..139b93876229 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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)
@@ -1624,14 +1635,13 @@ static void uart_set_ldisc(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *uport;
-	struct tty_port *port = &state->port;
-
-	if (!tty_port_initialized(port))
-		return;
 
 	guard(mutex)(&state->port.mutex);
 	uport = uart_port_check(state);
-	if (uport && uport->ops->set_ldisc)
+	if (!uport || tty_io_error(tty))
+		return;
+
+	if (uport->ops->set_ldisc)
 		uport->ops->set_ldisc(uport, &tty->termios);
 }
 
@@ -1647,7 +1657,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 +1809,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;
-- 
2.55.0

[PATCH v4 3/4] serial: fix TIOCMIWAIT race

[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: sashiko-bot@kernel.org
Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 139b93876229..f7b30f77b231 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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)) ||
-- 
2.55.0

[PATCH v4 4/4] serial: abort TIOCMIWAIT on hangup

[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: sashiko-bot@kernel.org
Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f7b30f77b231..33c8f3b6ac81 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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 */
-- 
2.55.0

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

[sashiko-bot]

> 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: sashiko-bot@kernel.org
> Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

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

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

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

[sashiko-bot]

> 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 guard conversion in preparation for fixing a hangup race.
> 
> Fixes: 56609c050051 ("serial: serial_core: use guard()s")
> Cc: stable@vger.kernel.org	# 6.18
> Cc: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

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

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The `TIOCMIWAIT` implementation in `drivers/tty/mxser.c` suffers from the exact same missing hangup check as `serial_core.c`, causing indefinite sleep on hangup.
- [High] The `TIOCMIWAIT` loop in `drivers/tty/amiserial.c` is fundamentally broken, instantly returns `-EIO` without waiting, and lacks a hangup check.
--

commit a8c074101564c35207f8f5e3fb06b52d01a55d88
Author: Johan Hovold <johan@kernel.org>
Subject: serial: abort TIOCMIWAIT on hangup

The commit adds a missing hangup check to the TIOCMIWAIT wait loop in
serial_core.c to avoid indefinite sleep.

Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f7b30f77b2312..33c8f3b6ac816 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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;
> +		}
> +

[Severity: High]
This isn't a bug introduced by this patch, but the TIOCMIWAIT implementation
in drivers/tty/mxser.c suffers from the exact same missing hangup check,
causing indefinite sleep on hangup.

In drivers/tty/mxser.c:mxser_cflags_changed(), the condition for
wait_event_interruptible() is evaluated without checking for a hangup:

	ret =	((arg & TIOCM_RNG) && (cnow.rng != cprev->rng)) ||
		((arg & TIOCM_DSR) && (cnow.dsr != cprev->dsr)) ||
		((arg & TIOCM_CD)  && (cnow.dcd != cprev->dcd)) ||
		((arg & TIOCM_CTS) && (cnow.cts != cprev->cts));

Does mxser.c also need a tty_io_error() check here to prevent the process
from going back to sleep indefinitely instead of aborting when woken on
hangup?

[Severity: High]
This is a pre-existing issue, but the TIOCMIWAIT loop in
drivers/tty/amiserial.c is fundamentally broken, instantly returning
-EIO without waiting, and lacks a hangup check.

In drivers/tty/amiserial.c:rs_ioctl(), cprev is captured right before
the wait loop. Immediately inside the loop, cnow is captured again and
checked:

				cnow = info->icount; /* atomic copy */
				local_irq_restore(flags);
				if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr && 
				    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts) {
					ret = -EIO; /* no change => error */
					break;
				}

Because the copies happen almost back-to-back, the counters will almost
always be identical on the first iteration before schedule() is ever
called, causing an instant failure instead of waiting. 

Should this faulty check be removed, and a proper hangup check like the
one you're adding in this patch be included instead?

>  		schedule();
>  
>  		/* see if a signal did it */

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

Re: [PATCH v4 2/4] serial: fix ioctl hangup race

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch misses several tty driver callbacks that access hardware and can race with hangup, including those triggered by the TCXONC ioctl and racing writes.
--

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

serial: fix ioctl hangup race

This commit adds tty_io_error checks to several tty ioctls and driver
operations. It aims to ensure the port has not been hung up before accessing
the hardware, avoiding kernel panics.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 1553bc6cbe7ba..139b938762298 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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;

[Severity: High]
This isn't a bug introduced by this patch, but does this miss several other
tty driver callbacks that access hardware and can race with hangup, including
those triggered by the TCXONC ioctl and racing writes?

Invoking hardware callbacks (start_tx, stop_tx, send_xchar) on a powered-down
or unclocked UART port causes bus errors and kernel panics.

Can a user call ioctl(TCXONC) on a hung-up terminal and invoke uart_stop(),
uart_start(), or uart_send_xchar()? These do not check tty_io_error(tty) and
unconditionally access the hardware:

drivers/tty/serial/serial_core.c:uart_stop() {
	port = uart_port_ref_lock(state, &flags);
	if (port)
		port->ops->stop_tx(port);
	...
}

drivers/tty/serial/serial_core.c:uart_start() {
	port = uart_port_ref_lock(state, &flags);
	__uart_start(state);
	...
}

drivers/tty/serial/serial_core.c:uart_send_xchar() {
	if (port->ops->send_xchar)
		port->ops->send_xchar(port, ch);
	...
}

Additionally, can a concurrent write() race with uart_shutdown()? If
uart_write() acquires the port lock after uart_port_shutdown() disables the
hardware but before uart_free_xmit_buf() clears the buffer pointer, it will
pass the !state->port.xmit_buf check and invoke start_tx on the dead hardware:

drivers/tty/serial/serial_core.c:uart_write() {
	port = uart_port_ref_lock(state, &flags);
	if (!state->port.xmit_buf) {
		uart_port_unlock_deref(port, flags);
		return 0;
	}
	...
	__uart_start(state);
	...
}

[ ... ]

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