Re: [PATCH] usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Minas Harutyunyan]

On 4/16/2018 2:16 PM, Grigor Tovmasyan wrote:
> In dwc2_gadget_init() we allocate EP0 request via
> dwc2_hsotg_ep_alloc_request(). After that there are
> usb_add_gadget_udc() call and if it failed, then
> ctrl_req will not be freed and will cause memory leak.
> 
> Reordered function calls in gadget_init: moved up usb_add_gadget_udc()
> before dwc2_hsotg_ep_alloc_request().
> 
> Tested using kmemleak.
> 
> Cc: Stefan Wahren 
> Signed-off-by: Grigor Tovmasyan 
> ---

Acked-by: Minas Harutyunyan 

>   drivers/usb/dwc2/gadget.c | 8 
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6c32bf26e48e..24000bda5c20 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
>   INIT_LIST_HEAD(>gadget.ep_list);
>   hsotg->gadget.ep0 = >eps_out[0]->ep;
>   
> + ret = usb_add_gadget_udc(dev, >gadget);
> + if (ret)
> + return ret;
> +
>   /* allocate EP0 request */
>   
>   hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(>eps_out[0]->ep,
> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
> epnum, 0);
>   }
>   
> - ret = usb_add_gadget_udc(dev, >gadget);
> - if (ret)
> - return ret;
> -
>   dwc2_hsotg_dump(hsotg);
>   
>   return 0;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Stefan Wahren]

Am 23.04.2018 um 09:05 schrieb Grigor Tovmasyan:

Hi Stefan,

On 4/18/2018 1:11 AM, Stefan Wahren wrote:

Hi Grigor,


Grigor Tovmasyan  hat am 16. April 2018 um 12:16 
geschrieben:


In dwc2_gadget_init() we allocate EP0 request via
dwc2_hsotg_ep_alloc_request(). After that there are
usb_add_gadget_udc() call and if it failed, then
ctrl_req will not be freed and will cause memory leak.

Reordered function calls in gadget_init: moved up usb_add_gadget_udc()
before dwc2_hsotg_ep_alloc_request().

i'm not sure, but doesn't this change introduce a race condition before EP0 
request has been allocated?

As far as I know the firt request to EP0 coming when the function driver
first bind() to the device, which happens after dwc2 probe.
So, in my opinion this patch is safe.

okay fine


Grigor

Stefan


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Grigor Tovmasyan]

Hi Stefan,

On 4/18/2018 1:11 AM, Stefan Wahren wrote:
> Hi Grigor,
> 
>> Grigor Tovmasyan  hat am 16. April 2018 um 
>> 12:16 geschrieben:
>>
>>
>> In dwc2_gadget_init() we allocate EP0 request via
>> dwc2_hsotg_ep_alloc_request(). After that there are
>> usb_add_gadget_udc() call and if it failed, then
>> ctrl_req will not be freed and will cause memory leak.
>>
>> Reordered function calls in gadget_init: moved up usb_add_gadget_udc()
>> before dwc2_hsotg_ep_alloc_request().
> 
> i'm not sure, but doesn't this change introduce a race condition before EP0 
> request has been allocated?
As far as I know the firt request to EP0 coming when the function driver 
first bind() to the device, which happens after dwc2 probe.
So, in my opinion this patch is safe.

Grigor
> 
> Stefan
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Stefan Wahren]

Hi Grigor,

> Grigor Tovmasyan  hat am 16. April 2018 um 
> 12:16 geschrieben:
> 
> 
> In dwc2_gadget_init() we allocate EP0 request via
> dwc2_hsotg_ep_alloc_request(). After that there are
> usb_add_gadget_udc() call and if it failed, then
> ctrl_req will not be freed and will cause memory leak.
> 
> Reordered function calls in gadget_init: moved up usb_add_gadget_udc()
> before dwc2_hsotg_ep_alloc_request().

i'm not sure, but doesn't this change introduce a race condition before EP0 
request has been allocated?

Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Grigor Tovmasyan]

In dwc2_gadget_init() we allocate EP0 request via
dwc2_hsotg_ep_alloc_request(). After that there are
usb_add_gadget_udc() call and if it failed, then
ctrl_req will not be freed and will cause memory leak.

Reordered function calls in gadget_init: moved up usb_add_gadget_udc()
before dwc2_hsotg_ep_alloc_request().

Tested using kmemleak.

Cc: Stefan Wahren 
Signed-off-by: Grigor Tovmasyan 
---
 drivers/usb/dwc2/gadget.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6c32bf26e48e..24000bda5c20 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
INIT_LIST_HEAD(>gadget.ep_list);
hsotg->gadget.ep0 = >eps_out[0]->ep;
 
+   ret = usb_add_gadget_udc(dev, >gadget);
+   if (ret)
+   return ret;
+
/* allocate EP0 request */
 
hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(>eps_out[0]->ep,
@@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
  epnum, 0);
}
 
-   ret = usb_add_gadget_udc(dev, >gadget);
-   if (ret)
-   return ret;
-
dwc2_hsotg_dump(hsotg);
 
return 0;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html