linux-kernel - Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2017 10:17:45 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Felipe Balbi <balbi@...nel.org>
cc:     Roger Quadros <rogerq@...com>, <vivek.gautam@...eaurora.org>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same
 gadget device

On Tue, 4 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@...land.harvard.edu> writes:
> > On Mon, 3 Apr 2017, Roger Quadros wrote:
> >
> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> >> repeatedly on the same gadget->dev structure.
> >> 
> >> We need to clear the gadget->dev structure so that kobject_init()
> >> doesn't complain about already initialized object.
> >> 
> >> Signed-off-by: Roger Quadros <rogerq@...com>
> >> ---
> >>  drivers/usb/gadget/udc/core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> >> index d685d82..efce68e 100644
> >> --- a/drivers/usb/gadget/udc/core.c
> >> +++ b/drivers/usb/gadget/udc/core.c
> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >>  	flush_work(&gadget->work);
> >>  	device_unregister(&udc->dev);
> >>  	device_unregister(&gadget->dev);
> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >>  }
> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >
> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> 
> not on the gadget API, no.
> 
> > call on the previous line invokes the gadget->dev.release callback, 
> > which might deallocate gadget.  If that happens, your new memset will 
> > oops.
> 
> that won't happen. struct usb_gadget is a member of the UDC's private
> structure, like this:
> 
> struct dwc3 {
> 	[...]
> 	struct usb_gadget	gadget;
> 	struct usb_gadget_driver *gadget_driver;
> 	[...]
> };

Yes.  So what?  Can't the UDC driver use the refcount inside struct 
usb_gadget to control the lifetime of its private structure?

(By the way, can you tell what's going on in net2280.c?  I must be
missing something; it looks like gadget_release() would quickly run
into problems because it calls dev_get_drvdata() for &gadget->dev, but
net2280_probe() never calls dev_set_drvdata() for that device.  
Furthermore, net2280_remove() continues to reference the net2280 struct
after calling usb_del_gadget_udc(), and it never does seem to do a
final put.)

> I'm actually thinking that struct usb_gadget shouldn't have a struct
> device at all. Just a pointer to a device, that would solve all these
> issues.

A pointer to which device?  The UDC?  That would change the directory 
layout in sysfs.

Or a pointer to a separate dynamically allocated device (the way struct 
usb_hcd contains a pointer to the root hub device)?  That would work.  
If the UDC driver wanted to re-register the gadget, it would have to 
allocate a new device.

Alan Stern