| Message ID | TYUPR06MB621763AB815989161F4033AFD2762@TYUPR06MB6217.apcprd06.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: u_serial: Disable ep before setting port to null to fix the crash caused by port being null | expand |
Hi 胡连勤 On Mon, Sep 30, 2024 at 3:48 AM 胡连勤 <hulianqin@vivo.com> wrote: > > From: Lianqin Hu <hulianqin@vivo.com> > > Considering that in some extreme cases, when performing the > unbinding operation, gserial_disconnect has cleared gser->ioport, > which triggers gadget reconfiguration, and then calls gs_read_complete, > resulting in access to a null pointer. Therefore, ep is disabled before > gserial_disconnect sets port to null to prevent this from happening. > > Unable to handle kernel NULL pointer dereference at > virtual address 00000000000001a8 > pc : gs_read_complete+0x58/0x240 > lr : usb_gadget_giveback_request+0x40/0x160 > sp : ffffffc00f1539c0 > x29: ffffffc00f1539c0 x28: ffffff8002a30000 x27: 0000000000000000 > x26: ffffff8002a30000 x25: 0000000000000000 x24: ffffff8002a30000 > x23: ffffff8002ff9a70 x22: ffffff898e7a7b00 x21: ffffff803c9af9d8 > x20: ffffff898e7a7b00 x19: 00000000000001a8 x18: ffffffc0099fd098 > x17: 0000000000001000 x16: 0000000080000000 x15: 0000000ac1200000 > x14: 0000000000000003 x13: 000000000000d5e8 x12: 0000000355c314ac > x11: 0000000000000015 x10: 0000000000000012 x9 : 0000000000000008 > x8 : 0000000000000000 x7 : 0000000000000000 x6 : ffffff887cd12000 > x5 : 0000000000000002 x4 : ffffffc00f9b07f0 x3 : ffffffc00f1538d0 > x2 : 0000000000000001 x1 : 0000000000000000 x0 : 00000000000001a8 > Call trace: > gs_read_complete+0x58/0x240 > usb_gadget_giveback_request+0x40/0x160 > dwc3_remove_requests+0x170/0x484 > dwc3_ep0_out_start+0xb0/0x1d4 > __dwc3_gadget_start+0x25c/0x720 > kretprobe_trampoline.cfi_jt+0x0/0x8 > kretprobe_trampoline.cfi_jt+0x0/0x8 > udc_bind_to_driver+0x1d8/0x300 > usb_gadget_probe_driver+0xa8/0x1dc > gadget_dev_desc_UDC_store+0x13c/0x188 > configfs_write_iter+0x160/0x1f4 > vfs_write+0x2d0/0x40c > ksys_write+0x7c/0xf0 > __arm64_sys_write+0x20/0x30 > invoke_syscall+0x60/0x150 > el0_svc_common+0x8c/0xf8 > do_el0_svc+0x28/0xa0 > el0_svc+0x24/0x84 > el0t_64_sync_handler+0x88/0xec > el0t_64_sync+0x1b4/0x1b8 > Code: aa1f03e1 aa1303e0 52800022 2a0103e8 (88e87e62) > ---[ end trace 938847327a739172 ]--- > Kernel panic - not syncing: Oops: Fatal exception > > Fixes: c1dca562be8a ("usb gadget: split out serial core") > Cc: stable@vger.kernel.org > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> > --- > drivers/usb/gadget/function/u_serial.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index b394105e55d6..1712e9cd08be 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -1395,6 +1395,10 @@ void gserial_disconnect(struct gserial *gser) > /* REVISIT as above: how best to track this? */ > port->port_line_coding = gser->port_line_coding; > > + /* disable endpoints, aborting down any active I/O */ > + usb_ep_disable(gser->out); > + usb_ep_disable(gser->in); > + According to the review from here https://lore.kernel.org/lkml/64ad7661-4551-7b00-604b-6e15da23a1c7@quicinc.com/T/ Greg and I suggest to abort before but Prashanth K comment " Not sure about this case, I think generally we need stop IO before disabling EP, otherwise TX/RX functions may queue requests while EP is getting disabled, thats why i think port is removed before ep_disable. Moreover these callbacks (complete/suspend/resume etc) comes from UDC and can be async, so better to use locks to prevent these kind of races." Michael > port->port_usb = NULL; > gser->ioport = NULL; > if (port->port.count > 0) { > @@ -1406,10 +1410,6 @@ void gserial_disconnect(struct gserial *gser) > spin_unlock(&port->port_lock); > spin_unlock_irqrestore(&serial_port_lock, flags); > > - /* disable endpoints, aborting down any active I/O */ > - usb_ep_disable(gser->out); > - usb_ep_disable(gser->in); > - > /* finally, free any unused/unusable I/O buffers */ > spin_lock_irqsave(&port->port_lock, flags); > if (port->port.count == 0) > -- > 2.39.0 >
Hi Michael: >> From: Lianqin Hu <hulianqin@vivo.com> >> >> Considering that in some extreme cases, when performing the unbinding >> operation, gserial_disconnect has cleared gser->ioport, which triggers >> gadget reconfiguration, and then calls gs_read_complete, resulting in >> access to a null pointer. Therefore, ep is disabled before >> gserial_disconnect sets port to null to prevent this from happening. >> >> Unable to handle kernel NULL pointer dereference at virtual address >> 00000000000001a8 pc : gs_read_complete+0x58/0x240 lr : >> usb_gadget_giveback_request+0x40/0x160 >> sp : ffffffc00f1539c0 >> x29: ffffffc00f1539c0 x28: ffffff8002a30000 x27: 0000000000000000 >> x26: ffffff8002a30000 x25: 0000000000000000 x24: ffffff8002a30000 >> x23: ffffff8002ff9a70 x22: ffffff898e7a7b00 x21: ffffff803c9af9d8 >> x20: ffffff898e7a7b00 x19: 00000000000001a8 x18: ffffffc0099fd098 >> x17: 0000000000001000 x16: 0000000080000000 x15: 0000000ac1200000 >> x14: 0000000000000003 x13: 000000000000d5e8 x12: 0000000355c314ac >> x11: 0000000000000015 x10: 0000000000000012 x9 : 0000000000000008 >> x8 : 0000000000000000 x7 : 0000000000000000 x6 : ffffff887cd12000 >> x5 : 0000000000000002 x4 : ffffffc00f9b07f0 x3 : ffffffc00f1538d0 >> x2 : 0000000000000001 x1 : 0000000000000000 x0 : 00000000000001a8 Call >> trace: >> gs_read_complete+0x58/0x240 >> usb_gadget_giveback_request+0x40/0x160 >> dwc3_remove_requests+0x170/0x484 >> dwc3_ep0_out_start+0xb0/0x1d4 >> __dwc3_gadget_start+0x25c/0x720 >> kretprobe_trampoline.cfi_jt+0x0/0x8 >> kretprobe_trampoline.cfi_jt+0x0/0x8 >> udc_bind_to_driver+0x1d8/0x300 >> usb_gadget_probe_driver+0xa8/0x1dc >> gadget_dev_desc_UDC_store+0x13c/0x188 >> configfs_write_iter+0x160/0x1f4 >> vfs_write+0x2d0/0x40c >> ksys_write+0x7c/0xf0 >> __arm64_sys_write+0x20/0x30 >> invoke_syscall+0x60/0x150 >> el0_svc_common+0x8c/0xf8 >> do_el0_svc+0x28/0xa0 >> el0_svc+0x24/0x84 >> el0t_64_sync_handler+0x88/0xec >> el0t_64_sync+0x1b4/0x1b8 >> Code: aa1f03e1 aa1303e0 52800022 2a0103e8 (88e87e62) ---[ end trace > 938847327a739172 ]--- Kernel panic - not syncing: Oops: Fatal >> exception >> >> Fixes: c1dca562be8a ("usb gadget: split out serial core") >> Cc: stable@vger.kernel.org >> >> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Lianqin Hu <hulianqin@vivo.com> >> --- >> drivers/usb/gadget/function/u_serial.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/u_serial.c >> b/drivers/usb/gadget/function/u_serial.c >> index b394105e55d6..1712e9cd08be 100644 >> --- a/drivers/usb/gadget/function/u_serial.c >> +++ b/drivers/usb/gadget/function/u_serial.c >> @@ -1395,6 +1395,10 @@ void gserial_disconnect(struct gserial *gser) >> /* REVISIT as above: how best to track this? */ >> port->port_line_coding = gser->port_line_coding; >> >> + /* disable endpoints, aborting down any active I/O */ >> + usb_ep_disable(gser->out); >> + usb_ep_disable(gser->in); >> + >According to the review from here >https://lore.kernel.org/lkml/64ad7661-4551-7b00-604b-6e15da23a1c7@quicinc.com/T/ >Greg and I suggest to abort before but Prashanth K comment " >Not sure about this case, I think generally we need stop IO before disabling EP, otherwise TX/RX functions may queue requests while EP is getting disabled, thats why i think port is removed before ep_disable. >Moreover these callbacks (complete/suspend/resume etc) comes from UDC and can be async, so better to use locks to prevent these kind of races." After merging the two patches separately and retesting, the problem did not recur. From the actual test situation, both methods can solve the crash problem caused by competition here. Thanks
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index b394105e55d6..1712e9cd08be 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1395,6 +1395,10 @@ void gserial_disconnect(struct gserial *gser) /* REVISIT as above: how best to track this? */ port->port_line_coding = gser->port_line_coding; + /* disable endpoints, aborting down any active I/O */ + usb_ep_disable(gser->out); + usb_ep_disable(gser->in); + port->port_usb = NULL; gser->ioport = NULL; if (port->port.count > 0) { @@ -1406,10 +1410,6 @@ void gserial_disconnect(struct gserial *gser) spin_unlock(&port->port_lock); spin_unlock_irqrestore(&serial_port_lock, flags); - /* disable endpoints, aborting down any active I/O */ - usb_ep_disable(gser->out); - usb_ep_disable(gser->in); - /* finally, free any unused/unusable I/O buffers */ spin_lock_irqsave(&port->port_lock, flags); if (port->port.count == 0)