tty: fix break race

PATCH In review Adriano Cordova 3 participants 6 messages Sep 3

Subsystems (4)

Stable branch USB TTY layer and serial drivers USB serial

lore

Share & tools

lore

Adriano Cordova

author

2026-09-03 04:25 UTC #0 lore raw

v2 cover tty: fix break race

A timed break (TCSBRK/TCSBRKP) and the unconditional TIOCSBRK/TIOCCBRK can race against USB disconnect and call break_ctl() after the device is gone, dereferencing freed port data in keyspan_break_ctl().

Both patches are needed: the tty core must not issue break_ctl() to a hung-up tty, and serial_break() must not call the driver after disconnect. This is the same two-layer protection read and write already use.

Changes since v1:
- v1 https://lore.kernel.org/stable/[email protected]/T/#t
- Proper check on the tty side, plus check on the USB side.
- Tested syzbot

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

Tested-by [email protected]

Adriano Cordova (2):
USB: serial: fix break_ctl() NULL-pointer dereference
tty: fix break race

 drivers/tty/tty_io.c            | 19 +++++++++++++++----
 drivers/usb/serial/usb-serial.c |  3 +++
 2 files changed, 18 insertions(+), 4 deletions(-)

Signature · 1 line

2.51.0

Adriano Cordova

author

2026-09-03 04:25 UTC #1 lore raw

PATCH v2 1/2 USB: serial: fix break_ctl() NULL-pointer dereference

Don't call into the driver after disconnect, just like serial_write() already does.

Fixes 9e98966c7bb9 ("tty: rework break handling")

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

Tested-by [email protected]

Cc [email protected]

Signed-off-by Adriano Cordova <[email protected]>

---
 drivers/usb/serial/usb-serial.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/serial/usb-serial.c +3 -0

index 17edc057a311..5f42bfe39212 100644
@@ -532,6 +532,9 @@ static int serial_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 
+	if (port->serial->dev->state == USB_STATE_NOTATTACHED)
+		return -ENODEV;
+
 	dev_dbg(&port->dev, "%s\n", __func__);
 
 	if (port->serial->type->break_ctl)

Signature · 1 line

2.51.0

[email protected]

AI review

2026-09-03 04:41 UTC #2 lore raw

Re: [PATCH v2 1/2] USB: serial: fix break_ctl() NULL-pointer dereference

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's attempt to fix it is flawed. The newly added lockless state check of USB_STATE_NOTATTACHED in serial_break leaves a Time-of-Check to Time-of-Use (TOCTOU) race window, failing to completely prevent Use-After-Free or NULL-pointer dereference vulnerabilities upon concurrent device disconnect.
- [High] This problem wasn't introduced by this patch, but while the patch aims to prevent calling into the driver after disconnect, it completely omits other TTY operations in the generic usb-serial driver, leaving them vulnerable to the same Use-After-Free crashes.
--

commit 35b2c0cc5ee711aab91cfbffe24f9565620f8f65
Author: Adriano Cordova <[email protected]>
USB: serial: fix break_ctl() NULL-pointer dereference

This commit attempts to fix a NULL-pointer dereference by checking if the USB device is detached before calling into the sub-driver's break_ctl callback.

Link https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

drivers/usb/serial/usb-serial.c static int serial_break(struct tty_struct *tty, int break_state) · line 537

Quoted 10 lines

drivers/usb/serial/usb-serial.c +3 -0

@@ -532,6 +532,9 @@ static int serial_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 
+	if (port->serial->dev->state == USB_STATE_NOTATTACHED)
+		return -ENODEV;
+

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check leave a Time-of-Check to Time-of-Use (TOCTOU) race window?

If the thread executing serial_break() in drivers/usb/serial/usb-serial.c evaluates USB_STATE_NOTATTACHED to false and gets preempted, could a concurrent usb_disconnect() finish port_remove() and free driver-private data before the thread resumes?

Quoted 4 lines

drivers/usb/serial/usb-serial.c +0 -0
 	dev_dbg(&port->dev, "%s\n", __func__);
 
 	if (port->serial->type->break_ctl)
return port->serial->type->break_ctl(tty, break_state);

[Severity: High]
This is a pre-existing issue, but does this still execute the sub-driver's break_ctl() function on freed memory or a NULL pointer if the race condition described above triggers during a concurrent TIOCSBRK or TIOCCBRK ioctl?

Additionally, this is a pre-existing issue, but while this patch aims to prevent calling into the driver after disconnect in serial_break(), what protects other TTY operations in the generic usb-serial driver from the same Use-After-Free crashes?

For example, serial_tiocmget() unconditionally calls into sub-drivers without checking for disconnect:

drivers/usb/serial/usb-serial.c:serial_tiocmget() {
    ...
	if (port->serial->type->tiocmget)
		return port->serial->type->tiocmget(tty);
    ...
}

Similar unprotected calls appear to happen in serial_ioctl(), serial_tiocmset(), serial_throttle(), and serial_unthrottle(). Can these also be triggered by an unprivileged user spamming concurrent TTY ioctls (like TIOCMGET) while the physical device is unplugged?

Signature · 1 line

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

Adriano Cordova

author

2026-09-03 04:25 UTC #3 lore raw

PATCH v2 2/2 tty: fix break race

In reply to Adriano Cordova

A timed break (TCSBRK and TCSBRKP) can race against hangup and call break_ctl() on a tty whose driver data is gone, causing a use-after-free or NULL-pointer dereference in the driver.

Check the tty hung-up state before each break_ctl() call, including the unconditional TIOCSBRK/TIOCCBRK paths, so the driver is no longer called after hangup.

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

Tested-by [email protected]

Cc [email protected]

Signed-off-by Adriano Cordova <[email protected]>

---
 drivers/tty/tty_io.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

drivers/tty/tty_io.c +15 -4

index 48569035da56..272f2839acdb 100644
@@ -2429,6 +2429,7 @@ static int tiocgetd(struct tty_struct *tty, int __user *p)
 
 /**
  * send_break - performed time break
+ * @file: file object
  * @tty: device to break on
  * @duration: timeout in mS
  *

@@ -2438,13 +2439,16 @@ 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)
 {
 	int retval;
 
 	if (tty->ops->break_ctl == NULL)
 		return 0;
 
+	if (tty_hung_up_p(file))
+		return -EIO;
+
 	if (tty->driver->flags & TTY_DRIVER_HARDWARE_BREAK)
 		return tty->ops->break_ctl(tty, duration);

@@ -2455,7 +2459,10 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	retval = tty->ops->break_ctl(tty, -1);
 	if (!retval) {
 		msleep_interruptible(duration);
-		retval = tty->ops->break_ctl(tty, 0);
+		if (tty_hung_up_p(file))
+			retval = -EIO;
+		else
+			retval = tty->ops->break_ctl(tty, 0);
 	} else if (retval == -EOPNOTSUPP) {
 		/* some drivers can tell only dynamically */
 		retval = 0;

@@ -2714,10 +2721,14 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	 * Break handling
 	 */
 	case TIOCSBRK:	/* Turn break on, unconditionally */
+		if (tty_hung_up_p(file))
+			return -EIO;
 		if (tty->ops->break_ctl)
 			return tty->ops->break_ctl(tty, -1);
 		return 0;
 	case TIOCCBRK:	/* Turn break off, unconditionally */
+		if (tty_hung_up_p(file))
+			return -EIO;
 		if (tty->ops->break_ctl)
 			return tty->ops->break_ctl(tty, 0);
 		return 0;

@@ -2727,10 +2738,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);

Signature · 1 line

2.51.0

[email protected]

AI review

2026-09-03 04:40 UTC #4 lore raw

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

Pre-existing issues:
- [High] The lockless checks of tty_hung_up_p(file) introduce a Time-of-Check to Time-of-Use (TOCTOU) race against __tty_hangup, failing to fully resolve the Use-After-Free/NULL pointer dereference. --

commit 8d56321f205bd51f6c216651862e6a270f2637ea
Author: Adriano Cordova <[email protected]>

tty: fix break race

This commit attempts to fix a race condition where a timed break can call break_ctl() on a tty whose driver data is gone, causing a use-after-free or NULL-pointer dereference. It adds checks for the tty hung-up state before each break_ctl() call.

Link https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

Quoted 4 lines

drivers/tty/tty_io.c +0 -0
index 48569035da56e..272f2839acdba 100644

[ ... ]

Quoted 5 lines

drivers/tty/tty_io.c +0 -1

@@ -2438,13 +2439,16 @@ 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)

drivers/tty/tty_io.c static int tiocgetd(struct tty_struct *tty, int __user *p) · line 2451

Quoted 10 lines

drivers/tty/tty_io.c +4 -0

+static int send_break(struct file *file, struct tty_struct *tty, unsigned int duration)
 {
 	int retval;
 
 	if (tty->ops->break_ctl == NULL)
 		return 0;
 
+	if (tty_hung_up_p(file))
+		return -EIO;
+

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check fully resolve the Time-of-Check to Time-of-Use (TOCTOU) race? If a concurrent thread executes __tty_hangup() immediately after tty_hung_up_p() evaluates to false, could __tty_hangup() tear down the driver data before tty->ops->break_ctl() is executed on the hardware break path, leading to a use-after-free?

drivers/tty/tty_io.c static int send_break(struct tty_struct *tty, unsigned int duration) · line 2465

Quoted 12 lines

drivers/tty/tty_io.c +4 -1

 	if (tty->driver->flags & TTY_DRIVER_HARDWARE_BREAK)
 		return tty->ops->break_ctl(tty, duration);

@@ -2455,7 +2459,10 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	retval = tty->ops->break_ctl(tty, -1);
 	if (!retval) {
 		msleep_interruptible(duration);
-		retval = tty->ops->break_ctl(tty, 0);
+		if (tty_hung_up_p(file))
+			retval = -EIO;
+		else
+			retval = tty->ops->break_ctl(tty, 0);

[Severity: High]
This is a pre-existing issue, but does checking tty_hung_up_p() here under atomic_write_lock provide mutual exclusion against __tty_hangup()? Could a concurrent hangup occur right after this check, causing the subsequent call to tty->ops->break_ctl() to dereference freed driver data?

Quoted 3 lines

drivers/tty/tty_io.c +0 -0

 	} else if (retval == -EOPNOTSUPP) {
 		/* some drivers can tell only dynamically */
 		retval = 0;

[ ... ]

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2727

Quoted 8 lines

drivers/tty/tty_io.c +2 -0

@@ -2714,10 +2721,14 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	 * Break handling
 	 */
 	case TIOCSBRK:	/* Turn break on, unconditionally */
+		if (tty_hung_up_p(file))
+			return -EIO;
 		if (tty->ops->break_ctl)
 			return tty->ops->break_ctl(tty, -1);

[Severity: High]
This isn't a bug introduced by this patch, but do these unconditional break paths suffer from the same TOCTOU race? Since they do not hold tty_lock, could a concurrent __tty_hangup() free the driver data between the tty_hung_up_p() check and the break_ctl() call?

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2733

Quoted 6 lines

drivers/tty/tty_io.c +2 -0

 		return 0;
 	case TIOCCBRK:	/* Turn break off, unconditionally */
+		if (tty_hung_up_p(file))
+			return -EIO;
 		if (tty->ops->break_ctl)
 			return tty->ops->break_ctl(tty, 0);

[Severity: High]
This is a pre-existing issue, but similar to TIOCSBRK, is this unprotected check before unconditional break off vulnerable to a concurrent hangup?

Quoted 1 line

drivers/tty/tty_io.c +0 -0
 		return 0;

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Johan Hovold

maintainer

2026-09-03 06:22 UTC #5 lore raw

In reply to Adriano Cordova

Quoted 8 lines · Adriano Cordova

A timed break (TCSBRK/TCSBRKP) and the unconditional TIOCSBRK/TIOCCBRK can race against USB disconnect and call break_ctl() after the device is gone, dereferencing freed port data in keyspan_break_ctl().
Both patches are needed: the tty core must not issue break_ctl() to a hung-up tty, and serial_break() must not call the driver after disconnect. This is the same two-layer protection read and write already use.

This doesn't work and as I said in my reply to the syzbot report I'm preparing a proper fix.

Johan