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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <35a367f7d71014cf9a6890abc248e18a3d07bc35.camel@kernel.crashing.org>
Date:   Thu, 23 Jul 2020 10:59:06 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Evgeny Novikov <novikov@...ras.ru>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Corentin Labbe <clabbe@...libre.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ldv-project@...uxtesting.org" <ldv-project@...uxtesting.org>
Subject: Re: [PATCH] usb: gadget: net2280: fix memory leak on probe error
 handling paths

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@...land.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@...ras.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