Hello,
usb_add_gadget_udc_release() gets release() argument that allows to
release user resources.
As far as I can see, the release() is called on error paths
of usb_add_gadget_udc_release() as a result of
put_device(&gadget->dev);
except for the only path going via err1.
As a result a caller of the usb_add_gadget_udc_release() have no chance
to know if the release() was invoked or not.
It may lead to memory leaks (drivers/usb/gadget/udc/snps_udc_core.c)
or to double free (drivers/usb/gadget/udc/fsl_udc_core.c).
Is my reading correct? If so, should we always call release() on error paths?
--
Alexey Khoroshilov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
Hi,
Alexey Khoroshilov <[email protected]> writes:
> Hello,
>
> usb_add_gadget_udc_release() gets release() argument that allows to
> release user resources.
>
> As far as I can see, the release() is called on error paths
> of usb_add_gadget_udc_release() as a result of
> put_device(&gadget->dev);
> except for the only path going via err1.
>
> As a result a caller of the usb_add_gadget_udc_release() have no chance
> to know if the release() was invoked or not.
>
> It may lead to memory leaks (drivers/usb/gadget/udc/snps_udc_core.c)
> or to double free (drivers/usb/gadget/udc/fsl_udc_core.c).
>
> Is my reading correct? If so, should we always call release() on error paths?
unfortunately, it's not :-)
Note that we don't register gadget->dev until later in the code, so
there's nothing to be ->released() that early.
--
balbi
On Wed, 16 Aug 2017, Alexey Khoroshilov wrote:
> Hello,
>
> usb_add_gadget_udc_release() gets release() argument that allows to
> release user resources.
>
> As far as I can see, the release() is called on error paths
> of usb_add_gadget_udc_release() as a result of
> put_device(&gadget->dev);
> except for the only path going via err1.
>
> As a result a caller of the usb_add_gadget_udc_release() have no chance
> to know if the release() was invoked or not.
>
> It may lead to memory leaks (drivers/usb/gadget/udc/snps_udc_core.c)
> or to double free (drivers/usb/gadget/udc/fsl_udc_core.c).
>
> Is my reading correct? If so, should we always call release() on error paths?
How about this (untested)?
Alan Stern
Index: usb-4.x/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/udc/core.c
+++ usb-4.x/drivers/usb/gadget/udc/core.c
@@ -1137,10 +1137,6 @@ int usb_add_gadget_udc_release(struct de
struct usb_udc *udc;
int ret = -ENOMEM;
- udc = kzalloc(sizeof(*udc), GFP_KERNEL);
- if (!udc)
- goto err1;
-
dev_set_name(&gadget->dev, "gadget");
INIT_WORK(&gadget->work, usb_gadget_state_work);
gadget->dev.parent = parent;
@@ -1150,7 +1146,13 @@ int usb_add_gadget_udc_release(struct de
else
gadget->dev.release = usb_udc_nop_release;
- ret = device_register(&gadget->dev);
+ device_initialize(&gadget->dev);
+
+ udc = kzalloc(sizeof(*udc), GFP_KERNEL);
+ if (!udc)
+ goto err1;
+
+ ret = device_add(&gadget->dev);
if (ret)
goto err2;
@@ -1197,10 +1199,10 @@ err3:
device_del(&gadget->dev);
err2:
- put_device(&gadget->dev);
kfree(udc);
err1:
+ put_device(&gadget->dev);
return ret;
}
EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
On 16.08.2017 18:24, Alan Stern wrote:
> On Wed, 16 Aug 2017, Alexey Khoroshilov wrote:
>
>> Hello,
>>
>> usb_add_gadget_udc_release() gets release() argument that allows to
>> release user resources.
>>
>> As far as I can see, the release() is called on error paths
>> of usb_add_gadget_udc_release() as a result of
>> put_device(&gadget->dev);
>> except for the only path going via err1.
>>
>> As a result a caller of the usb_add_gadget_udc_release() have no chance
>> to know if the release() was invoked or not.
>>
>> It may lead to memory leaks (drivers/usb/gadget/udc/snps_udc_core.c)
>> or to double free (drivers/usb/gadget/udc/fsl_udc_core.c).
>>
>> Is my reading correct? If so, should we always call release() on error paths?
>
> How about this (untested)?
>
It looks reasonable. I would only suggest also to make contract
description more explicit, e.g.
/**
* usb_add_gadget_udc_release - adds a new gadget to the udc class
driver list
* @parent: the parent device to this udc. Usually the controller driver's
* device.
* @gadget: the gadget to be added to the list.
* @release: a gadget release function.
*
* Returns zero on success, negative errno otherwise.
+* Calls the gadget release function in the latter case.
*/
--
Alexey Khoroshilov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
The usb_add_gadget_udc_release() routine in the USB gadget core will
sometimes but not always call the gadget's release function when an
error occurs. More specifically, if the struct usb_udc allocation
fails then the release function is not called, and for other errors it
is.
As a result, users of this routine cannot know whether they need to
deallocate the memory containing the gadget structure following an
error. This leads to unavoidable memory leaks or double frees.
This patch fixes the problem by splitting the existing
device_register() call into device_initialize() and device_add(), and
doing the udc allocation in between. That way, even if the allocation
fails it is still possible to call device_del(), and so the release
function will be always called following an error.
Signed-off-by: Alan Stern <[email protected]>
Reported-by: Alexey Khoroshilov <[email protected]>
---
[as1837]
drivers/usb/gadget/udc/core.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Index: usb-4.x/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/udc/core.c
+++ usb-4.x/drivers/usb/gadget/udc/core.c
@@ -1130,6 +1130,7 @@ static int check_pending_gadget_drivers(
* @release: a gadget release function.
*
* Returns zero on success, negative errno otherwise.
+ * Calls the gadget release function in the latter case.
*/
int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
void (*release)(struct device *dev))
@@ -1137,10 +1138,6 @@ int usb_add_gadget_udc_release(struct de
struct usb_udc *udc;
int ret = -ENOMEM;
- udc = kzalloc(sizeof(*udc), GFP_KERNEL);
- if (!udc)
- goto err1;
-
dev_set_name(&gadget->dev, "gadget");
INIT_WORK(&gadget->work, usb_gadget_state_work);
gadget->dev.parent = parent;
@@ -1150,7 +1147,13 @@ int usb_add_gadget_udc_release(struct de
else
gadget->dev.release = usb_udc_nop_release;
- ret = device_register(&gadget->dev);
+ device_initialize(&gadget->dev);
+
+ udc = kzalloc(sizeof(*udc), GFP_KERNEL);
+ if (!udc)
+ goto err1;
+
+ ret = device_add(&gadget->dev);
if (ret)
goto err2;
@@ -1197,10 +1200,10 @@ err3:
device_del(&gadget->dev);
err2:
- put_device(&gadget->dev);
kfree(udc);
err1:
+ put_device(&gadget->dev);
return ret;
}
EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);