diff mbox

[U-Boot,01/10] usb: gadget: don't leak configs when unbinding

Message ID 1441425831-3441-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Stephen Warren Sept. 5, 2015, 4:03 a.m. UTC
From: Stephen Warren <swarren@nvidia.com>

By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
so the free() there does nothing, and the config struct is leaked.
Equally, struct usb_gadget contains a linked list of config structs, so
the code should iterate over them all and free each one, rather than
freeing one particular config struct.

composite_unbind() already iterates over the list of config structs, and
unlinks each from the linked list. Fix this loop to free() each struct as
it's unlinked and otherwise forgotten.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/composite.c | 1 +
 drivers/usb/gadget/g_dnl.c     | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

Comments

Ɓukasz Majewski Sept. 8, 2015, noon UTC | #1
Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
> so the free() there does nothing, and the config struct is leaked.
> Equally, struct usb_gadget contains a linked list of config structs,
> so the code should iterate over them all and free each one, rather
> than freeing one particular config struct.
> 
> composite_unbind() already iterates over the list of config structs,
> and unlinks each from the linked list. Fix this loop to free() each
> struct as it's unlinked and otherwise forgotten.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/gadget/composite.c | 1 +
>  drivers/usb/gadget/g_dnl.c     | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c index d96296cd73b1..a13b21d0a0f2
> 100644 --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -948,6 +948,7 @@ static void composite_unbind(struct usb_gadget
> *gadget) debug("unbind config '%s'/%p\n", c->label, c);
>  			c->unbind(c);
>  		}
> +		free(c);
>  	}
>  	if (composite->unbind)
>  		composite->unbind(cdev);
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index ad89a0d2e670..2fa6da4091e1 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -93,8 +93,6 @@ static int g_dnl_unbind(struct usb_composite_dev
> *cdev) {
>  	struct usb_gadget *gadget = cdev->gadget;
>  
> -	free(cdev->config);
> -	cdev->config = NULL;
>  	debug("%s: calling usb_gadget_disconnect for "
>  			"controller '%s'\n", __func__, gadget->name);
>  	usb_gadget_disconnect(gadget);

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

Tested-by: Lukasz Majewski <l.majewski@samsung.com>
Test HW: Odroid XU3 - Exynos5433.
Stephen Warren Sept. 8, 2015, 9:50 p.m. UTC | #2
On 09/08/2015 05:00 AM, Lukasz Majewski wrote:
> Hi Stephen,
> 
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
>> so the free() there does nothing, and the config struct is leaked.
>> Equally, struct usb_gadget contains a linked list of config structs,
>> so the code should iterate over them all and free each one, rather
>> than freeing one particular config struct.
>>
>> composite_unbind() already iterates over the list of config structs,
>> and unlinks each from the linked list. Fix this loop to free() each
>> struct as it's unlinked and otherwise forgotten.
...
> 
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> 
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>
> Test HW: Odroid XU3 - Exynos5433.

Thanks for all the tests/acks.
Tom Rini Sept. 12, 2015, 12:51 p.m. UTC | #3
On Fri, Sep 04, 2015 at 10:03:42PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
> so the free() there does nothing, and the config struct is leaked.
> Equally, struct usb_gadget contains a linked list of config structs, so
> the code should iterate over them all and free each one, rather than
> freeing one particular config struct.
> 
> composite_unbind() already iterates over the list of config structs, and
> unlinks each from the linked list. Fix this loop to free() each struct as
> it's unlinked and otherwise forgotten.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!
diff mbox

Patch

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index d96296cd73b1..a13b21d0a0f2 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -948,6 +948,7 @@  static void composite_unbind(struct usb_gadget *gadget)
 			debug("unbind config '%s'/%p\n", c->label, c);
 			c->unbind(c);
 		}
+		free(c);
 	}
 	if (composite->unbind)
 		composite->unbind(cdev);
diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index ad89a0d2e670..2fa6da4091e1 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -93,8 +93,6 @@  static int g_dnl_unbind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 
-	free(cdev->config);
-	cdev->config = NULL;
 	debug("%s: calling usb_gadget_disconnect for "
 			"controller '%s'\n", __func__, gadget->name);
 	usb_gadget_disconnect(gadget);