Inconsistency in usb_add_gadget_udc_release() interface

Inconsistency in usb_add_gadget_udc_release() interface

[Alexey Khoroshilov]

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

Re: Inconsistency in usb_add_gadget_udc_release() interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]


Hi,

Alexey Khoroshilov <khoroshilov@ispras.ru> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: Inconsistency in usb_add_gadget_udc_release() interface

[Alan Stern]

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

Re: Inconsistency in usb_add_gadget_udc_release() interface

[Alexey Khoroshilov]

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

[PATCH] USB: Gadget core: fix inconsistency in the interface tousb_add_gadget_udc_release()

[Alan Stern]

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 <stern@rowland.harvard.edu>
Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

---


[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);