* [PATCH] udc: Memory leak on error path and use after free
@ 2017-08-16 13:47 Anton Vasilyev
2017-08-16 15:29 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Anton Vasilyev @ 2017-08-16 13:47 UTC (permalink / raw)
To: Felipe Balbi
Cc: Anton Vasilyev, Greg Kroah-Hartman, Alan Stern, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] udc: Memory leak on error path and use after free
2017-08-16 13:47 [PATCH] udc: Memory leak on error path and use after free Anton Vasilyev
@ 2017-08-16 15:29 ` Alan Stern
2017-08-16 16:00 ` Anton Vasilyev
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-08-16 15:29 UTC (permalink / raw)
To: Anton Vasilyev
Cc: Felipe Balbi, Greg Kroah-Hartman, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] udc: Memory leak on error path and use after free
2017-08-16 15:29 ` Alan Stern
@ 2017-08-16 16:00 ` Anton Vasilyev
2017-08-16 16:35 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Anton Vasilyev @ 2017-08-16 16:00 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Greg Kroah-Hartman, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] udc: Memory leak on error path and use after free
2017-08-16 16:00 ` Anton Vasilyev
@ 2017-08-16 16:35 ` Alan Stern
2017-08-22 15:45 ` Anton Vasilyev
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-08-16 16:35 UTC (permalink / raw)
To: Anton Vasilyev
Cc: Felipe Balbi, Greg Kroah-Hartman, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] udc: Memory leak on error path and use after free
2017-08-16 16:35 ` Alan Stern
@ 2017-08-22 15:45 ` Anton Vasilyev
2017-08-22 18:37 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Anton Vasilyev @ 2017-08-22 15:45 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Greg Kroah-Hartman, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] udc: Memory leak on error path and use after free
2017-08-22 15:45 ` Anton Vasilyev
@ 2017-08-22 18:37 ` Alan Stern
0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-08-22 18:37 UTC (permalink / raw)
To: Anton Vasilyev
Cc: Felipe Balbi, Greg Kroah-Hartman, Jussi Kivilinna,
Peter Senna Tschudin, Raz Manor, Romain Perier, linux-usb,
linux-kernel, ldv-project
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-22 18:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-16 13:47 [PATCH] udc: Memory leak on error path and use after free Anton Vasilyev
2017-08-16 15:29 ` Alan Stern
2017-08-16 16:00 ` Anton Vasilyev
2017-08-16 16:35 ` Alan Stern
2017-08-22 15:45 ` Anton Vasilyev
2017-08-22 18:37 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®