Forwarded: [PATCH] usb: gadget: u_serial: fix use-after-free in release_tty

syzbot posted 1 patch 2 weeks, 1 day ago
drivers/usb/gadget/function/u_serial.c | 55 +++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 2 deletions(-)
Forwarded: [PATCH] usb: gadget: u_serial: fix use-after-free in release_tty
Posted by syzbot 2 weeks, 1 day ago
For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] usb: gadget: u_serial: fix use-after-free in release_tty
Author: adrianox@gmail.com

#syz test
gserial_free_line() frees the whole struct gs_port with kfree() once its
own open counter reaches zero. But the tty core keeps tty->port (which is
the embedded struct tty_port) for the lifetime of each tty; a concurrent
struct tty release can still dereference it from release_tty() after the
port has been freed, on a workqueue, racing the configfs teardown:

  release_tty -> tty->port->itty = NULL   (UAF)

Give the tty_port a proper reference count so it is only destroyed once
the last open tty releases it:
  . gs_install holds a tty_port_get() for every opened tty,
  . .cleanup drops it via tty_port_put(),
  . the port's ops->destruct kfree()s the enclosing gs_port, and
  . gserial_free_port just drops the base reference instead of kfree().
---
 drivers/usb/gadget/function/u_serial.c | 55 +++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc666c4..5a78deea2904 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -908,7 +908,53 @@ static int gs_get_icount(struct tty_struct *tty,
 	return 0;
 }
 
+static void gs_port_destruct(struct tty_port *port)
+{
+	struct gs_port	*gs = container_of(port, struct gs_port, port);
+
+	kfree(gs);
+}
+
+static const struct tty_port_operations gs_port_ops = {
+	.destruct =		gs_port_destruct,
+};
+
+/*
+ * The tty core stores tty->port and later dereferences it from release_tty()
+ * on a workqueue, possibly after gserial_free_line() tore the gadget line
+ * down. Keep a reference to the port for the whole life of each tty so the
+ * embedded struct tty_port (and the enclosing gs_port) is only freed once no
+ * tty can reach it.
+ */
+static int gs_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct gs_port	*port;
+	int		status;
+
+	mutex_lock(&ports[tty->index].lock);
+	port = ports[tty->index].port;
+	if (!port) {
+		mutex_unlock(&ports[tty->index].lock);
+		return -ENODEV;
+	}
+	tty_port_get(&port->port);
+	mutex_unlock(&ports[tty->index].lock);
+
+	status = tty_port_install(&port->port, driver, tty);
+	if (status)
+		tty_port_put(&port->port);
+
+	return status;
+}
+
+static void gs_cleanup(struct tty_struct *tty)
+{
+	tty_port_put(tty->port);
+}
+
 static const struct tty_operations gs_tty_ops = {
+	.install =		gs_install,
+	.cleanup =		gs_cleanup,
 	.open =			gs_open,
 	.close =		gs_close,
 	.write =		gs_write,
@@ -1222,6 +1268,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 +1305,12 @@ 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);
+	/*
+	 * Drop the base reference. The tty port is only freed (via
+	 * gs_port_destruct) once the last open tty also released its
+	 * install-time reference in gs_cleanup().
+	 */
+	tty_port_put(&port->port);
 }
 
 void gserial_free_line(unsigned char port_num)
-- 
2.51.0