[ldv-project] [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 at 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 at 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