Messages in this thread |  | | Subject | Re: [PATCH] udc: Memory leak on error path and use after free | From | Anton Vasilyev <> | Date | Tue, 22 Aug 2017 18:45:29 +0300 |
| |
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.
> > 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.
Alternative way to move allocation under devm interface.
> > Alan Stern >
-- Anton Vasilyev Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: vasilyev@ispras.ru
|  |