[PATCH v2 0/2] usb: gadget: u_serial: flush_buffer, and keep the port for its tty

[PATCH v2 0/2] usb: gadget: u_serial: flush_buffer, and keep the port for its tty

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

On a gadget whose host side does not read the port, the last close of
/dev/ttyGS* waits GS_CLOSE_TIMEOUT (15 s) for the write buffer to drain,
and tcflush(TCOFLUSH) cannot avoid that because u_serial has no
flush_buffer operation. v1 added it.

Sashiko pointed out that the hangup gserial_disconnect() schedules can
then call flush_buffer after gserial_free_line() has freed the port.
That window is older than the new callback: the tty core already reads
the freed port there, KASAN reports it in check_tty_count() with a
delay added to gs_close(). Patch 1 makes the port live as long as its
tty, with a tty_port reference taken in install and dropped in cleanup
as cdc-acm does, and carries Fixes and Cc stable. Patch 2 is v1.

Changes in v2:
- new patch 1, the port lifetime fix
- patch 2 unchanged

v1: https://lore.kernel.org/linux-usb/20261015162732.3170938-1-mehmet.fide@gmail.com/

Tested on a Verdin AM62 with KASAN and lockdep, 6.12.113 plus both
patches: with a 3 s delay in gs_close() the hangup after
gserial_free_line() no longer reports a use-after-free and the port is
freed after the tty; four function teardowns, two with the port open,
free four ports without leaks or warnings; close after tcflush returns
at once with the host not reading and the data written after the flush
arrives; 47850 flushes against a concurrent writer; NCM torn down
under iperf traffic. The same two patches run in our i.MX8MP and VF50
product images on CI rigs, gadget replug included. On usb-next both
patches build with W=1 and sparse without new warnings, each on its own.

Mehmet Fide (2):
  usb: gadget: u_serial: keep the port until its tty is released
  usb: gadget: u_serial: implement flush_buffer

 drivers/usb/gadget/function/u_serial.c | 59 +++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 2 deletions(-)


base-commit: 04ac7b445880b0878c83e6e89d961455cbea0427
-- 
2.55.0

[PATCH v2 1/2] usb: gadget: u_serial: keep the port until its tty is released

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

gserial_free_line() frees the gs_port, and with it the tty_port it
embeds, as soon as the last close has returned. The tty core keeps
using tty->port until release_tty(), so a hangup that
gserial_disconnect() scheduled before that close can still run and
read the freed port:

  BUG: KASAN: slab-use-after-free in check_tty_count+0x230/0x278
  Workqueue: events do_tty_hangup
   __tty_hangup.part.0
   do_tty_hangup
  Allocated by: gserial_alloc_line_no_console <- acm_alloc_instance
  Freed by: gserial_free_port <- gserial_free_line <- acm_free_instance

and release_tty() then cancels the buffer work of the freed port.

Take a reference on the tty_port in install and drop it in cleanup,
and let the last reference free the gs_port.

Fixes: 19b10a8828a6 ("usb: gadget: allocate & giveback serial ports instead hard code them")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---

Notes:
    Changes in v2:
    - new patch: Sashiko pointed out that with flush_buffer the hangup that
      gserial_disconnect() schedules can run after gserial_free_line() has
      freed the port; the tty core itself already reads the freed port in that
      window (check_tty_count, release_tty), so the port has to live as long
      as its tty

 drivers/usb/gadget/function/u_serial.c | 45 ++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc666c4..19eea724280b 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -908,7 +908,39 @@ 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		ret;
+
+	mutex_lock(&ports[port_num].lock);
+	port = ports[port_num].port;
+	if (port)
+		tty_port_get(&port->port);
+	mutex_unlock(&ports[port_num].lock);
+	if (!port)
+		return -ENODEV;
+
+	ret = tty_port_install(&port->port, driver, tty);
+	if (ret) {
+		tty_port_put(&port->port);
+		return ret;
+	}
+	tty->driver_data = port;
+	return 0;
+}
+
+static void gs_cleanup(struct tty_struct *tty)
+{
+	struct gs_port	*port = tty->driver_data;
+
+	tty_port_put(&port->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 +1235,15 @@ static void gs_console_exit(struct gs_port *port)
 
 #endif
 
+static void gs_port_destruct(struct tty_port *tport)
+{
+	kfree(container_of(tport, struct gs_port, 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 +1263,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 +1300,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.55.0

Re: [PATCH v2 1/2] usb: gadget: u_serial: keep the port until its tty is released

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: gadget: u_serial: port reallocation race between gs_install() and gs_open()
- [High] usb: gadget: u_serial: unbalanced port count in gs_close() when gs_open() fails
--

commit 80163fbde5c8a63d71508c20f74ff07e66b93936
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

usb: gadget: u_serial: keep the port until its tty is released

This commit adds install and cleanup callbacks to gs_tty_ops to take a
reference on tty_port during tty installation and release the gs_port via
a tty_port destructor when the last reference is dropped.

> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index cdd1dfc666c4f..19eea724280b0 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -908,7 +908,39 @@ 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		ret;
> +
> +	mutex_lock(&ports[port_num].lock);
> +	port = ports[port_num].port;
> +	if (port)
> +		tty_port_get(&port->port);
> +	mutex_unlock(&ports[port_num].lock);
> +	if (!port)
> +		return -ENODEV;
> +
> +	ret = tty_port_install(&port->port, driver, tty);
> +	if (ret) {
> +		tty_port_put(&port->port);
> +		return ret;
> +	}
> +	tty->driver_data = port;
> +	return 0;
> +}

[Severity: High]
Does setting tty->driver_data in gs_install() cause an unbalanced
port->port.count decrement in gs_close() when gs_open() fails?

When gs_open() returns a non-zero error, tty_open() cleans up by calling
tty_release() -> gs_close(). In gs_open(), the function can fail and return
-ENODEV or -ENOMEM before incrementing port->port.count:

drivers/usb/gadget/function/u_serial.c:gs_open() {
	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 */
	if (!kfifo_initialized(&port->port_write_buf)) {
		spin_unlock_irq(&port->port_lock);
		status = kfifo_alloc(&port->port_write_buf,
				     WRITE_BUF_SIZE, GFP_KERNEL);
		if (status) {
			...
			goto out;
		}
		spin_lock_irq(&port->port_lock);
	}

	/* already open?  Great. */
	if (port->port.count++)
		goto exit_unlock_port;
}

Because gs_install() already set tty->driver_data = port, gs_close() runs
with a valid port pointer even though gs_open() never incremented
port->port.count:

drivers/usb/gadget/function/u_serial.c:gs_close() {
	struct gs_port *port = tty->driver_data;
	...
	spin_lock_irq(&port->port_lock);

	if (port->port.count != 1) {
raced_with_open:
		if (port->port.count == 0)
			WARN_ON(1);
		else
			--port->port.count;
		goto exit;
	}
}

On a failed first open where port->port.count is 0, gs_close() hits
WARN_ON(1).

Furthermore, if a process already holds the tty open (port->port.count == 1)
and gserial_free_line() has cleared ports[port_num].port = NULL while
waiting in gserial_free_port() at wait_event(port->close_wait,
gs_closed(port)), a second process reopening the existing tty via
tty_reopen() -> gs_open() sees ports[port_num].port == NULL and returns
-ENODEV. The resulting tty_release() -> gs_close() sees port->port.count == 1
from the first opener and runs final-close teardown on the active port,
resetting port->port_write_buf, clearing port->port.tty = NULL, setting
port->port.count = 0, and waking port->close_wait while the first opener
still has the tty open.

> +
> +static void gs_cleanup(struct tty_struct *tty)
> +{
> +	struct gs_port	*port = tty->driver_data;
> +
> +	tty_port_put(&port->port);
> +}

[Severity: High]
Can a port free and reallocation between gs_install() and gs_open() cause
gs_cleanup() to free a newly allocated struct gs_port while leaking the
originally installed struct gs_port?

On first open, tty_open() -> tty_open_by_driver() -> tty_init_dev() calls
gs_install(), which locks ports[port_num].lock, takes a reference on the
current port (port1), unlocks ports[port_num].lock, binds tty->port to
&port1->port, and sets tty->driver_data = port1.

Before tty_open() invokes gs_open(), neither ports[port_num].lock nor
tty_mutex is held, and port1->port.count is still 0. If gserial_free_line()
runs in this window, wait_event(port->close_wait, gs_closed(port)) in
gserial_free_port() returns immediately because port1->port.count is 0, and
gserial_free_port() drops port1's initial reference. If the line is then
reallocated via gs_port_alloc(), a new struct gs_port (port2) is stored in
ports[port_num].port with kref == 1.

When gs_open() then executes, it re-reads ports[port_num].port instead of
using tty->driver_data:

drivers/usb/gadget/function/u_serial.c:gs_open() {
	mutex_lock(&ports[port_num].lock);
	port = ports[port_num].port;
	...
	if (port->port.count++)
		goto exit_unlock_port;

	tty->driver_data = port;
	port->port.tty = tty;
}

This overwrites tty->driver_data with port2 without taking a tty_port
reference on &port2->port, while tty->port remains &port1->port (and
port2->port.itty remains NULL). When the tty is later released, gs_cleanup()
calls tty_port_put(&port->port) on tty->driver_data (port2), dropping
port2's sole reference and freeing port2 via gs_port_destruct() while
ports[port_num].port still points to it, while port1's reference from
gs_install() is never dropped.

[ ... ]

> @@ -1258,8 +1300,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)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019085855.3259716-1-mehmet.fide@gmail.com?part=1

[PATCH v2 2/2] usb: gadget: u_serial: implement flush_buffer

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

gs_close() waits up to GS_CLOSE_TIMEOUT (15 s) for the write buffer
to drain while the gadget is connected. When the host does not read
the port, for example because no application on the host has it open,
the last close blocks for the full timeout. tcflush(TCOFLUSH) cannot
avoid that: u_serial has no flush_buffer operation, so the request is
a no-op.

Implement flush_buffer: discard the buffered output under the port
lock and wake up the close and write waiters. As on other tty drivers,
a hangup and an interrupt character (ISIG without NOFLSH) now discard
the pending output as well.

Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---

Notes:
    Changes in v2:
    - none, now after the patch that keeps the port alive for the hangup

 drivers/usb/gadget/function/u_serial.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 19eea724280b..187a2598181a 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -822,6 +822,19 @@ static void gs_flush_chars(struct tty_struct *tty)
 	spin_unlock_irqrestore(&port->port_lock, flags);
 }
 
+static void gs_flush_buffer(struct tty_struct *tty)
+{
+	struct gs_port	*port = tty->driver_data;
+	unsigned long	flags;
+
+	spin_lock_irqsave(&port->port_lock, flags);
+	kfifo_reset(&port->port_write_buf);
+	spin_unlock_irqrestore(&port->port_lock, flags);
+
+	wake_up_interruptible(&port->drain_wait);
+	tty_wakeup(tty);
+}
+
 static unsigned int gs_write_room(struct tty_struct *tty)
 {
 	struct gs_port	*port = tty->driver_data;
@@ -946,6 +959,7 @@ static const struct tty_operations gs_tty_ops = {
 	.write =		gs_write,
 	.put_char =		gs_put_char,
 	.flush_chars =		gs_flush_chars,
+	.flush_buffer =		gs_flush_buffer,
 	.write_room =		gs_write_room,
 	.chars_in_buffer =	gs_chars_in_buffer,
 	.unthrottle =		gs_unthrottle,
-- 
2.55.0

Re: [PATCH v2 2/2] usb: gadget: u_serial: implement flush_buffer

[sashiko-bot]

> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> gs_close() waits up to GS_CLOSE_TIMEOUT (15 s) for the write buffer
> to drain while the gadget is connected. When the host does not read
> the port, for example because no application on the host has it open,
> the last close blocks for the full timeout. tcflush(TCOFLUSH) cannot
> avoid that: u_serial has no flush_buffer operation, so the request is
> a no-op.
> 
> Implement flush_buffer: discard the buffered output under the port
> lock and wake up the close and write waiters. As on other tty drivers,
> a hangup and an interrupt character (ISIG without NOFLSH) now discard
> the pending output as well.
> 
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019085855.3259716-1-mehmet.fide@gmail.com?part=2