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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: gadget: u_serial: tty_struct UAF in gserial_free_line()
  via vhangup
- [High] usb: gadget: u_serial: premature teardown in gs_close() on
  failed reopen
- [Medium] usb: gadget: u_serial: stale tty in ttys[] blocks open after
  line reuse
--

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

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

This commit switches gs_port lifetime management to the embedded tty_port
kref via tty_operations.install and .cleanup callbacks, removes the
close_wait synchronization in gserial_free_port(), and hangs up the port in
gserial_free_line() when tearing down a serial line.

> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index e172050f13927..f279a2c6507c6 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c

[ ... ]

> @@ -615,8 +614,9 @@ static int gs_open(struct tty_struct *tty, struct file *file)
>  	int		status = 0;
>  
>  	mutex_lock(&ports[port_num].lock);
> +	/* Reject opens on a removed port, or a stale tty after line reuse. */
>  	port = ports[port_num].port;
> -	if (!port) {
> +	if (!port || port != tty->driver_data) {
>  		status = -ENODEV;
>  		goto out;
>  	}

[Severity: Medium]
If a line is freed and reallocated while a process still holds an open file
descriptor from the previous instance, does this check make the newly
registered /dev/ttyGS* device unopenable until the old descriptor is closed?

tty_port_tty_vhangup() marks the old tty_struct as hung up and kills its
line discipline, but it does not remove the tty_struct from
gs_tty_driver->ttys[port_num]; only release_tty() clears that slot when the
last open file descriptor is closed.  When gserial_alloc_line() reuses
port_num in the meantime, any new open() on /dev/ttyGS* finds the old
tty_struct in gs_tty_driver->ttys[port_num] via tty_driver_lookup_tty() and
calls tty_reopen() instead of gs_install().  Because tty->driver_data still
points to the old gs_port while ports[port_num].port points to the new one,
every open() on the new device fails with -ENODEV, and tty_reopen() also
reinitializes an active line discipline on the hung-up tty_struct via
tty_ldisc_reinit() that remains attached until the old file descriptor is
closed.

[ ... ]

> @@ -693,15 +692,14 @@ 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);
>  
>  	if (port->port.count != 1) {
>  raced_with_open:
> -		if (port->port.count == 0)
> -			WARN_ON(1);
> -		else
> +		/* A failed open can still reach ->close() with a zero count. */
> +		if (port->port.count)
> +			--port->port.count;
>  		goto exit;
>  	}

[Severity: High]
Can a failed reopen in gs_open() decrement port->port.count or run final
close teardown in gs_close() while another process still has the port open?

When gs_open() returns -ENODEV before incrementing port->port.count,
tty_open() calls tty_release() -> gs_close().  If another process already
holds an open file descriptor on the same tty, port->port.count is at least
1 when the second opener takes tty_reopen() and fails in gs_open():

CPU0 (process A)                  CPU1 (process B)
tty_open()
  gs_install()
  gs_open()
    port->port.count++ (now 1)
                                  gserial_free_line()
                                    ports[port_num].port = NULL
                                  tty_open()
                                    tty_reopen()
                                    gs_open()
                                      returns -ENODEV
                                    tty_release()
                                      gs_close()
                                        /* port->port.count == 1 */
                                        kfifo_free(&port->port_write_buf)
                                        port->port.count = 0
                                        port->port.tty = NULL
                                  gserial_free_line() continues:
                                    tty_port_tty_vhangup()
                                      /* port->port.tty is NULL, skips */
gs_write() / gs_put_char()
  kfifo_put(&port->port_write_buf, ...)  <-- NULL pointer dereference

If port->port.count was greater than 1, the failed reopen decrements an
active opener's count; if port->port.count was 1, gs_close() frees
port_write_buf and clears port->port.tty while process A's descriptor is
still open, preventing tty_port_tty_vhangup() from hanging up process A's
tty and causing subsequent writes to dereference a NULL kfifo buffer.

[ ... ]

> @@ -1277,6 +1302,13 @@ void gserial_free_line(unsigned char port_num)
>  	tty_unregister_device(gs_tty_driver, port_num);
>  	mutex_unlock(&ports[port_num].lock);
>  
> +	/*
> +	 * Hang up the tty before the line can be reused. Drop
> +	 * ports[port_num].lock first: it takes tty_lock, which gs_install()
> +	 * already holds when it acquires that mutex.
> +	 */
> +	tty_port_tty_vhangup(&port->port);
> +
>  	gserial_free_port(port);
>  }
>  EXPORT_SYMBOL_GPL(gserial_free_line);

[Severity: High]
Can tty_port_tty_vhangup() race with gs_close() and cause a use-after-free
and double free of struct tty_struct?

tty_port_tty_vhangup() calls tty_port_tty_get(), which locks port->port.lock
(struct tty_port::lock) and calls tty_kref_get(port->tty), assuming
port->tty holds a reference via tty_port_tty_set().  However, u_serial never
calls tty_port_tty_set(): gs_open() assigns port->port.tty = tty without
taking a kref, and gs_close() clears port->port.tty = NULL under
port->port_lock (struct gs_port::port_lock) rather than port->port.lock.

When gserial_free_line() races with the final close of /dev/ttyGS*:

CPU0                              CPU1
tty_release()                     gserial_free_line()
  gs_close()                        tty_port_tty_vhangup(&port->port)
    spin_lock_irq(&port->port_lock)   tty_port_tty_get(&port->port)
                                        spin_lock_irqsave(&port->port.lock)
                                        /* reads non-NULL port->port.tty */
    port->port.tty = NULL
    spin_unlock_irq(&port->port_lock)
  tty_release_struct()
    release_tty()
      tty_kref_put(tty) -> 0
                                        tty_kref_get(tty)  <-- refcount 0->1
                                      tty_vhangup(tty)     <-- use-after-free
                                      tty_kref_put(tty)    <-- double free

Using tty_port_tty_set() in gs_open() and gs_close() would maintain the tty
kref and serialize port->port.tty updates under port->port.lock.

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