[PATCH] usb: gadget: u_serial: fix NULL deref in gs_close() after failed open

[PATCH] usb: gadget: u_serial: fix NULL deref in gs_close() after failed open

[Brian Ellis via B4 Relay]

From: Brian Ellis <brianellis@gotenna.com>

gs_close() dereferences tty->driver_data without checking it:

	struct gs_port *port = tty->driver_data;
	struct gserial	*gser;

	spin_lock_irq(&port->port_lock);

but gs_open() assigns tty->driver_data only after two error returns:

	if (!port) {
		status = -ENODEV;
		goto out;
	}
	...
		status = kfifo_alloc(&port->port_write_buf,
				     WRITE_BUF_SIZE, GFP_KERNEL);
		if (status) {
			...
			goto out;
		}
	...
	tty->driver_data = port;

and when ->open() returns an error the tty core calls ->close() anyway:

	if (tty->ops->open)
		retval = tty->ops->open(tty, filp);
	...
	if (retval) {
		tty_debug_hangup(tty, "open error %d, releasing\n", retval);
		tty_unlock(tty); /* need to call tty_release without BTM */
		tty_release(inode, filp);

So opening /dev/ttyGS<n> for a port that has already been freed, or when
the write-buffer allocation fails, oopses in gs_close().

This is not the situation commit ffd603f21423 ("usb: gadget: u_serial: Add
null pointer check in gs_start_io") was reverted for. That check was
redundant because gs_start_io()'s callers are required to hold port_lock
with port.tty and port_usb non-NULL, and the dereference it hid was a race.
Here the NULL is not a race: the tty core deliberately calls ->close() for
an ->open() that failed, as its own debug message says, so driver_data is
legitimately unset on that path and every tty driver has to tolerate it.

Fixes: c1dca562be8a ("usb gadget: split out serial core")
Cc: stable@vger.kernel.org
Signed-off-by: Brian Ellis <brianellis@gotenna.com>
---
Found on a downstream 6.6-adi BSP kernel, then confirmed present in usb-linus
by inspection: gs_close() still dereferences tty->driver_data unguarded, and
gs_open() still assigns it only after its error returns.

Compile-tested against usb-linus with allmodconfig (CONFIG_USB_U_SERIAL=m).
Not boot-tested on mainline: the board it was found on needs a vendor BSP
device tree to boot, so the argument here is from source, with both halves
quoted in the commit message.
---
 drivers/usb/gadget/function/u_serial.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc66..5bbb4d04b 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -695,6 +695,13 @@ static void gs_close(struct tty_struct *tty, struct file *file)
 	struct gs_port *port = tty->driver_data;
 	struct gserial	*gser;
 
+	/*
+	 * tty_open() calls tty_release(), and hence this, when ->open()
+	 * failed before gs_open() had set tty->driver_data.
+	 */
+	if (!port)
+		return;
+
 	spin_lock_irq(&port->port_lock);
 
 	if (port->port.count != 1) {

---
base-commit: be4219dd98608736e13e0b790ef742b76a13254d
change-id: 20260910-u_serial-gs-close-null-a80a436666e8

Best regards,
-- 
Brian Ellis <brianellis@gotenna.com>