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