[PATCH] udc: Memory leak on error path and use after free

[PATCH] udc: Memory leak on error path and use after free

[Anton Vasilyev]

gadget_release() is responsible for cleanup dev memory.
But if net2280_probe() fails after dev allocation, then
gadget_release() become unregistered and dev memory leaks.
Also net2280_remove() calls usb_del_gadget_udc() which
perform schedule_delayed_work() with gadget_release(), so
it is possible that dev will be deallocated exactly after
this call and leads to use after free.

The patch moves deallocation from gadget_release() to
net2280_remove().

Found by Linux Driver Verififcation project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
 drivers/usb/gadget/udc/net2280.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index f608c1f..62ac876 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3546,15 +3546,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 	return IRQ_HANDLED;
 }
 
-/*-------------------------------------------------------------------------*/
-
-static void gadget_release(struct device *_dev)
-{
-	struct net2280	*dev = dev_get_drvdata(_dev);
-
-	kfree(dev);
-}
-
 /* tear down the binding between this driver and the pci device */
 
 static void net2280_remove(struct pci_dev *pdev)
@@ -3592,6 +3583,8 @@ static void net2280_remove(struct pci_dev *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_registers);
 
 	ep_info(dev, "unbind\n");
+
+	kfree(dev);
 }
 
 /* wrap this driver around the specified device, but
@@ -3769,8 +3762,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (retval)
 		goto done;
 
-	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
-			gadget_release);
+	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
 	if (retval)
 		goto done;
 	return 0;
-- 
2.7.4

Re: [PATCH] udc: Memory leak on error path and use after free

[Alan Stern]

On Wed, 16 Aug 2017, Anton Vasilyev wrote:

> gadget_release() is responsible for cleanup dev memory.
> But if net2280_probe() fails after dev allocation, then
> gadget_release() become unregistered and dev memory leaks.

This isn't needed if usb_add_gadget_udc_release() is fixed, right?

> Also net2280_remove() calls usb_del_gadget_udc() which
> perform schedule_delayed_work() with gadget_release(), so
> it is possible that dev will be deallocated exactly after
> this call and leads to use after free.

Where is there a possible use after free?

> The patch moves deallocation from gadget_release() to
> net2280_remove().

Alan Stern

Re: [PATCH] udc: Memory leak on error path and use after free

[Anton Vasilyev]

On 16.08.2017 18:29, Alan Stern wrote:
> On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> 
>> gadget_release() is responsible for cleanup dev memory.
>> But if net2280_probe() fails after dev allocation, then
>> gadget_release() become unregistered and dev memory leaks.
> 
> This isn't needed if usb_add_gadget_udc_release() is fixed, right?
> 

No, this situation could appear before call
usb_add_gadget_udc_release().

>> Also net2280_remove() calls usb_del_gadget_udc() which
>> perform schedule_delayed_work() with gadget_release(), so
>> it is possible that dev will be deallocated exactly after
>> this call and leads to use after free.
> 
> Where is there a possible use after free?
> 

net2280_remove() continue work with struct net2280 *dev after call
usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be
deallocated by gadget_release()

>> The patch moves deallocation from gadget_release() to
>> net2280_remove().
> 
> Alan Stern
> 

-- 
Anton Vasilyev
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: vasilyev@ispras.ru

Re: [PATCH] udc: Memory leak on error path and use after free

[Alan Stern]

On Wed, 16 Aug 2017, Anton Vasilyev wrote:

> On 16.08.2017 18:29, Alan Stern wrote:
> > On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> > 
> >> gadget_release() is responsible for cleanup dev memory.
> >> But if net2280_probe() fails after dev allocation, then
> >> gadget_release() become unregistered and dev memory leaks.
> > 
> > This isn't needed if usb_add_gadget_udc_release() is fixed, right?
> > 
> 
> No, this situation could appear before call
> usb_add_gadget_udc_release().
> 
> >> Also net2280_remove() calls usb_del_gadget_udc() which
> >> perform schedule_delayed_work() with gadget_release(), so
> >> it is possible that dev will be deallocated exactly after
> >> this call and leads to use after free.
> > 
> > Where is there a possible use after free?
> > 
> 
> net2280_remove() continue work with struct net2280 *dev after call
> usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be
> deallocated by gadget_release()
> 
> >> The patch moves deallocation from gadget_release() to
> >> net2280_remove().
> > 
> > Alan Stern

Okay, now I understand what you were saying.  Yes, I agree, the 
existing code isn't right.

But a better solution would be to move the usb_del_gadget_udc() call
from the beginning of net2280_remove() to the end.  And make the call
conditional, depending on whether usb_add_gadget_udc_release() has
already been called successfully.

The point is that the device core does not allow drivers to deallocate 
memory containing a struct device before the ->release callback has 
been invoked.  Your patch might do that, if the release was delayed for 
some reason.

Alan Stern

Re: [PATCH] udc: Memory leak on error path and use after free

[Anton Vasilyev]

Sorry for delayed reply.

On 16.08.2017 19:35, Alan Stern wrote:
> On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> 
>> On 16.08.2017 18:29, Alan Stern wrote:
>>> On Wed, 16 Aug 2017, Anton Vasilyev wrote:
>>>
>>>> gadget_release() is responsible for cleanup dev memory.
>>>> But if net2280_probe() fails after dev allocation, then
>>>> gadget_release() become unregistered and dev memory leaks.
>>>
>>> This isn't needed if usb_add_gadget_udc_release() is fixed, right?
>>>
>>
>> No, this situation could appear before call
>> usb_add_gadget_udc_release().
>>
>>>> Also net2280_remove() calls usb_del_gadget_udc() which
>>>> perform schedule_delayed_work() with gadget_release(), so
>>>> it is possible that dev will be deallocated exactly after
>>>> this call and leads to use after free.
>>>
>>> Where is there a possible use after free?
>>>
>>
>> net2280_remove() continue work with struct net2280 *dev after call
>> usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be
>> deallocated by gadget_release()
>>
>>>> The patch moves deallocation from gadget_release() to
>>>> net2280_remove().
>>>
>>> Alan Stern
> 
> Okay, now I understand what you were saying.  Yes, I agree, the
> existing code isn't right.
> 
> But a better solution would be to move the usb_del_gadget_udc() call
> from the beginning of net2280_remove() to the end.  And make the call
> conditional, depending on whether usb_add_gadget_udc_release() has
> already been called successfully.

If allow gadget_release() to deallocate net2280 *dev then it will be 
called on fail of usb_add_gadget_udc_release() and it will be unsafe to 
perform clean-up.
My point is that gadget shouldn't deallocate its parent memory at all.

> 
> The point is that the device core does not allow drivers to deallocate
> memory containing a struct device before the ->release callback has
> been invoked.  Your patch might do that, if the release was delayed for
> some reason.

I don't see possibility for parent device to be removed before its child 
was released. Please point if I'm wrong.

Alternative way to move allocation under devm interface.

> 
> Alan Stern
> 

-- 
Anton Vasilyev
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: vasilyev@ispras.ru

Re: [PATCH] udc: Memory leak on error path and use after free

[Alan Stern]

On Tue, 22 Aug 2017, Anton Vasilyev wrote:

> Sorry for delayed reply.
> 
> On 16.08.2017 19:35, Alan Stern wrote:
> > On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> > 
> >> On 16.08.2017 18:29, Alan Stern wrote:
> >>> On Wed, 16 Aug 2017, Anton Vasilyev wrote:
> >>>
> >>>> gadget_release() is responsible for cleanup dev memory.
> >>>> But if net2280_probe() fails after dev allocation, then
> >>>> gadget_release() become unregistered and dev memory leaks.
> >>>
> >>> This isn't needed if usb_add_gadget_udc_release() is fixed, right?
> >>>
> >>
> >> No, this situation could appear before call
> >> usb_add_gadget_udc_release().
> >>
> >>>> Also net2280_remove() calls usb_del_gadget_udc() which
> >>>> perform schedule_delayed_work() with gadget_release(), so
> >>>> it is possible that dev will be deallocated exactly after
> >>>> this call and leads to use after free.
> >>>
> >>> Where is there a possible use after free?
> >>>
> >>
> >> net2280_remove() continue work with struct net2280 *dev after call
> >> usb_del_gadget_udc(&dev->gadget), but this net2280 *dev could be
> >> deallocated by gadget_release()
> >>
> >>>> The patch moves deallocation from gadget_release() to
> >>>> net2280_remove().
> >>>
> >>> Alan Stern
> > 
> > Okay, now I understand what you were saying.  Yes, I agree, the
> > existing code isn't right.
> > 
> > But a better solution would be to move the usb_del_gadget_udc() call
> > from the beginning of net2280_remove() to the end.  And make the call
> > conditional, depending on whether usb_add_gadget_udc_release() has
> > already been called successfully.
> 
> If allow gadget_release() to deallocate net2280 *dev then it will be 
> called on fail of usb_add_gadget_udc_release() and it will be unsafe to 
> perform clean-up.
> My point is that gadget shouldn't deallocate its parent memory at all.

I disagree.  It's okay for the parent memory to be deallocated along
with the gadget, provided the driver can guarantee that the parent
memory won't be used any more after the gadget is released.

One way to guarantee this is to make usb_add_gadget_udc_release() do an 
extra device_get().  Then the release won't occur until after
usb_del_gadget_udc() has been called _and_ the driver has called 
device_put().

> > The point is that the device core does not allow drivers to deallocate
> > memory containing a struct device before the ->release callback has
> > been invoked.  Your patch might do that, if the release was delayed for
> > some reason.
> 
> I don't see possibility for parent device to be removed before its child 
> was released. Please point if I'm wrong.

The device core has lots of ways of keeping references to a device,
even after the device has been unregistered.  I don't know if any of
them apply in this case, but even if they don't, it's possible that
such a mechanism will be added in the future.

> Alternative way to move allocation under devm interface.

Yes, that would work too.

Don't forget, this problem affects all UDC drivers that call 
usb_add_gadget_udc_release(), not just net2280.  A proper fix will most 
likely have to change all of them.

Alan Stern