Re: [PATCH] usb: gadget: net2280: fix memory leak on probe error handling paths

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2020-07-22 at 22:56 +0300, Evgeny Novikov wrote:
> Hi Alan,
> 
> I have neither an appropriate hardware nor an experience to deal with
> issues that you mentioned. Our framework does not allow to detect
> them as well at the moment. At last, it seems that rather many
> drivers can suffer from these issues. So, it would be much better if
> somebody else will suggest necessary fixes and test them carefully.
> 
> BTW, you have already discussed the race within net2280_remove() with
> my colleague about 3 years ago. But you did not achieve a consensus
> at that time and no fixes were made after all.
> 
> Anyway, one can consider both issues independently on the one fixed
> by the patch.

FYI. It looks like I'm likely to resume my work on that driver in the
next few weeks in which case I could probably look into these Alan.

Cheers,
Ben.


> -- 
> Evgeny Novikov
> Linux Verification Center, ISP RAS
> http://linuxtesting.org
> 
> 22.07.2020, 17:17, "Alan Stern" <stern@rowland.harvard.edu>:
> > On Tue, Jul 21, 2020 at 11:15:58PM +0300, Evgeny Novikov wrote:
> > >  Driver does not release memory for device on error handling
> > > paths in
> > >  net2280_probe() when gadget_release() is not registered yet.
> > > 
> > >  The patch fixes the bug like in other similar drivers.
> > > 
> > >  Found by Linux Driver Verification project (linuxtesting.org).
> > > 
> > >  Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> > >  ---
> > >   drivers/usb/gadget/udc/net2280.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > >  diff --git a/drivers/usb/gadget/udc/net2280.c
> > > b/drivers/usb/gadget/udc/net2280.c
> > >  index 5eff85eeaa5a..d5fe071b2db2 100644
> > >  --- a/drivers/usb/gadget/udc/net2280.c
> > >  +++ b/drivers/usb/gadget/udc/net2280.c
> > >  @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev
> > > *pdev, const struct pci_device_id *id)
> > >           return 0;
> > > 
> > >   done:
> > >  - if (dev)
> > >  + if (dev) {
> > >                   net2280_remove(pdev);
> > >  + kfree(dev);
> > >  + }
> > >           return retval;
> > >   }
> > 
> > This patch seems to be the tip of an iceberg. Following through its
> > implications led to a couple of discoveries.
> > 
> > usb_del_gadget_udc() calls device_unregister(&gadget->dev). Once
> > this
> > call returns, gadget has to be regarded as a stale pointer. But the
> > very next line of code does:
> > 
> >         memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> > 
> > for no apparent reason. I'm amazed this hasn't caused problems
> > already.
> > Is there any justification for keeping this memset? It's hard to
> > imagine that it does any good.
> > 
> > Similarly, net2280_remove() calls usb_del_gadget_udc(&dev->gadget)
> > at
> > its start, and so dev must be a stale pointer for the entire
> > remainder
> > of the routine. But it gets used repeatedly. Surely we ought to
> > have
> > a device_get() and device_put() in there.
> > 
> > Alan Stern