lkml.org 
[lkml]   [2020]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] usb: gadget: net2280: fix memory leak on probe error handling paths
From
Date
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

\
 
 \ /
  Last update: 2020-07-23 03:00    [W:0.056 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site