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

[Syed Tayyab Farooq]

Hi,

syzbot has tested this against the reported issue, and the test passed
successfully.

Best Regards,
Tayyab

On Thu, Sep 10, 2026 at 3:54 PM Syed Tayyab Farooq
<syedtayyabfarooq08@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=fe63e4d633540f230624
> Fixes: c1dca562be8a ("usb gadget: split out serial core")
> Assisted-by: Gemini:gemini-3.7-flash Gemini:gemini-3.1-pro-preview syzbot
> Assisted-by: Claude:sonnet-5-medium
> Signed-off-by: Syed Tayyab Farooq <syedtayyabfarooq08@xxxxxxxxx>
> ---
> v2:
>  - Added Assisted-by tag.
>
>  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
>