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

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

[Evgeny Novikov]

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;
 }
 
-- 
2.16.4

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

[Alan Stern]

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

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

[Evgeny Novikov]

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.

-- 
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

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

[Benjamin Herrenschmidt]

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

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

[Alan Stern]

On Thu, Jul 23, 2020 at 10:59:06AM +1000, Benjamin Herrenschmidt wrote:
> 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.

That probably won't be needed, although thanks for the offer.  I'll CC 
you on any patches when they are submitted.

Alan Stern

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

[Alan Stern]

On Wed, Jul 22, 2020 at 10:56:09PM +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.

Heh...  Working from home, I no longer have access to the appropriate 
hardware either.  But at least I do have the necessary experience.  :-)

> 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.

I don't recall that.  Do you have a pointer to the email thread in the 
archives?

> Anyway, one can consider both issues independently on the one fixed by 
> the patch.

Yes.  I'll write and submit a series of patches.

Alan Stern

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

[Evgeny Novikov]

23.07.2020, 04:00, "Alan Stern" <stern@rowland.harvard.edu>:
> On Wed, Jul 22, 2020 at 10:56:09PM +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.
>
> Heh... Working from home, I no longer have access to the appropriate
> hardware either. But at least I do have the necessary experience. :-)
>
>>  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.
>
> I don't recall that. Do you have a pointer to the email thread in the
> archives?
https://lkml.org/lkml/2017/8/16/345 - this is the top message of that thread.

Evgeny
>>  Anyway, one can consider both issues independently on the one fixed by
>>  the patch.
>
> Yes. I'll write and submit a series of patches.
>
> Alan Stern