| Message ID | TYUPR06MB62172CC6BA9647EF769D4DF2D2812@TYUPR06MB6217.apcprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: u_serial: check Null pointer in EP callback | expand |
On Fri, Aug 16, 2024 at 10:49:19AM +0000, 胡连勤 wrote: > From: Lianqin Hu <hulianqin@vivo.com> > > Added null pointer check to avoid system crash. > > 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 > > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> > --- > drivers/usb/gadget/function/u_serial.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index b394105e55d6..65637d53bf02 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -454,6 +454,14 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) > { > struct gs_port *port = ep->driver_data; > > + /* > + * When port is NULL, Return to avoid panic. > + */ Multiple line comments for something like this really isn't needed. > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); No need for this either, right? But how can this happen? What is causing driver_data to be NULL? And what protects it from happening right after you check for it? > + return; > + } > + > /* Queue all received data until the tty layer is ready for it. */ > spin_lock(&port->port_lock); > list_add_tail(&req->list, &port->read_queue); > @@ -465,6 +473,14 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) > { > struct gs_port *port = ep->driver_data; > > + /* > + * When port is NULL, Return to avoid panic. > + */ > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); > + return; > + } Same question here, what is keeping it from changing right after you test? thanks, greg k-h
Hello linux community expert: Q: What is causing driver_data to be NULL? And what protects it from happening right after you check for it? A: This is a very in-depth question, and I need to have a deeper understanding of the code before I can answer it. Judging from the crash stack, this patch currently has improved the crash problem. Thanks -----邮件原件----- 发件人: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> 发送时间: 2024年8月16日 19:01 收件人: 胡连勤 <hulianqin@vivo.com> 抄送: quic_prashk@quicinc.com; quic_jjohnson@quicinc.com; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>; akpm@linux-foundation.org 主题: Re: [PATCH] usb: gadget: u_serial: check Null pointer in EP callback On Fri, Aug 16, 2024 at 10:49:19AM +0000, 胡连勤 wrote: > From: Lianqin Hu <hulianqin@vivo.com> > > Added null pointer check to avoid system crash. > > 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 > > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> > --- > drivers/usb/gadget/function/u_serial.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_serial.c > b/drivers/usb/gadget/function/u_serial.c > index b394105e55d6..65637d53bf02 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -454,6 +454,14 @@ static void gs_read_complete(struct usb_ep *ep, > struct usb_request *req) { > struct gs_port *port = ep->driver_data; > > + /* > + * When port is NULL, Return to avoid panic. > + */ Multiple line comments for something like this really isn't needed. > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); No need for this either, right? But how can this happen? What is causing driver_data to be NULL? And what protects it from happening right after you check for it? > + return; > + } > + > /* Queue all received data until the tty layer is ready for it. */ > spin_lock(&port->port_lock); > list_add_tail(&req->list, &port->read_queue); @@ -465,6 +473,14 @@ > static void gs_write_complete(struct usb_ep *ep, struct usb_request > *req) { > struct gs_port *port = ep->driver_data; > > + /* > + * When port is NULL, Return to avoid panic. > + */ > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); > + return; > + } Same question here, what is keeping it from changing right after you test? thanks, greg k-h
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Fri, Aug 16, 2024 at 11:16:08AM +0000, 胡连勤 wrote: > Hello linux community expert: > > Q: What is causing driver_data to be NULL? And what protects it from happening right after you check for it? > A: This is a very in-depth question, and I need to have a deeper understanding of the code before I can answer it. > Judging from the crash stack, this patch currently has improved the crash problem. Please figure this out, we can't take changes that don't actually fix the root problem for obvious reasons (and you wouldn't want us to either.) thanks, greg k-h
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index b394105e55d6..65637d53bf02 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -454,6 +454,14 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) { struct gs_port *port = ep->driver_data; + /* + * When port is NULL, Return to avoid panic. + */ + if (!port) { + pr_err("%s, failed to get port\n", __func__); + return; + } + /* Queue all received data until the tty layer is ready for it. */ spin_lock(&port->port_lock); list_add_tail(&req->list, &port->read_queue); @@ -465,6 +473,14 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) { struct gs_port *port = ep->driver_data; + /* + * When port is NULL, Return to avoid panic. + */ + if (!port) { + pr_err("%s, failed to get port\n", __func__); + return; + } + spin_lock(&port->port_lock); list_add(&req->list, &port->write_pool); port->write_started--;