usb: gadget: u_serial: check Null pointer in EP callback

usb: gadget: u_serial: check Null pointer in EP callback

[Daehwan Jung]

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;
+	}
+
 	/* 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--;
-- 
2.7.4

Re: usb: gadget: u_serial: check Null pointer in EP callback

[Greg Kroah-Hartman]

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

Re: Re: usb: gadget: u_serial: check Null pointer in EP callback

[gregkh]

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

Re: usb: gadget: u_serial: check Null pointer in EP callback

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 4039 bytes --]

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
> 
> 



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: gadget: u_serial: check Null pointer in EP callback

[Greg Kroah-Hartman]

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