| Message ID | 1628236406-185160-1-git-send-email-dh10.jung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: u_serial: check Null pointer in EP callback | expand |
On Fri, Aug 06, 2021 at 04:53:26PM +0900, Daehwan Jung wrote: > From: taehyun cho <taehyun.cho@samsung.com> > > Endpoint complete function in u_serial can be executed > when 'gs_port' is Null. This situation happens when > 'dwc3_gadget_pullup' returns ETIMEDOUT. The reason why > ETIMEDOUT is returned is that whole system is out of order > including interrupt regardless of USB. > > pc : __lock_acquire+0x54/0x5ec > lr : lock_acquire+0xe8/0x198 > sp : ffffffc03914b9d0 > x29: ffffffc03914b9d0 x28: ffffff895f13b680 > x27: 0000000000000000 x26: 0000000000000000 > x25: 00000000000003c8 x24: 0000000000000080 > x23: ffffffc010a8f650 x22: 0000000000000000 > x21: 0000000000000000 x20: 0000000000000000 > x19: ffffffc010a8f650 x18: ffffffc02d70a0b0 > x17: 0000000000000000 x16: 00000000000229e0 > x15: 0000000000000004 x14: 00000000000004f2 > x13: ffffffc0120fe178 x12: 0000000000000003 > x11: 00000000ffffffff x10: 0000000100000001 > x9 : 0000000000000001 x8 : 00000000000003c8 > x7 : 0000000000000000 x6 : ffffffc010a8f650 > x5 : 0000000000000000 x4 : 0000000000000080 > x3 : 0000000000000000 x2 : 0000000000000000 > x1 : 0000000000000000 x0 : 00000000000003c8 > Call trace: > __lock_acquire+0x54/0x5ec > lock_acquire+0xe8/0x198 > _raw_spin_lock+0x70/0x88 > gs_read_complete+0x48/0xac > usb_gadget_giveback_request+0x48/0x80 > dwc3_gadget_giveback+0xcc/0xe8 > dwc3_remove_requests+0xa8/0x188 > __dwc3_gadget_ep_disable+0x98/0x110 > dwc3_gadget_ep_disable+0x50/0xbc > usb_ep_disable+0x44/0x94 > gserial_disconnect+0xc0/0x250 > acm_free_func+0x30/0x48 > usb_put_function+0x34/0x68 > config_usb_cfg_unlink+0xdc/0xf8 > configfs_unlink+0x144/0x264 > vfs_unlink+0x134/0x218 > do_unlinkat+0x13c/0x2a0 > __arm64_sys_unlinkat+0x48/0x60 > el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4 > do_el0_svc+0x24/0x8c > el0_svc+0x10/0x1c > el0_sync_handler+0x68/0xac > el0_sync+0x18c/0x1c0 > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > --- > drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index 6f68cbe..af08a18 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) > { > struct gs_port *port = ep->driver_data; > > + /* > + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > + * Return here to avoid panic. > + */ > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); > + return; > + } > + What prevents port from being null right after checking this? Where is the lock to prevent this? > /* 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); > @@ -461,6 +470,15 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) > { > struct gs_port *port = ep->driver_data; > > + /* > + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > + * Return here to avoid panic. > + */ > + if (!port) { > + pr_err("%s, failed to get port\n", __func__); > + return; > + } Same here, where is the lock? And why report an error, what can a user do about it? thanks, greg k-h
On Wed, Sep 08, 2021 at 04:54:13PM +0900, 정대환 wrote: <snip> Please do not send html email, it is rejected by the mailing lists. Please fix your email client and resend. thanks, greg k-h
On Fri, Aug 06, 2021 at 05:13:34PM +0200, Greg Kroah-Harman wrote: > On Fri, Aug 06, 2021 at 04:53:26PM +0900, Daehwan Jung wrote: > > From: taehyun cho <taehyun.cho@samsung.com> > > > > Endpoint complete function in u_serial can be executed when 'gs_port' > > is Null. This situation happens when 'dwc3_gadget_pullup' returns > > ETIMEDOUT. The reason why ETIMEDOUT is returned is that whole system > > is out of order including interrupt regardless of USB. > > > > pc : __lock_acquire+0x54/0x5ec > > lr : lock_acquire+0xe8/0x198 > > sp : ffffffc03914b9d0 > > x29: ffffffc03914b9d0 x28: ffffff895f13b680 > > x27: 0000000000000000 x26: 0000000000000000 > > x25: 00000000000003c8 x24: 0000000000000080 > > x23: ffffffc010a8f650 x22: 0000000000000000 > > x21: 0000000000000000 x20: 0000000000000000 > > x19: ffffffc010a8f650 x18: ffffffc02d70a0b0 > > x17: 0000000000000000 x16: 00000000000229e0 > > x15: 0000000000000004 x14: 00000000000004f2 > > x13: ffffffc0120fe178 x12: 0000000000000003 > > x11: 00000000ffffffff x10: 0000000100000001 > > x9 : 0000000000000001 x8 : 00000000000003c8 > > x7 : 0000000000000000 x6 : ffffffc010a8f650 > > x5 : 0000000000000000 x4 : 0000000000000080 > > x3 : 0000000000000000 x2 : 0000000000000000 > > x1 : 0000000000000000 x0 : 00000000000003c8 Call trace: > > __lock_acquire+0x54/0x5ec > > lock_acquire+0xe8/0x198 > > _raw_spin_lock+0x70/0x88 > > gs_read_complete+0x48/0xac > > usb_gadget_giveback_request+0x48/0x80 > > dwc3_gadget_giveback+0xcc/0xe8 > > dwc3_remove_requests+0xa8/0x188 > > __dwc3_gadget_ep_disable+0x98/0x110 > > dwc3_gadget_ep_disable+0x50/0xbc > > usb_ep_disable+0x44/0x94 > > gserial_disconnect+0xc0/0x250 > > acm_free_func+0x30/0x48 > > usb_put_function+0x34/0x68 > > config_usb_cfg_unlink+0xdc/0xf8 > > configfs_unlink+0x144/0x264 > > vfs_unlink+0x134/0x218 > > do_unlinkat+0x13c/0x2a0 > > __arm64_sys_unlinkat+0x48/0x60 > > el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4 > > do_el0_svc+0x24/0x8c > > el0_svc+0x10/0x1c > > el0_sync_handler+0x68/0xac > > el0_sync+0x18c/0x1c0 > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > --- > > drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/u_serial.c > > b/drivers/usb/gadget/function/u_serial.c > > index 6f68cbe..af08a18 100644 > > --- a/drivers/usb/gadget/function/u_serial.c > > +++ b/drivers/usb/gadget/function/u_serial.c > > @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, > > struct usb_request *req) { > > struct gs_port *port = ep->driver_data; > > > > + /* > > + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > > + * Return here to avoid panic. > > + */ > > + if (!port) { > > + pr_err("%s, failed to get port\n", __func__); > > + return; > > + } > > + > spin_lock(&port->port_lock); ... spin_unlock(&port->port_lock); > What prevents port from being null right after checking this? Where is the > lock to prevent this? > It tries to get lock first in gs_read_complete/gs_write_complete like above. That's why the panic occured during getting lock but this issue is not related to lock. We just want to prevent doing something after port becomes null. > > /* 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); @@ -461,6 +470,15 @@ > > static void gs_write_complete(struct usb_ep *ep, struct usb_request > > *req) { > > struct gs_port *port = ep->driver_data; > > > > + /* > > + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > > + * Return here to avoid panic. > > + */ > > + if (!port) { > > + pr_err("%s, failed to get port\n", __func__); > > + return; > > + } > > Same here, where is the lock? > > And why report an error, what can a user do about it? > It could happen to access null pointer and occur whole system panic. Best Regards, Jung Daehwan > thanks, > > greg k-h > >
On Wed, Sep 08, 2021 at 07:21:29PM +0900, Jung Daehwan wrote: > On Fri, Aug 06, 2021 at 05:13:34PM +0200, Greg Kroah-Harman wrote: > > On Fri, Aug 06, 2021 at 04:53:26PM +0900, Daehwan Jung wrote: > > > From: taehyun cho <taehyun.cho@samsung.com> > > > > > > Endpoint complete function in u_serial can be executed when 'gs_port' > > > is Null. This situation happens when 'dwc3_gadget_pullup' returns > > > ETIMEDOUT. The reason why ETIMEDOUT is returned is that whole system > > > is out of order including interrupt regardless of USB. > > > > > > pc : __lock_acquire+0x54/0x5ec > > > lr : lock_acquire+0xe8/0x198 > > > sp : ffffffc03914b9d0 > > > x29: ffffffc03914b9d0 x28: ffffff895f13b680 > > > x27: 0000000000000000 x26: 0000000000000000 > > > x25: 00000000000003c8 x24: 0000000000000080 > > > x23: ffffffc010a8f650 x22: 0000000000000000 > > > x21: 0000000000000000 x20: 0000000000000000 > > > x19: ffffffc010a8f650 x18: ffffffc02d70a0b0 > > > x17: 0000000000000000 x16: 00000000000229e0 > > > x15: 0000000000000004 x14: 00000000000004f2 > > > x13: ffffffc0120fe178 x12: 0000000000000003 > > > x11: 00000000ffffffff x10: 0000000100000001 > > > x9 : 0000000000000001 x8 : 00000000000003c8 > > > x7 : 0000000000000000 x6 : ffffffc010a8f650 > > > x5 : 0000000000000000 x4 : 0000000000000080 > > > x3 : 0000000000000000 x2 : 0000000000000000 > > > x1 : 0000000000000000 x0 : 00000000000003c8 Call trace: > > > __lock_acquire+0x54/0x5ec > > > lock_acquire+0xe8/0x198 > > > _raw_spin_lock+0x70/0x88 > > > gs_read_complete+0x48/0xac > > > usb_gadget_giveback_request+0x48/0x80 > > > dwc3_gadget_giveback+0xcc/0xe8 > > > dwc3_remove_requests+0xa8/0x188 > > > __dwc3_gadget_ep_disable+0x98/0x110 > > > dwc3_gadget_ep_disable+0x50/0xbc > > > usb_ep_disable+0x44/0x94 > > > gserial_disconnect+0xc0/0x250 > > > acm_free_func+0x30/0x48 > > > usb_put_function+0x34/0x68 > > > config_usb_cfg_unlink+0xdc/0xf8 > > > configfs_unlink+0x144/0x264 > > > vfs_unlink+0x134/0x218 > > > do_unlinkat+0x13c/0x2a0 > > > __arm64_sys_unlinkat+0x48/0x60 > > > el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4 > > > do_el0_svc+0x24/0x8c > > > el0_svc+0x10/0x1c > > > el0_sync_handler+0x68/0xac > > > el0_sync+0x18c/0x1c0 > > > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > > --- > > > drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/u_serial.c > > > b/drivers/usb/gadget/function/u_serial.c > > > index 6f68cbe..af08a18 100644 > > > --- a/drivers/usb/gadget/function/u_serial.c > > > +++ b/drivers/usb/gadget/function/u_serial.c > > > @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, > > > struct usb_request *req) { > > > struct gs_port *port = ep->driver_data; > > > > > > + /* > > > + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > > > + * Return here to avoid panic. > > > + */ > > > + if (!port) { > > > + pr_err("%s, failed to get port\n", __func__); > > > + return; > > > + } > > > + > > > spin_lock(&port->port_lock); > ... > spin_unlock(&port->port_lock); > > What prevents port from being null right after checking this? Where is the > > lock to prevent this? > > > It tries to get lock first in gs_read_complete/gs_write_complete like above. > That's why the panic occured during getting lock but this issue is not related > to lock. We just want to prevent doing something after port becomes null. I do not understand, you are not protecting anything here, what happens if port becomes NULL right after checking it and before the lock? Either this needs to be tested like this, or it does not at all. This change does not really fix the issue. > > > /* 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); @@ -461,6 +470,15 @@ > > > static void gs_write_complete(struct usb_ep *ep, struct usb_request > > > *req) { > > > struct gs_port *port = ep->driver_data; > > > > > > + /* > > > + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. > > > + * Return here to avoid panic. > > > + */ > > > + if (!port) { > > > + pr_err("%s, failed to get port\n", __func__); > > > + return; > > > + } > > > > Same here, where is the lock? > > > > And why report an error, what can a user do about it? > > > > It could happen to access null pointer and occur whole system panic. Again, what is setting that pointer to NULL and why isn't it caught before this? Shouldn't everything be properly torn down and the completions finished _before_ the port is set to NULL? If not, then fix that issue, as this change will not fix the real problem here, only delay it. thanks, greg k-h
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 6f68cbe..af08a18 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) { struct gs_port *port = ep->driver_data; + /* + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. + * Return here 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); @@ -461,6 +470,15 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) { struct gs_port *port = ep->driver_data; + /* + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT. + * Return here 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--;