| Message ID | TYUPR06MB62178D00DC96CC2702114CF5D2222@TYUPR06MB6217.apcprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer | expand |
On 21-11-24 06:11 pm, 胡连勤 wrote: > From: Lianqin Hu <hulianqin@vivo.com> > > 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() > You can remove these extra lines. > 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 avoid this, add a null pointer check to gs_start_io before attempting > to access the value of the pointer port->port_usb. > > 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 These address lists can also omitted. And the below stack can be shortened. > 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 ]--- > > Suggested-by: Prashanth K <quic_prashk@quicinc.com> > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> Include fixes tag and CC stable kernel. > > v2: > - Modify patch content and description according to "v1 suggestion" > - Link to v1: https://lore.kernel.org/all/TYUPR06MB621737D16F68B5ABD6CF5772D2272@TYUPR06MB6217.apcprd06.prod.outlook.com/ > > drivers/usb/gadget/function/u_serial.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index 0a8c05b2746b..53d9fc41acc5 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port) > * we didn't in gs_start_tx() */ > tty_wakeup(port->port.tty); > } else { > - gs_free_requests(ep, head, &port->read_allocated); > - gs_free_requests(port->port_usb->in, &port->write_pool, > - &port->write_allocated); > + /* Free reqs only if we are still connected */ > + if (port->port_usb) { > + gs_free_requests(ep, head, &port->read_allocated); > + gs_free_requests(port->port_usb->in, &port->write_pool, > + &port->write_allocated); You can also mention in commit text that skip freeing reqs if port_usb is null, since it will be done by gser_disconnect. Maybe you can wait for other people to review before sending next version. > + } > status = -EIO; > } > Regards, Prashanth K
Hi Prashanth K: > > 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() > > > You can remove these extra lines. > > 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 avoid this, add a null pointer check to gs_start_io before > > attempting to access the value of the pointer port->port_usb. > > > > 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 > > These address lists can also omitted. And the below stack can be shortened. > > > 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 ]--- > > > > Suggested-by: Prashanth K <quic_prashk@quicinc.com> > > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> > > Include fixes tag and CC stable kernel. > > > > v2: > > - Modify patch content and description according to "v1 suggestion" > > - Link to v1: > > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Fall%2FTYUPR06MB621737D16F68B5ABD6CF5772D2272%40TY > UPR06MB > > > 6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40vivo.c > om% > > > 7Cd69c2ad2d46d4726186c08dd0acef565%7C923e42dc48d54cbeb5821a797a64 > 12ed% > > > 7C0%7C0%7C638678606223680078%7CUnknown%7CTWFpbGZsb3d8eyJFbXB > 0eU1hcGkiO > > > nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf > Q% > > > 3D%3D%7C0%7C%7C%7C&sdata=MhFYbqrjUByq%2Fnr4bSYyT4OyXfccCRRoa > c66zng1G5I > > %3D&reserved=0 > > > > drivers/usb/gadget/function/u_serial.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/u_serial.c > > b/drivers/usb/gadget/function/u_serial.c > > index 0a8c05b2746b..53d9fc41acc5 100644 > > --- a/drivers/usb/gadget/function/u_serial.c > > +++ b/drivers/usb/gadget/function/u_serial.c > > @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port) > > * we didn't in gs_start_tx() */ > > tty_wakeup(port->port.tty); > > } else { > > - gs_free_requests(ep, head, &port->read_allocated); > > - gs_free_requests(port->port_usb->in, &port->write_pool, > > - &port->write_allocated); > > + /* Free reqs only if we are still connected */ > > + if (port->port_usb) { > > + gs_free_requests(ep, head, &port->read_allocated); > > + gs_free_requests(port->port_usb->in, &port- > >write_pool, > > + &port->write_allocated); > You can also mention in commit text that skip freeing reqs if port_usb is null, > since it will be done by gser_disconnect. Maybe you can wait for other > people to review before sending next version. OK, I'll wait for other people's review comments and send a new version later. > > + } > > status = -EIO; > > } > > Thanks
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 0a8c05b2746b..53d9fc41acc5 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port) * we didn't in gs_start_tx() */ tty_wakeup(port->port.tty); } else { - gs_free_requests(ep, head, &port->read_allocated); - gs_free_requests(port->port_usb->in, &port->write_pool, - &port->write_allocated); + /* Free reqs only if we are still connected */ + if (port->port_usb) { + gs_free_requests(ep, head, &port->read_allocated); + gs_free_requests(port->port_usb->in, &port->write_pool, + &port->write_allocated); + } status = -EIO; }