usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

Commit Message

胡连勤 Nov. 18, 2024, 3:12 a.m. UTC

From: Lianqin Hu <[e-mail]>

Considering that in some extreme cases,
when u_serial driver is accessed by multiple threads,
Thread A is executing the open operation and calling the gs_open,
Thread B is executing the disconnect operation and calling the
gserial_disconnect function,The port->port_usb pointer will be set to NULL.

E.g.
    Thread A                                 Thread B

    gs_open()                                gadget_unbind_driver()

    gs_start_io()                            composite_disconnect()

    gs_start_rx()                            gserial_disconnect()
    ...                                      ...
    spin_unlock(&port->port_lock)
    status = usb_ep_queue()                  spin_lock(&port->port_lock)
    spin_lock(&port->port_lock)              port->port_usb = NULL
    gs_free_requests(port->port_usb->in)     spin_unlock(&port->port_lock)
    Crash

This causes thread A to access a null pointer (port->port_usb is null)
when calling the gs_free_requests function, causing a crash.

To solve this problem, add the read_busy flag, before setting port_usb
to null in gserial_disconnect, add the read_busy flag judgment.

Unable to handle kernel NULL pointer dereference at
virtual address 00000000000000e8
pc : gs_start_io+0x164/0x25c
lr : gs_start_io+0x238/0x25c
sp : ffffffc08b75ba00
x29: ffffffc08b75ba00 x28: ffffffed8ba01000 x27: 0000000000020902
x26: dead000000000100 x25: ffffff899f43a400 x24: ffffff8862325400
x23: ffffff88623256a4 x22: ffffff8862325690 x21: ffffff88623255ec
x20: ffffff88623255d8 x19: ffffff885e19d700 x18: ffffffed8c45ae40
x17: 00000000d48d30ad x16: 00000000d48d30ad x15: 0010000000000001
x14: ffffffed8c50fcc0 x13: 0000000040000000 x12: 0000000000000001
x11: 0000000080200012 x10: 0000000080200012 x9 : ffffff88623255d8
x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
x5 : ffffffed8ae0b9a4 x4 : fffffffe267d0ea0 x3 : 0000000080200012
x2 : ffffff899f43a400 x1 : 0000000080200013 x0 : ffffff899f43b100
Call trace:
 gs_start_io+0x164/0x25c
 gs_open+0x108/0x13c
 tty_open+0x314/0x638
 chrdev_open+0x1b8/0x258
 do_dentry_open+0x2c4/0x700
 vfs_open+0x2c/0x3c
 path_openat+0xa64/0xc60
 do_filp_open+0xb8/0x164
 do_sys_openat2+0x84/0xf0
 __arm64_sys_openat+0x70/0x9c
 invoke_syscall+0x58/0x114
 el0_svc_common+0x80/0xe0
 do_el0_svc+0x1c/0x28
 el0_svc+0x38/0x68
 el0t_64_sync_handler+0x68/0xbc
 el0t_64_sync+0x1a8/0x1ac
Code: f2fbd5ba eb14013f 540004a1 f940e708 (f9407513)
---[ end trace 0000000000000000 ]---

Signed-off-by: Lianqin Hu <[e-mail]>
---
 drivers/usb/gadget/function/u_serial.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Patch

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 0a8c05b2746b..9ab2dbed60a8 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -124,6 +124,7 @@  struct gs_port {
 	struct kfifo		port_write_buf;
 	wait_queue_head_t	drain_wait;	/* wait while writes drain */
 	bool                    write_busy;
+	bool                    read_busy;
 	wait_queue_head_t	close_wait;
 	bool			suspended;	/* port suspended */
 	bool			start_delayed;	/* delay start when suspended */
@@ -331,9 +332,11 @@  __acquires(&port->port_lock)
 		/* drop lock while we call out; the controller driver
 		 * may need to call us back (e.g. for disconnect)
 		 */
+		port->read_busy = true;
 		spin_unlock(&port->port_lock);
 		status = usb_ep_queue(out, req, GFP_ATOMIC);
 		spin_lock(&port->port_lock);
+		port->read_busy = false;
 
 		if (status) {
 			pr_debug("%s: %s %s err %d\n",
@@ -1412,19 +1415,21 @@  void gserial_disconnect(struct gserial *gser)
 	/* tell the TTY glue not to do I/O here any more */
 	spin_lock(&port->port_lock);
 
-	gs_console_disconnect(port);
+	if (!port->read_busy) {
+		gs_console_disconnect(port);
 
-	/* REVISIT as above: how best to track this? */
-	port->port_line_coding = gser->port_line_coding;
+		/* REVISIT as above: how best to track this? */
+		port->port_line_coding = gser->port_line_coding;
 
-	port->port_usb = NULL;
-	gser->ioport = NULL;
-	if (port->port.count > 0) {
-		wake_up_interruptible(&port->drain_wait);
-		if (port->port.tty)
-			tty_hangup(port->port.tty);
+		port->port_usb = NULL;
+		gser->ioport = NULL;
+		if (port->port.count > 0) {
+			wake_up_interruptible(&port->drain_wait);
+			if (port->port.tty)
+				tty_hangup(port->port.tty);
+		}
+		port->suspended = false;
 	}
-	port->suspended = false;
 	spin_unlock(&port->port_lock);
 	spin_unlock_irqrestore(&serial_port_lock, flags);