[PATCH] usb: gadget: u_serial: fix use-after-free between tty open/close and gserial_free_line

[PATCH] usb: gadget: u_serial: fix use-after-free between tty open/close and gserial_free_line

[Syed Tayyab Farooq]

syzbot reports a slab-use-after-free in tty_init_dev()/gs_close()
involving struct gs_port. The port is allocated when a gadget serial
function instance is created via configfs mkdir (gserial_alloc_line())
and freed via a plain kfree() in gserial_free_port() when the instance
is removed via configfs rmdir(). Nothing prevented a concurrent
open("/dev/ttyGS*") from racing with rmdir: the tty core could still
reach gs_open()/gs_close() and dereference the gs_port after it had
already been freed, since the ports[] table entry these functions
looked up was a bare pointer with no refcounting tied to the tty core.

Fix this by giving struct gs_port proper tty_port-managed lifetime:

 - Add gs_install()/gs_cleanup() tty_operations. gs_install() looks up
   the port once under ports[idx].lock, takes a tty_port_get()
   reference, and stores the result in tty->driver_data. gs_cleanup()
   drops that reference when the tty_struct is released. This ties the
   gs_port's minimum lifetime to the tty_struct using it, so it can no
   longer be freed out from under an open tty.

 - Give tty_port a destructor (gs_port_destruct()) that does the
   kfree() that gserial_free_port() used to do directly, and
   switch gserial_free_port() to tty_port_put() instead of an
   unconditional kfree(). Also, tty_port_destroy is automatically called
   when the reference reaches 0. The struct is now only actually freed once
   its last reference (held by either the ports[] table or a live tty)
   is dropped.

 - Stop gs_open() from independently re-deriving the port from
   ports[port_num].port and reassigning tty->driver_data. gs_install()
   is now the single place that looks up and pins the port; gs_open()
   re-deriving it separately could, on an unlucky race with the port
   index being freed and reallocated, leave tty->port and
   tty->driver_data pointing at two different gs_port instances, with
   the one gs_close() uses left unprotected. gs_open() now just uses
   the already-validated tty->driver_data, while still holding
   ports[port_num].lock across the first-open kfifo allocation to
   serialize concurrent first opens against each other (kfifo_alloc()
   needs GFP_KERNEL, so it can't run under port_lock).

 - In gs_close(), a tty's .close() can legitimately run against a port
   whose port.count is still 0, e.g. when gs_open() fails and the tty
   core unwinds via tty_release(). The previous code treated this as
   an impossible state (WARN_ON(1)), which trips panic_on_warn on
   syzbot and isn't actually a bug -- it's an expected outcome of a
   failed open. Treat count == 0 the same as any other "not the last
   closer" case and return quietly instead of warning.

Reported-by: syzbot+fe63e4d633540f230624@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fe63e4d633540f230624
Fixes: c1dca562be8a ("usb gadget: split out serial core")
Signed-off-by: Syed Tayyab Farooq <syedtayyabfarooq08@gmail.com>
---
 drivers/usb/gadget/function/u_serial.c | 64 +++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc666c4..9da860589861 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -603,6 +603,41 @@ static int gserial_wakeup_host(struct gserial *gser)
 
 /* TTY Driver */
 
+static int gs_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct gs_port *port;
+	struct tty_port *tport;
+	int ret;
+
+	mutex_lock(&ports[tty->index].lock);
+	port = ports[tty->index].port;
+	if (!port) {
+		mutex_unlock(&ports[tty->index].lock);
+		return -ENODEV;
+	}
+
+	tport = tty_port_get(&port->port);
+	mutex_unlock(&ports[tty->index].lock);
+
+	if (!tport)
+		return -ENODEV;
+
+	ret = tty_port_install(tport, driver, tty);
+	if (ret) {
+		tty_port_put(tport);
+		return ret;
+	}
+
+	tty->driver_data = port;
+
+	return 0;
+}
+
+static void gs_cleanup(struct tty_struct *tty)
+{
+	tty_port_put(tty->port);
+}
+
 /*
  * gs_open sets up the link between a gs_port and its associated TTY.
  * That link is broken *only* by TTY close(), and all driver methods
@@ -615,7 +650,7 @@ static int gs_open(struct tty_struct *tty, struct file *file)
 	int		status = 0;
 
 	mutex_lock(&ports[port_num].lock);
-	port = ports[port_num].port;
+	port = tty->driver_data;
 	if (!port) {
 		status = -ENODEV;
 		goto out;
@@ -648,7 +683,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 */
@@ -695,14 +729,16 @@ static void gs_close(struct tty_struct *tty, struct file *file)
 	struct gs_port *port = tty->driver_data;
 	struct gserial	*gser;
 
+	if (!port)
+		return;
+
 	spin_lock_irq(&port->port_lock);
 
 	if (port->port.count != 1) {
 raced_with_open:
-		if (port->port.count == 0)
-			WARN_ON(1);
-		else
+		if (port->port.count > 0)
 			--port->port.count;
+
 		goto exit;
 	}
 
@@ -911,6 +947,8 @@ static int gs_get_icount(struct tty_struct *tty,
 static const struct tty_operations gs_tty_ops = {
 	.open =			gs_open,
 	.close =		gs_close,
+	.install =		gs_install,
+	.cleanup =		gs_cleanup,
 	.write =		gs_write,
 	.put_char =		gs_put_char,
 	.flush_chars =		gs_flush_chars,
@@ -1203,6 +1241,18 @@ static void gs_console_exit(struct gs_port *port)
 
 #endif
 
+static void gs_port_destruct(struct tty_port *tport)
+{
+	struct gs_port *port = container_of(tport, struct gs_port, port);
+
+	kfree(port);
+}
+
+static const struct tty_port_operations gs_port_ops = {
+	.destruct = gs_port_destruct,
+};
+
+
 static int
 gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 {
@@ -1222,6 +1272,7 @@ 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);
@@ -1258,8 +1309,7 @@ static void gserial_free_port(struct gs_port *port)
 	/* 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)
-- 
2.43.0

Re: [PATCH] usb: gadget: u_serial: fix use-after-free between tty open/close and gserial_free_line

[Greg Kroah-Hartman]

On Thu, Sep 10, 2026 at 01:17:19PM +0500, Syed Tayyab Farooq wrote:
> syzbot reports a slab-use-after-free in tty_init_dev()/gs_close()
> involving struct gs_port. The port is allocated when a gadget serial
> function instance is created via configfs mkdir (gserial_alloc_line())
> and freed via a plain kfree() in gserial_free_port() when the instance
> is removed via configfs rmdir(). Nothing prevented a concurrent
> open("/dev/ttyGS*") from racing with rmdir: the tty core could still
> reach gs_open()/gs_close() and dereference the gs_port after it had
> already been freed, since the ports[] table entry these functions
> looked up was a bare pointer with no refcounting tied to the tty core.
> 
> Fix this by giving struct gs_port proper tty_port-managed lifetime:
> 
>  - Add gs_install()/gs_cleanup() tty_operations. gs_install() looks up
>    the port once under ports[idx].lock, takes a tty_port_get()
>    reference, and stores the result in tty->driver_data. gs_cleanup()
>    drops that reference when the tty_struct is released. This ties the
>    gs_port's minimum lifetime to the tty_struct using it, so it can no
>    longer be freed out from under an open tty.
> 
>  - Give tty_port a destructor (gs_port_destruct()) that does the
>    kfree() that gserial_free_port() used to do directly, and
>    switch gserial_free_port() to tty_port_put() instead of an
>    unconditional kfree(). Also, tty_port_destroy is automatically called
>    when the reference reaches 0. The struct is now only actually freed once
>    its last reference (held by either the ports[] table or a live tty)
>    is dropped.
> 
>  - Stop gs_open() from independently re-deriving the port from
>    ports[port_num].port and reassigning tty->driver_data. gs_install()
>    is now the single place that looks up and pins the port; gs_open()
>    re-deriving it separately could, on an unlucky race with the port
>    index being freed and reallocated, leave tty->port and
>    tty->driver_data pointing at two different gs_port instances, with
>    the one gs_close() uses left unprotected. gs_open() now just uses
>    the already-validated tty->driver_data, while still holding
>    ports[port_num].lock across the first-open kfifo allocation to
>    serialize concurrent first opens against each other (kfifo_alloc()
>    needs GFP_KERNEL, so it can't run under port_lock).
> 
>  - In gs_close(), a tty's .close() can legitimately run against a port
>    whose port.count is still 0, e.g. when gs_open() fails and the tty
>    core unwinds via tty_release(). The previous code treated this as
>    an impossible state (WARN_ON(1)), which trips panic_on_warn on
>    syzbot and isn't actually a bug -- it's an expected outcome of a
>    failed open. Treat count == 0 the same as any other "not the last
>    closer" case and return quietly instead of warning.
> 
> Reported-by: syzbot+fe63e4d633540f230624@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fe63e4d633540f230624
> Fixes: c1dca562be8a ("usb gadget: split out serial core")
> Signed-off-by: Syed Tayyab Farooq <syedtayyabfarooq08@gmail.com>
> ---
>  drivers/usb/gadget/function/u_serial.c | 64 +++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 7 deletions(-)

Did you forget an Assisted-by: tag?  And did syzbot test this?

thanks,

greg k-h

Re: [PATCH] usb: gadget: u_serial: fix use-after-free between tty open/close and gserial_free_line

[Syed Tayyab Farooq]

Thanks for catching that. Yes to both. I have sent a Patch v2 with the
added assisted-by tag.

Best Regards,
Tayyab

On Thu, Sep 10, 2026 at 1:38 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 10, 2026 at 01:17:19PM +0500, Syed Tayyab Farooq wrote:
> > syzbot reports a slab-use-after-free in tty_init_dev()/gs_close()
> > involving struct gs_port. The port is allocated when a gadget serial
> > function instance is created via configfs mkdir (gserial_alloc_line())
> > and freed via a plain kfree() in gserial_free_port() when the instance
> > is removed via configfs rmdir(). Nothing prevented a concurrent
> > open("/dev/ttyGS*") from racing with rmdir: the tty core could still
> > reach gs_open()/gs_close() and dereference the gs_port after it had
> > already been freed, since the ports[] table entry these functions
> > looked up was a bare pointer with no refcounting tied to the tty core.
> >
> > Fix this by giving struct gs_port proper tty_port-managed lifetime:
> >
> >  - Add gs_install()/gs_cleanup() tty_operations. gs_install() looks up
> >    the port once under ports[idx].lock, takes a tty_port_get()
> >    reference, and stores the result in tty->driver_data. gs_cleanup()
> >    drops that reference when the tty_struct is released. This ties the
> >    gs_port's minimum lifetime to the tty_struct using it, so it can no
> >    longer be freed out from under an open tty.
> >
> >  - Give tty_port a destructor (gs_port_destruct()) that does the
> >    kfree() that gserial_free_port() used to do directly, and
> >    switch gserial_free_port() to tty_port_put() instead of an
> >    unconditional kfree(). Also, tty_port_destroy is automatically called
> >    when the reference reaches 0. The struct is now only actually freed once
> >    its last reference (held by either the ports[] table or a live tty)
> >    is dropped.
> >
> >  - Stop gs_open() from independently re-deriving the port from
> >    ports[port_num].port and reassigning tty->driver_data. gs_install()
> >    is now the single place that looks up and pins the port; gs_open()
> >    re-deriving it separately could, on an unlucky race with the port
> >    index being freed and reallocated, leave tty->port and
> >    tty->driver_data pointing at two different gs_port instances, with
> >    the one gs_close() uses left unprotected. gs_open() now just uses
> >    the already-validated tty->driver_data, while still holding
> >    ports[port_num].lock across the first-open kfifo allocation to
> >    serialize concurrent first opens against each other (kfifo_alloc()
> >    needs GFP_KERNEL, so it can't run under port_lock).
> >
> >  - In gs_close(), a tty's .close() can legitimately run against a port
> >    whose port.count is still 0, e.g. when gs_open() fails and the tty
> >    core unwinds via tty_release(). The previous code treated this as
> >    an impossible state (WARN_ON(1)), which trips panic_on_warn on
> >    syzbot and isn't actually a bug -- it's an expected outcome of a
> >    failed open. Treat count == 0 the same as any other "not the last
> >    closer" case and return quietly instead of warning.
> >
> > Reported-by: syzbot+fe63e4d633540f230624@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=fe63e4d633540f230624
> > Fixes: c1dca562be8a ("usb gadget: split out serial core")
> > Signed-off-by: Syed Tayyab Farooq <syedtayyabfarooq08@gmail.com>
> > ---
> >  drivers/usb/gadget/function/u_serial.c | 64 +++++++++++++++++++++++---
> >  1 file changed, 57 insertions(+), 7 deletions(-)
>
> Did you forget an Assisted-by: tag?  And did syzbot test this?
>
> thanks,
>
> greg k-h