[PATCH v2] serial: fix ioctl hangup race

[PATCH v2] 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.

Fixes: 04f378b198da ("tty: BKL pushdown")
Cc: stable@vger.kernel.org	# 2.6.26
Signed-off-by: Johan Hovold <johan@kernel.org>
---

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(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 95774b0f1484..b0d3902fe4fc 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,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;
-- 
2.55.0

Re: [PATCH v2] serial: fix ioctl hangup race

[sashiko-bot]

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 <johan@kernel.org>
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.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 95774b0f1484b..b0d3902fe4fca 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
[ ... ]
> @@ -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?

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

>  
>  		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?

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

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