Date:	Sat, 12 Dec 2015 16:36:30 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Jiri Slaby <jslaby@...e.cz>, linux-kernel@...r.kernel.org,
	Peter Hurley <peter@...leysoftware.com>
Subject: [PATCH 6/8] serial: core: Prevent unsafe uart port access, part 1

uart_remove_one_port() may race with every serial core operation
requiring a valid dereference of state->uart_port. In particular,
uart_remove_one_port() may unlink the uart port concurrently with
any serial core operation that may dereference same.

Ensure safe dereference for those operations that already claim
the port->mutex, and extend that guarantee for trivial cases,
such as the ioctl handlers.

For ioctls, return -EIO as if the port has been hung up (since it
has).

Signed-off-by: Peter Hurley <peter@...leysoftware.com>
---
 drivers/tty/serial/serial_core.c | 115 ++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 16c4c48..2806f52 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -681,10 +681,11 @@ static void uart_unthrottle(struct tty_struct *tty)
 		uart_set_mctrl(port, TIOCM_RTS);
 }
 
-static void uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
+static int uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 {
 	struct uart_state *state = container_of(port, struct uart_state, port);
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
+	int ret = -ENODEV;
 
 	memset(retinfo, 0, sizeof(*retinfo));
 
@@ -693,6 +694,10 @@ static void uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 	 * occur as we go
 	 */
 	mutex_lock(&port->mutex);
+	uport = state->uart_port;
+	if (!uport)
+		goto out;
+
 	retinfo->type	    = uport->type;
 	retinfo->line	    = uport->line;
 	retinfo->port	    = uport->iobase;
@@ -711,7 +716,11 @@ static void uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 	retinfo->io_type         = uport->iotype;
 	retinfo->iomem_reg_shift = uport->regshift;
 	retinfo->iomem_base      = (void *)(unsigned long)uport->mapbase;
+
+	ret = 0;
+out:
 	mutex_unlock(&port->mutex);
+	return ret;
 }
 
 static int uart_get_info_user(struct tty_port *port,
@@ -719,7 +728,8 @@ static int uart_get_info_user(struct tty_port *port,
 {
 	struct serial_struct tmp;
 
-	uart_get_info(port, &tmp);
+	if (uart_get_info(port, &tmp) < 0)
+		return -EIO;
 
 	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
 		return -EFAULT;
@@ -737,6 +747,9 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	upf_t old_flags, new_flags;
 	int retval = 0;
 
+	if (!uport)
+		return -EIO;
+
 	new_port = new_info->port;
 	if (HIGH_BITS_OFFSET)
 		new_port += (unsigned long) new_info->port_high << HIGH_BITS_OFFSET;
@@ -946,8 +959,6 @@ static int uart_set_info_user(struct tty_struct *tty, struct uart_state *state,
  *	@tty: tty associated with the UART
  *	@state: UART being queried
  *	@value: returned modem value
- *
- *	Note: uart_ioctl protects us against hangups.
  */
 static int uart_get_lsr_info(struct tty_struct *tty,
 			struct uart_state *state, unsigned int __user *value)
@@ -975,18 +986,22 @@ static int uart_tiocmget(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
 	struct tty_port *port = &state->port;
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
 	int result = -EIO;
 
 	mutex_lock(&port->mutex);
+	uport = state->uart_port;
+	if (!uport)
+		goto out;
+
 	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
 		result = uport->mctrl;
 		spin_lock_irq(&uport->lock);
 		result |= uport->ops->get_mctrl(uport);
 		spin_unlock_irq(&uport->lock);
 	}
+out:
 	mutex_unlock(&port->mutex);
-
 	return result;
 }
 
@@ -994,15 +1009,20 @@ static int
 uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear)
 {
 	struct uart_state *state = tty->driver_data;
-	struct uart_port *uport = state->uart_port;
 	struct tty_port *port = &state->port;
+	struct uart_port *uport;
 	int ret = -EIO;
 
 	mutex_lock(&port->mutex);
+	uport = state->uart_port;
+	if (!uport)
+		goto out;
+
 	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
 		uart_update_mctrl(uport, set, clear);
 		ret = 0;
 	}
+out:
 	mutex_unlock(&port->mutex);
 	return ret;
 }
@@ -1011,21 +1031,26 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct uart_state *state = tty->driver_data;
 	struct tty_port *port = &state->port;
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
+	int ret = -EIO;
 
 	mutex_lock(&port->mutex);
+	uport = state->uart_port;
+	if (!uport)
+		goto out;
 
 	if (uport->type != PORT_UNKNOWN)
 		uport->ops->break_ctl(uport, break_state);
-
+	ret = 0;
+out:
 	mutex_unlock(&port->mutex);
-	return 0;
+	return ret;
 }
 
 static int uart_do_autoconfig(struct tty_struct *tty,struct uart_state *state)
 {
-	struct uart_port *uport = state->uart_port;
 	struct tty_port *port = &state->port;
+	struct uart_port *uport;
 	int flags, ret;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1039,6 +1064,12 @@ static int uart_do_autoconfig(struct tty_struct *tty,struct uart_state *state)
 	if (mutex_lock_interruptible(&port->mutex))
 		return -ERESTARTSYS;
 
+	uport = state->uart_port;
+	if (!uport) {
+		ret = -EIO;
+		goto out;
+	}
+
 	ret = -EBUSY;
 	if (tty_port_users(port) == 1) {
 		uart_shutdown(tty, state);
@@ -1062,6 +1093,7 @@ static int uart_do_autoconfig(struct tty_struct *tty,struct uart_state *state)
 
 		ret = uart_startup(tty, state, 1);
 	}
+out:
 	mutex_unlock(&port->mutex);
 	return ret;
 }
@@ -1210,11 +1242,11 @@ static int uart_set_rs485_config(struct uart_port *port,
  * Called via sys_ioctl.  We can use spin_lock_irq() here.
  */
 static int
-uart_ioctl(struct tty_struct *tty, unsigned int cmd,
-	   unsigned long arg)
+uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 {
 	struct uart_state *state = tty->driver_data;
 	struct tty_port *port = &state->port;
+	struct uart_port *uport;
 	void __user *uarg = (void __user *)arg;
 	int ret = -ENOIOCTLCMD;
 
@@ -1266,8 +1298,9 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
 		goto out;
 
 	mutex_lock(&port->mutex);
+	uport = state->uart_port;
 
-	if (tty->flags & (1 << TTY_IO_ERROR)) {
+	if (!uport || test_bit(TTY_IO_ERROR, &tty->flags)) {
 		ret = -EIO;
 		goto out_up;
 	}
@@ -1283,19 +1316,17 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
 		break;
 
 	case TIOCGRS485:
-		ret = uart_get_rs485_config(state->uart_port, uarg);
+		ret = uart_get_rs485_config(uport, uarg);
 		break;
 
 	case TIOCSRS485:
-		ret = uart_set_rs485_config(state->uart_port, uarg);
+		ret = uart_set_rs485_config(uport, uarg);
 		break;
-	default: {
-		struct uart_port *uport = state->uart_port;
+	default:
 		if (uport->ops->ioctl)
 			ret = uport->ops->ioctl(uport, cmd, arg);
 		break;
 	}
-	}
 out_up:
 	mutex_unlock(&port->mutex);
 out:
@@ -1305,24 +1336,29 @@ out:
 static void uart_set_ldisc(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
 
-	if (uport->ops->set_ldisc) {
-		mutex_lock(&state->port.mutex);
+	mutex_lock(&state->port.mutex);
+	uport = state->uart_port;
+	if (uport && uport->ops->set_ldisc)
 		uport->ops->set_ldisc(uport, &tty->termios);
-		mutex_unlock(&state->port.mutex);
-	}
+	mutex_unlock(&state->port.mutex);
 }
 
 static void uart_set_termios(struct tty_struct *tty,
 						struct ktermios *old_termios)
 {
 	struct uart_state *state = tty->driver_data;
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
 	unsigned int cflag = tty->termios.c_cflag;
 	unsigned int iflag_mask = IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK;
 	bool sw_changed = false;
 
+	mutex_lock(&state->port.mutex);
+	uport = state->uart_port;
+	if (!uport)
+		goto out;
+
 	/*
 	 * Drivers doing software flow control also need to know
 	 * about changes to these input settings.
@@ -1345,12 +1381,10 @@ static void uart_set_termios(struct tty_struct *tty,
 	    tty->termios.c_ispeed == old_termios->c_ispeed &&
 	    ((tty->termios.c_iflag ^ old_termios->c_iflag) & iflag_mask) == 0 &&
 	    !sw_changed) {
-		return;
+		goto out;
 	}
 
-	mutex_lock(&state->port.mutex);
 	uart_change_speed(tty, state, old_termios);
-	mutex_unlock(&state->port.mutex);
 	/* reload cflag from termios; port driver may have overriden flags */
 	cflag = tty->termios.c_cflag;
 
@@ -1364,6 +1398,8 @@ static void uart_set_termios(struct tty_struct *tty,
 			mask |= TIOCM_RTS;
 		uart_set_mctrl(uport, mask);
 	}
+out:
+	mutex_unlock(&state->port.mutex);
 }
 
 /*
@@ -1659,13 +1695,15 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 	struct uart_state *state = drv->state + i;
 	struct tty_port *port = &state->port;
 	enum uart_pm_state pm_state;
-	struct uart_port *uport = state->uart_port;
+	struct uart_port *uport;
 	char stat_buf[32];
 	unsigned int status;
 	int mmio;
 
+	mutex_lock(&port->mutex);
+	uport = state->uart_port;
 	if (!uport)
-		return;
+		goto out;
 
 	mmio = uport->iotype >= UPIO_MEM;
 	seq_printf(m, "%d: uart:%s %s%08llX irq:%d",
@@ -1677,11 +1715,10 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 
 	if (uport->type == PORT_UNKNOWN) {
 		seq_putc(m, '\n');
-		return;
+		goto out;
 	}
 
 	if (capable(CAP_SYS_ADMIN)) {
-		mutex_lock(&port->mutex);
 		pm_state = state->pm_state;
 		if (pm_state != UART_PM_STATE_ON)
 			uart_change_pm(state, UART_PM_STATE_ON);
@@ -1690,7 +1727,6 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 		spin_unlock_irq(&uport->lock);
 		if (pm_state != UART_PM_STATE_ON)
 			uart_change_pm(state, pm_state);
-		mutex_unlock(&port->mutex);
 
 		seq_printf(m, " tx:%d rx:%d",
 				uport->icount.tx, uport->icount.rx);
@@ -1732,6 +1768,9 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 	seq_putc(m, '\n');
 #undef STATBIT
 #undef INFOBIT
+out:
+	mutex_unlock(&port->mutex);
+	return;
 }
 
 static int uart_proc_show(struct seq_file *m, void *v)
@@ -1995,11 +2034,11 @@ EXPORT_SYMBOL_GPL(uart_set_options);
 static void uart_change_pm(struct uart_state *state,
 			   enum uart_pm_state pm_state)
 {
-	struct uart_port *port = state->uart_port;
+	struct uart_port *uport = state->uart_port;
 
 	if (state->pm_state != pm_state) {
-		if (port->ops->pm)
-			port->ops->pm(port, pm_state, state->pm_state);
+		if (uport && uport->ops->pm)
+			uport->ops->pm(uport, pm_state, state->pm_state);
 		state->pm_state = pm_state;
 	}
 }
@@ -2803,7 +2842,9 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 */
 	uport->type = PORT_UNKNOWN;
 
+	mutex_lock(&port->mutex);
 	state->uart_port = NULL;
+	mutex_unlock(&port->mutex);
 out:
 	mutex_unlock(&port_mutex);
 
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists