Re: PROBLEM: kernel NULL pointer dereference when yanking ftdi usb-serial during BREAK

[Johan Hovold <johan@kernel.org>]

On Wed, Jun 07, 2023 at 10:20:31PM -0400, Nick Bowler wrote:

> I just hit an oops when unplugging my usb serial adapter.  So naturally,
> I tried it again, and found if I use minicom to send BREAK and then
> quickly yank the cable, I can reliably cause this oops every single
> time.

Well, don't do that then. ;)

I ran into this a couple of years ago myself but ended up preempted
before I could finish the fix I was working on.

The problem is that the tty layer happily calls back into the driver to
disable the break state after the device is gone.

Something like the below fixes it. I'll revisit this in a couple of
weeks.

Johan


From 14d3b01c02760979ede01d064fb1700d48c3ee68 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 18 Oct 2021 09:43:38 +0200
Subject: [PATCH] tty: fix break race

TCSBRK and TCSBRKP can race with hangup and end up calling into a tty
driver for a device that is already gone.

FIXME: expand

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_io.c | 68 ++++++++++++++++++++++++++++++--------------
 include/linux/tty.h  |  1 +
 2 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c84be40fb8df..85fe52135f4b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -630,6 +630,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 
 	tty_ldisc_hangup(tty, cons_filp != NULL);
 
+	wake_up_interruptible(&tty->break_wait);
+
 	spin_lock_irq(&tty->ctrl.lock);
 	clear_bit(TTY_THROTTLED, &tty->flags);
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
@@ -1757,10 +1759,10 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	/*
 	 * Sanity check: if tty->count is going to zero, there shouldn't be
-	 * any waiters on tty->read_wait or tty->write_wait.  We test the
-	 * wait queues and kick everyone out _before_ actually starting to
-	 * close.  This ensures that we won't block while releasing the tty
-	 * structure.
+	 * any waiters on tty->read_wait, tty->write_wait or tty->break_wait.
+	 * We test the wait queues and kick everyone out _before_ actually
+	 * starting to close.  This ensures that we won't block while
+	 * releasing the tty structure.
 	 *
 	 * The test for the o_tty closing is necessary, since the master and
 	 * slave sides may close in any order.  If the slave side closes out
@@ -1780,6 +1782,10 @@ int tty_release(struct inode *inode, struct file *filp)
 				wake_up_poll(&tty->write_wait, EPOLLOUT);
 				do_sleep++;
 			}
+			if (waitqueue_active(&tty->break_wait)) {
+				wake_up(&tty->break_wait);
+				do_sleep++;
+			}
 		}
 		if (o_tty && o_tty->count <= 1) {
 			if (waitqueue_active(&o_tty->read_wait)) {
@@ -2461,6 +2467,7 @@ static int tiocgetd(struct tty_struct *tty, int __user *p)
  * send_break	-	performed time break
  * @tty: device to break on
  * @duration: timeout in mS
+ * @file: file object
  *
  * Perform a timed break on hardware that lacks its own driver level timed
  * break functionality.
@@ -2468,30 +2475,46 @@ static int tiocgetd(struct tty_struct *tty, int __user *p)
  * Locking:
  *	@tty->atomic_write_lock serializes
  */
-static int send_break(struct tty_struct *tty, unsigned int duration)
+static int send_break(struct file *file, struct tty_struct *tty, unsigned int duration)
 {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	int retval;
 
 	if (tty->ops->break_ctl == NULL)
 		return 0;
 
 	if (tty->driver->flags & TTY_DRIVER_HARDWARE_BREAK)
-		retval = tty->ops->break_ctl(tty, duration);
-	else {
-		/* Do the work ourselves */
-		if (tty_write_lock(tty, 0) < 0)
-			return -EINTR;
-		retval = tty->ops->break_ctl(tty, -1);
-		if (retval)
-			goto out;
-		if (!signal_pending(current))
-			msleep_interruptible(duration);
-		retval = tty->ops->break_ctl(tty, 0);
-out:
-		tty_write_unlock(tty);
-		if (signal_pending(current))
-			retval = -EINTR;
+		return tty->ops->break_ctl(tty, duration);
+
+	if (tty_write_lock(tty, 0) < 0)
+		return -EINTR;
+
+	add_wait_queue(&tty->break_wait, &wait);
+
+	if (signal_pending(current)) {
+		retval = -EINTR;
+		goto out;
+	}
+
+	if (tty_hung_up_p(file)) {
+		retval = -EIO;
+		goto out;
 	}
+
+	retval = tty->ops->break_ctl(tty, -1);
+	if (retval)
+		goto out;
+
+	wait_woken(&wait, TASK_INTERRUPTIBLE, msecs_to_jiffies(duration) + 1);
+
+	retval = tty->ops->break_ctl(tty, 0);
+out:
+	remove_wait_queue(&tty->break_wait, &wait);
+	tty_write_unlock(tty);
+
+	if (signal_pending(current))
+		retval = -EINTR;
+
 	return retval;
 }
 
@@ -2739,10 +2762,10 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		 * This is used by the tcdrain() termios function.
 		 */
 		if (!arg)
-			return send_break(tty, 250);
+			return send_break(file, tty, 250);
 		return 0;
 	case TCSBRKP:	/* support for POSIX tcsendbreak() */
-		return send_break(tty, arg ? arg*100 : 250);
+		return send_break(file, tty, arg ? arg * 100 : 250);
 
 	case TIOCMGET:
 		return tty_tiocmget(tty, p);
@@ -3105,6 +3128,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
 	init_ldsem(&tty->ldisc_sem);
 	init_waitqueue_head(&tty->write_wait);
 	init_waitqueue_head(&tty->read_wait);
+	init_waitqueue_head(&tty->break_wait);
 	INIT_WORK(&tty->hangup_work, do_tty_hangup);
 	mutex_init(&tty->atomic_write_lock);
 	spin_lock_init(&tty->ctrl.lock);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8d5d9997aca..49b0e3b82901 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -235,6 +235,7 @@ struct tty_struct {
 	struct fasync_struct *fasync;
 	wait_queue_head_t write_wait;
 	wait_queue_head_t read_wait;
+	wait_queue_head_t break_wait;
 	struct work_struct hangup_work;
 	void *disc_data;
 	void *driver_data;
-- 
2.39.3