* [PATCH v2] usb: gadget: u_serial: pin ports while TTYs are installed
[not found] <6941beb7-8cae-4cc5-ba07-e1c8a8753fe1@mail.kernel.org>
@ 2026-10-22 9:13 ` Krystian Kaniewski
2026-10-22 9:37 ` sashiko-bot
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Krystian Kaniewski @ 2026-10-22 9:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, Neill Kapron, linux-usb,
Sebastian Andrzej Siewior
Cc: Mehmet Fide, Brian Ellis, Johan Hovold, Jiri Slaby, linux-serial,
syzkaller-bugs, Ai Chao, Kees Cook, linux-kernel, syzbot,
syzbot+fe63e4d633540f230624, stable
A slab-use-after-free can occur when gserial_free_line() frees struct
gs_port while it is still reachable through an installed TTY. The
port->port.count value tracks successful opens, but it is zero before
gs_open() and again after gs_close(), before TTY cleanup has finished.
Teardown can therefore free gs_port while tty_init_dev() or release_tty()
still accesses its tty_port.
Manage gs_port lifetime with tty_port references. Take a reference in
gs_install(), drop it in gs_cleanup(), and move the final free to
gs_port_destruct(). Unregister the TTY device before dropping the owner
reference.
Keep each minor reserved until the old port is destroyed so an installed
TTY for one generation cannot overlap a replacement at the same minor.
Publish the new port before device registration so the cdev and add uevent
never expose a TTY before gs_install() can pin it. Since commit
8df07fe93573 ("tty: fix saved termios reset race"), u_serial registration
no longer frees saved termios. On registration failure, unpublish the port
before dropping its owner reference.
Check in gs_open() that the installed port is still the published
generation. Track failures after a successful install so close callbacks
issued by the TTY core do not decrement port->port.count for opens that
never incremented it.
Fixes: 19b10a8828a6 ("usb: gadget: allocate & giveback serial ports instead hard code them")
Assisted-by: Gemini:gemini-3.8-flash syzbot
Reported-by: syzbot+fe63e4d633540f230624@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fe63e4d633540f230624
Link: https://syzkaller.appspot.com/ai_job?id=ec789387-2e7b-4a8f-a7a1-a47bf7fa57f5
Cc: stable@kernel.org
Signed-off-by: Krystian Kaniewski <krystianmkaniewski@gmail.com>
---
v2:
- Publish the port before TTY device registration so an open triggered by
the add uevent cannot observe an unpublished slot.
- Drop the saved-termios rationale made obsolete by 8df07fe93573
("tty: fix saved termios reset race").
- Drop unreachable NULL checks in gs_open() and gs_close().
- Rebase to usb-next and mark the fix for stable backporting.
- Do not carry Mehmet's Tested-by tag because the registration ordering
changed after his test.
v1: https://lore.kernel.org/r/6941beb7-8cae-4cc5-ba07-e1c8a8753fe1@mail.kernel.org/
Tested on usb-next in QEMU with KASAN, using the syzbot reproducer and
configuration: without the patch the use-after-free is reported, with it
there are no reports. A test that opens /dev/ttyGS0 while an ACM
function is created gets -ENODEV with v1 and not with v2.
Brian Ellis's "[PATCH v4] usb: gadget: u_serial: fix NULL deref in
gs_close() after failed open" fixes a crash that this patch also removes,
because gs_close() no longer sees a NULL driver_data after a failed open.
The two patches do not apply on top of each other:
https://lore.kernel.org/r/20261013-u_serial-gs-close-null-v4-1-9ea038501994@gotenna.com/
drivers/usb/gadget/function/u_serial.c | 167 ++++++++++++++++++++-----
1 file changed, 134 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc666c4f..41dee2afef5c6 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -70,10 +70,28 @@
* gserial->ioport == usb_ep->driver_data ... gs_port
* gs_port->port_usb ... gserial
*
- * gs_port <---> tty_struct ... links will be null when the TTY file
- * isn't opened; managed by gs_open()/gs_close()
- * gserial->port_tty ... tty_struct
- * tty_struct->driver_data ... gserial
+ * gs_port <---> tty_struct:
+ * tty->driver_data refers to gs_port from install through cleanup,
+ * pinning the port via tty_port reference counting.
+ * gs_port->port.tty exists only while the port is open.
+ *
+ * Each entry in ports[] is guarded by ports[i].lock and models three states:
+ * - Free: !port && !reserved
+ * - Published: port && reserved
+ * - Retiring: !port && reserved
+ *
+ * A minor remains reserved until gs_port_destruct() runs upon final tty_port
+ * release, preventing minor reuse while cleanup of the installed TTY is still
+ * underway. The owner reference is retained through device unregister so the
+ * destructor releases the reservation only after device teardown is complete.
+ *
+ * open_close_failures tracks failed gs_open() calls where port->port.count
+ * was not incremented. The TTY core calls gs_close() on open failure to tear
+ * down the tty; tracking failures as aggregate credits allows gs_close() to
+ * consume a credit and exit without decrementing port->port.count. Credits
+ * are aggregate rather than paired with individual files (another gs_close()
+ * can consume a failed open's credit before that file's own close runs).
+ * It is protected by tty_lock.
*/
/* RX and TX queues can buffer QUEUE_SIZE packets before they hit the
@@ -128,6 +146,7 @@ struct gs_port {
wait_queue_head_t close_wait;
bool suspended; /* port suspended */
bool start_delayed; /* delay start when suspended */
+ unsigned int open_close_failures; /* protected by tty_lock */
struct async_icount icount;
/* REVISIT this state ... */
@@ -135,8 +154,9 @@ struct gs_port {
};
static struct portmaster {
- struct mutex lock; /* protect open/close */
+ struct mutex lock; /* protect open/close and port slot */
struct gs_port *port;
+ bool reserved;
} ports[MAX_U_SERIAL_PORTS];
#define GS_CLOSE_TIMEOUT 15 /* seconds */
@@ -603,22 +623,60 @@ 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);
+ if (!tport) {
+ mutex_unlock(&ports[tty->index].lock);
+ return -ENODEV;
+ }
+
+ ret = tty_port_install(tport, driver, tty);
+ if (ret) {
+ mutex_unlock(&ports[tty->index].lock);
+ tty_port_put(tport);
+ return ret;
+ }
+
+ tty->driver_data = port;
+ mutex_unlock(&ports[tty->index].lock);
+
+ 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
- * know that.
+ * gs_open associates an open TTY with its gs_port.
+ *
+ * There are two TTY-to-port relationships: port->port.tty exists from the
+ * first successful open until final close, while tty->driver_data is assigned
+ * during gs_install() and remains valid through gs_cleanup().
*/
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) {
+ if (ports[port_num].port != port) {
status = -ENODEV;
- goto out;
+ goto fail_unlock;
}
spin_lock_irq(&port->port_lock);
@@ -638,7 +696,7 @@ static int gs_open(struct tty_struct *tty, struct file *file)
if (status) {
pr_debug("gs_open: ttyGS%d (%p,%p) no buffer\n",
port_num, tty, file);
- goto out;
+ goto fail_unlock;
}
spin_lock_irq(&port->port_lock);
@@ -648,7 +706,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,7 +729,11 @@ 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;
+
+fail_unlock:
+ port->open_close_failures++;
mutex_unlock(&ports[port_num].lock);
return status;
}
@@ -695,6 +756,11 @@ static void gs_close(struct tty_struct *tty, struct file *file)
struct gs_port *port = tty->driver_data;
struct gserial *gser;
+ if (port->open_close_failures > 0) {
+ port->open_close_failures--;
+ return;
+ }
+
spin_lock_irq(&port->port_lock);
if (port->port.count != 1) {
@@ -909,8 +975,10 @@ static int gs_get_icount(struct tty_struct *tty,
}
static const struct tty_operations gs_tty_ops = {
+ .install = gs_install,
.open = gs_open,
.close = gs_close,
+ .cleanup = gs_cleanup,
.write = gs_write,
.put_char = gs_put_char,
.flush_chars = gs_flush_chars,
@@ -1203,14 +1271,41 @@ static void gs_console_exit(struct gs_port *port)
#endif
-static int
+static void gs_port_destruct(struct tty_port *tport)
+{
+ struct gs_port *port = container_of(tport, struct gs_port, port);
+ unsigned int port_num = port->port_num;
+
+ mutex_lock(&ports[port_num].lock);
+ if (WARN_ON(ports[port_num].port))
+ ports[port_num].port = NULL;
+ WARN_ON(!ports[port_num].reserved);
+ if (gs_tty_driver)
+ gs_tty_driver->ports[port_num] = NULL;
+ ports[port_num].reserved = false;
+ mutex_unlock(&ports[port_num].lock);
+
+ kfifo_free(&port->port_write_buf);
+ kfree(port);
+}
+
+static const struct tty_port_operations gs_port_ops = {
+ .destruct = gs_port_destruct,
+};
+
+static struct gs_port *
gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
{
struct gs_port *port;
int ret = 0;
mutex_lock(&ports[port_num].lock);
- if (ports[port_num].port) {
+ if (WARN_ON_ONCE(ports[port_num].port && !ports[port_num].reserved)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ if (ports[port_num].reserved) {
ret = -EBUSY;
goto out;
}
@@ -1222,6 +1317,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);
@@ -1235,10 +1331,14 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
port->port_num = port_num;
port->port_line_coding = *coding;
+ ports[port_num].reserved = true;
ports[port_num].port = port;
+ mutex_unlock(&ports[port_num].lock);
+ return port;
+
out:
mutex_unlock(&ports[port_num].lock);
- return ret;
+ return ERR_PTR(ret);
}
static int gs_closed(struct gs_port *port)
@@ -1258,8 +1358,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)
@@ -1276,15 +1375,15 @@ 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);
int gserial_alloc_line_no_console(unsigned char *line_num)
{
struct usb_cdc_line_coding coding;
- struct gs_port *port;
+ struct gs_port *port = ERR_PTR(-EBUSY);
struct device *tty_dev;
int ret;
int port_num;
@@ -1295,19 +1394,20 @@ int gserial_alloc_line_no_console(unsigned char *line_num)
coding.bDataBits = USB_CDC_1_STOP_BITS;
for (port_num = 0; port_num < MAX_U_SERIAL_PORTS; port_num++) {
- ret = gs_port_alloc(port_num, &coding);
- if (ret == -EBUSY)
- continue;
- if (ret)
+ port = gs_port_alloc(port_num, &coding);
+ if (IS_ERR(port)) {
+ ret = PTR_ERR(port);
+ if (ret == -EBUSY)
+ continue;
return ret;
+ }
break;
}
- if (ret)
- return ret;
+ if (IS_ERR(port))
+ return PTR_ERR(port);
/* ... and sysfs class devices, so mdev/udev make /dev/ttyGS* */
- port = ports[port_num].port;
tty_dev = tty_port_register_device(&port->port,
gs_tty_driver, port_num, NULL);
if (IS_ERR(tty_dev)) {
@@ -1316,14 +1416,15 @@ int gserial_alloc_line_no_console(unsigned char *line_num)
ret = PTR_ERR(tty_dev);
mutex_lock(&ports[port_num].lock);
+ WARN_ON(ports[port_num].port != port);
ports[port_num].port = NULL;
mutex_unlock(&ports[port_num].lock);
- gserial_free_port(port);
- goto err;
+ tty_port_put(&port->port);
+ return ret;
}
+
*line_num = port_num;
-err:
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(gserial_alloc_line_no_console);
base-commit: bdf12cde30830d46a8ef1ba93986356f1164b4f1
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread