[PATCH] usb: gadget: u_serial: fix gs_port use-after-free on open/unbind race

[PATCH] usb: gadget: u_serial: fix gs_port use-after-free on open/unbind race

[Varun Singh]

syzbot reports a use-after-free when a USB gadget serial function is
removed through configfs while /dev/ttyGS* is being opened:

  BUG: KASAN: slab-use-after-free in check_tty_count+0x134/0x2b0
   tty_release+0x95/0x1300 drivers/tty/tty_io.c:1728

gs_port embeds a tty_port, which the TTY core stores in tty->port without
taking a reference, so the driver must keep it alive. u_serial waited for
port.count to reach zero in gserial_free_port() and then freed the port.
That never worked: gs_open() returns -ENODEV without incrementing the
count once gserial_free_line() has cleared the ports[] slot, so the wait
completes immediately, the port is freed, and tty_release() reads it.

Use the tty_port kref and move the lookup into .install, which runs before
the core publishes tty->port. gs_install() takes a reference under
ports[].lock and stores the port in tty->driver_data, .cleanup drops it,
.destruct frees the port, and gserial_free_port() drops the allocation
reference instead of calling kfree(). gs_open() then takes the port from
tty->driver_data and cannot race gserial_free_line() at all.

Drop the close_wait/gs_closed() handshake: the kref makes it redundant,
and once the count is correct it blocks rmdir until userspace closes
/dev/ttyGS*, which shows up as a hung task. Keep the
cancel_delayed_work_sync(), as cancelling from .destruct instead leaves a
use-after-free in the work cancellation path. Unregister the tty device
before freeing the port rather than after.

To reproduce: mount configfs and devtmpfs, create a gadget function that
uses u_serial such as obex or acm, then repeatedly open /dev/ttyGS0, hold
some of those opens so they overlap, and remove the function directory
with rmdir. An unpatched KASAN kernel reports slab-use-after-free within a
few thousand rounds.

Found with the assistance of Claude Opus 5.

Fixes: 19b10a8828a6 ("usb: gadget: allocate & giveback serial ports instead hard code them")
Reported-by: syzbot+39a2135de1558208d3b3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=39a2135de1558208d3b3
Tested-by: syzbot+39a2135de1558208d3b3@syzkaller.appspotmail.com
Signed-off-by: Varun Singh <varun.singh10011@gmail.com>
---
 drivers/usb/gadget/function/u_serial.c | 88 +++++++++++++++-----------
 1 file changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc66..906137033 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -125,7 +125,6 @@ struct gs_port {
 	struct kfifo		port_write_buf;
 	wait_queue_head_t	drain_wait;	/* wait while writes drain */
 	bool                    write_busy;
-	wait_queue_head_t	close_wait;
 	bool			suspended;	/* port suspended */
 	bool			start_delayed;	/* delay start when suspended */
 	struct async_icount	icount;
@@ -610,17 +609,9 @@ static int gserial_wakeup_host(struct gserial *gser)
  */
 static int gs_open(struct tty_struct *tty, struct file *file)
 {
-	int		port_num = tty->index;
-	struct gs_port	*port;
+	struct gs_port	*port = tty->driver_data;
 	int		status = 0;
 
-	mutex_lock(&ports[port_num].lock);
-	port = ports[port_num].port;
-	if (!port) {
-		status = -ENODEV;
-		goto out;
-	}
-
 	spin_lock_irq(&port->port_lock);
 
 	/* allocate circular buffer on first open */
@@ -629,16 +620,17 @@ static int gs_open(struct tty_struct *tty, struct file *file)
 		spin_unlock_irq(&port->port_lock);
 
 		/*
-		 * portmaster's mutex still protects from simultaneous open(),
-		 * and close() can't happen, yet.
+		 * The TTY core holds tty_lock across ->open(), so this is
+		 * serialised against a simultaneous open() or close() of
+		 * the same tty.
 		 */
 
 		status = kfifo_alloc(&port->port_write_buf,
 				     WRITE_BUF_SIZE, GFP_KERNEL);
 		if (status) {
 			pr_debug("gs_open: ttyGS%d (%p,%p) no buffer\n",
-				 port_num, tty, file);
-			goto out;
+				 port->port_num, tty, file);
+			return status;
 		}
 
 		spin_lock_irq(&port->port_lock);
@@ -648,7 +640,6 @@ static int gs_open(struct tty_struct *tty, struct file *file)
 	if (port->port.count++)
 		goto exit_unlock_port;
 
-	tty->driver_data = port;
 	port->port.tty = tty;
 
 	/* if connected, start the I/O stream */
@@ -672,8 +663,6 @@ static int gs_open(struct tty_struct *tty, struct file *file)
 
 exit_unlock_port:
 	spin_unlock_irq(&port->port_lock);
-out:
-	mutex_unlock(&ports[port_num].lock);
 	return status;
 }
 
@@ -693,7 +682,7 @@ static int gs_close_flush_done(struct gs_port *p)
 static void gs_close(struct tty_struct *tty, struct file *file)
 {
 	struct gs_port *port = tty->driver_data;
-	struct gserial	*gser;
+	struct gserial *gser;
 
 	spin_lock_irq(&port->port_lock);
 
@@ -744,7 +733,6 @@ static void gs_close(struct tty_struct *tty, struct file *file)
 	pr_debug("gs_close: ttyGS%d (%p,%p) done!\n",
 			port->port_num, tty, file);
 
-	wake_up(&port->close_wait);
 exit:
 	spin_unlock_irq(&port->port_lock);
 }
@@ -908,7 +896,40 @@ static int gs_get_icount(struct tty_struct *tty,
 	return 0;
 }
 
+static int gs_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	int port_num = tty->index;
+	struct gs_port *port;
+	int status;
+
+	mutex_lock(&ports[port_num].lock);
+	port = ports[port_num].port;
+	if (!port) {
+		mutex_unlock(&ports[port_num].lock);
+		return -ENODEV;
+	}
+	tty_port_get(&port->port);
+	mutex_unlock(&ports[port_num].lock);
+
+	status = tty_port_install(&port->port, driver, tty);
+	if (status) {
+		tty_port_put(&port->port);
+		return status;
+	}
+
+	tty->driver_data = port;
+
+	return 0;
+}
+
+static void gs_cleanup(struct tty_struct *tty)
+{
+	tty_port_put(tty->port);
+}
+
 static const struct tty_operations gs_tty_ops = {
+	.install =		gs_install,
+	.cleanup =		gs_cleanup,
 	.open =			gs_open,
 	.close =		gs_close,
 	.write =		gs_write,
@@ -1203,6 +1224,15 @@ static void gs_console_exit(struct gs_port *port)
 
 #endif
 
+static void gs_destruct_port(struct tty_port *port)
+{
+	kfree(container_of(port, struct gs_port, port));
+}
+
+static const struct tty_port_operations gs_port_ops = {
+	.destruct = gs_destruct_port,
+};
+
 static int
 gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 {
@@ -1222,9 +1252,9 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 	}
 
 	tty_port_init(&port->port);
+	port->port.ops = &gs_port_ops;
 	spin_lock_init(&port->port_lock);
 	init_waitqueue_head(&port->drain_wait);
-	init_waitqueue_head(&port->close_wait);
 
 	INIT_DELAYED_WORK(&port->push, gs_rx_push);
 
@@ -1241,25 +1271,11 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 	return ret;
 }
 
-static int gs_closed(struct gs_port *port)
-{
-	int cond;
-
-	spin_lock_irq(&port->port_lock);
-	cond = port->port.count == 0;
-	spin_unlock_irq(&port->port_lock);
-
-	return cond;
-}
-
 static void gserial_free_port(struct gs_port *port)
 {
 	cancel_delayed_work_sync(&port->push);
-	/* wait for old opens to finish */
-	wait_event(port->close_wait, gs_closed(port));
 	WARN_ON(port->port_usb != NULL);
-	tty_port_destroy(&port->port);
-	kfree(port);
+	tty_port_put(&port->port);
 }
 
 void gserial_free_line(unsigned char port_num)
@@ -1276,8 +1292,8 @@ void gserial_free_line(unsigned char port_num)
 	ports[port_num].port = NULL;
 	mutex_unlock(&ports[port_num].lock);
 
-	gserial_free_port(port);
 	tty_unregister_device(gs_tty_driver, port_num);
+	gserial_free_port(port);
 }
 EXPORT_SYMBOL_GPL(gserial_free_line);
 
-- 
2.34.1

Re: [PATCH] usb: gadget: u_serial: fix gs_port use-after-free on open/unbind race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: gadget: u_serial: port_num reuse in gserial_free_line() while tty is open
- [Medium] usb: gadget: u_serial: gs_open() succeeds on removed gs_port
- [Medium] usb: gadget: u_serial: kfifo_alloc() failure in gs_open() hits WARN_ON(1)
--

commit fd523a33aec5b372a5000905654db30970d6bfb6
Author: Varun Singh <varun.singh10011@gmail.com>

usb: gadget: u_serial: fix gs_port use-after-free on open/unbind race

This commit switches struct gs_port lifetime management to use the embedded
tty_port kref via tty_operations .install, .cleanup, and tty_port_operations
.destruct callbacks, and removes the close_wait waitqueue in
gserial_free_port().

> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index cdd1dfc666c4f..906137033eca6 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c

[ ... ]

> @@ -610,17 +609,9 @@ static int gserial_wakeup_host(struct gserial *gser)
>   */
>  static int gs_open(struct tty_struct *tty, struct file *file)
>  {
> -	int		port_num = tty->index;
> -	struct gs_port	*port;
> +	struct gs_port	*port = tty->driver_data;
>  	int		status = 0;
>  
> -	mutex_lock(&ports[port_num].lock);
> -	port = ports[port_num].port;
> -	if (!port) {
> -		status = -ENODEV;
> -		goto out;
> -	}
> -
>  	spin_lock_irq(&port->port_lock);

[Severity: Medium]
What happens if gserial_free_line() runs after gs_install() drops
ports[port_num].lock in drivers/usb/gadget/function/u_serial.c, but before
tty_open() calls gs_open() (or if an already-open tty is reopened via
tty_reopen() after gserial_free_line() has cleared ports[port_num].port)?

Previously, gs_open() checked ports[port_num].port under ports[port_num].lock
and returned -ENODEV if the line had been freed. With that check moved only
to gs_install(), gs_install() unlocks ports[port_num].lock before
tty_init_dev() finishes and before tty_open() invokes gs_open():

  tty_open()
    tty_open_by_driver()
      tty_init_dev()
        gs_install()
          mutex_lock(&ports[port_num].lock);
          ...
          mutex_unlock(&ports[port_num].lock);
    gs_open()

If gserial_free_line() runs in that window (or before a tty_reopen()), won't
gs_open() now allocate port->port_write_buf, increment port->port.count, and
return 0 on a gs_port that has already been removed instead of failing with
-ENODEV?

>  	/* allocate circular buffer on first open */
> @@ -629,16 +620,17 @@ static int gs_open(struct tty_struct *tty, struct file *file)
>  		spin_unlock_irq(&port->port_lock);
>  
>  		/*
> -		 * portmaster's mutex still protects from simultaneous open(),
> -		 * and close() can't happen, yet.
> +		 * The TTY core holds tty_lock across ->open(), so this is
> +		 * serialised against a simultaneous open() or close() of
> +		 * the same tty.
>  		 */
>  
>  		status = kfifo_alloc(&port->port_write_buf,
>  				     WRITE_BUF_SIZE, GFP_KERNEL);
>  		if (status) {
>  			pr_debug("gs_open: ttyGS%d (%p,%p) no buffer\n",
> -				 port_num, tty, file);
> -			goto out;
> +				 port->port_num, tty, file);
> +			return status;
>  		}

[Severity: Medium]
If kfifo_alloc() fails here on the first open of a port in gs_open() in
drivers/usb/gadget/function/u_serial.c, won't the subsequent error cleanup in
tty_open() trigger WARN_ON(1) in gs_close()?

Because gs_install() already set tty->driver_data = port, returning an error
from gs_open() before port->port.count is incremented causes tty_open() to
call tty_release(), which invokes gs_close():

  tty_open()
    gs_install()
      tty->driver_data = port;
    gs_open()
      kfifo_alloc() -> fails, port->port.count remains 0
    tty_release()
      gs_close()
        if (port->port.count != 1) {
            if (port->port.count == 0)
                WARN_ON(1);

[ ... ]

> @@ -1241,25 +1271,11 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
>  	return ret;
>  }
>  
> -static int gs_closed(struct gs_port *port)
> -{
> -	int cond;
> -
> -	spin_lock_irq(&port->port_lock);
> -	cond = port->port.count == 0;
> -	spin_unlock_irq(&port->port_lock);
> -
> -	return cond;
> -}
> -
>  static void gserial_free_port(struct gs_port *port)
>  {
>  	cancel_delayed_work_sync(&port->push);
> -	/* wait for old opens to finish */
> -	wait_event(port->close_wait, gs_closed(port));
>  	WARN_ON(port->port_usb != NULL);
> -	tty_port_destroy(&port->port);
> -	kfree(port);
> +	tty_port_put(&port->port);
>  }
>  
>  void gserial_free_line(unsigned char port_num)
> @@ -1276,8 +1292,8 @@ void gserial_free_line(unsigned char port_num)
>  	ports[port_num].port = NULL;
>  	mutex_unlock(&ports[port_num].lock);
>  
> -	gserial_free_port(port);
>  	tty_unregister_device(gs_tty_driver, port_num);
> +	gserial_free_port(port);
>  }

[Severity: High]
Can removing wait_event(port->close_wait, gs_closed(port)) from
gserial_free_port() in drivers/usb/gadget/function/u_serial.c cause a newly
allocated port at the same port_num to attach new opens to the old, orphaned
gs_port?

When a process holds an open file descriptor to /dev/ttyGS0 and the gadget
function is removed via configfs, gserial_free_line() sets
ports[port_num].port = NULL and returns immediately while the old tty_struct
(tty_A) is still open and installed in gs_tty_driver->ttys[port_num] (which
is only cleared by release_tty() after the last file descriptor is closed).

If a new gadget serial function is then created while that old file descriptor
is still held open:

1. gserial_alloc_line() -> gs_port_alloc() checks ports[port_num].port, sees
   NULL, allocates a new gs_port (port_B) at the same port_num, and
   re-registers /dev/ttyGS0.

2. When a process opens the new /dev/ttyGS0, tty_open_by_driver() calls
   tty_driver_lookup_tty(), finds the still-installed tty_A in
   gs_tty_driver->ttys[port_num], and takes the tty_reopen() path instead of
   calling tty_init_dev() -> gs_install().

3. tty_open() then calls gs_open(tty_A, filp), which reads
   port = tty->driver_data (still pointing to the old port_A), while
   gserial_connect() binds the USB endpoints to port_B in
   ports[port_num].port.

Should port_num reuse or gs_tty_driver->ttys[port_num] teardown be prevented
or synchronized while an existing tty_struct for that line is still active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102193436.6044-1-varun.singh10011@gmail.com?part=1