Re: [PATCH] udc: Memory leak on error path and use after free

[Alan Stern]

On Tue, 22 Aug 2017, Anton Vasilyev wrote:

> Sorry for delayed reply.
> 
> On 16.08.2017 19:35, Alan Stern wrote:
> > On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> > 
> >> On 16.08.2017 18:29, Alan Stern wrote:
> >>> On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> >>>
> >>>> gadget_release() is responsible for cleanup dev memory.
> >>>> But if net2280_probe() fails after dev allocation, then
> >>>> gadget_release() become unregistered and dev memory leaks.
> >>>
> >>> This isn't needed if usb_add_gadget_udc_release() is fixed, right?
> >>>
> >>
> >> No, this situation could appear before call
> >> usb_add_gadget_udc_release().
> >>
> >>>> Also net2280_remove() calls usb_del_gadget_udc() which
> >>>> perform schedule_delayed_work() with gadget_release(), so
> >>>> it is possible that dev will be deallocated exactly after
> >>>> this call and leads to use after free.
> >>>
> >>> Where is there a possible use after free?
> >>>
> >>
> >> net2280_remove() continue work with struct net2280 *dev after call
> >> usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be
> >> deallocated by gadget_release()
> >>
> >>>> The patch moves deallocation from gadget_release() to
> >>>> net2280_remove().
> >>>
> >>> Alan Stern
> > 
> > Okay, now I understand what you were saying.  Yes, I agree, the
> > existing code isn't right.
> > 
> > But a better solution would be to move the usb_del_gadget_udc() call
> > from the beginning of net2280_remove() to the end.  And make the call
> > conditional, depending on whether usb_add_gadget_udc_release() has
> > already been called successfully.
> 
> If allow gadget_release() to deallocate net2280 *dev then it will be 
> called on fail of usb_add_gadget_udc_release() and it will be unsafe to 
> perform clean-up.
> My point is that gadget shouldn't deallocate its parent memory at all.

I disagree.  It's okay for the parent memory to be deallocated along
with the gadget, provided the driver can guarantee that the parent
memory won't be used any more after the gadget is released.

One way to guarantee this is to make usb_add_gadget_udc_release() do an 
extra device_get().  Then the release won't occur until after
usb_del_gadget_udc() has been called _and_ the driver has called 
device_put().

> > The point is that the device core does not allow drivers to deallocate
> > memory containing a struct device before the ->release callback has
> > been invoked.  Your patch might do that, if the release was delayed for
> > some reason.
> 
> I don't see possibility for parent device to be removed before its child 
> was released. Please point if I'm wrong.

The device core has lots of ways of keeping references to a device,
even after the device has been unregistered.  I don't know if any of
them apply in this case, but even if they don't, it's possible that
such a mechanism will be added in the future.

> Alternative way to move allocation under devm interface.

Yes, that would work too.

Don't forget, this problem affects all UDC drivers that call 
usb_add_gadget_udc_release(), not just net2280.  A proper fix will most 
likely have to change all of them.

Alan Stern