usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

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 <stefan.wahren@i2se.com>
Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com>
---
 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(&hsotg->gadget.ep_list);
 	hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep;
 
+	ret = usb_add_gadget_udc(dev, &hsotg->gadget);
+	if (ret)
+		return ret;
+
 	/* allocate EP0 request */
 
 	hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep,
@@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
 					  epnum, 0);
 	}
 
-	ret = usb_add_gadget_udc(dev, &hsotg->gadget);
-	if (ret)
-		return ret;
-
 	dwc2_hsotg_dump(hsotg);
 
 	return 0;

usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Felipe Balbi]

Hi,

Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> writes:
> Hi Felipe,
>
> Please drop this patch from your next branch.
> I will provide another fix based on Marek's suggestion.

this time I'll rebase my 'next' branch. Next time, make sure this
doesn't happen anymore as I like my 'next' to be immutable.

usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Grigor Tovmasyan]

Hi Felipe,

Please drop this patch from your next branch.
I will provide another fix based on Marek's suggestion.

Thanks,
Grigor.

On 5/17/2018 4:18 PM, Marek Szyprowski wrote:
> Hi All,
> 
> I've just noticed this patch has been applied to linux-next.
> 
> It breaks DWC2 driver initialization on Samsung Exynos-based boards
> I've use for kernel daily tests.
> 
> Here is an example of the call stack:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> pgd = (ptrval)
> [0000000c] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 0 PID: 25 Comm: kworker/0:1 Not tainted 4.17.0-rc5-next-20180517 #782
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: events deferred_probe_work_func
> PC is at usb_ep_alloc_request+0x1c/0x200
> LR is at composite_dev_prepare+0x20/0xec
> pc : [<c06976a8>]    lr : [<c0694744>]    psr: 60000153
> sp : d95f1d10  ip : 00000000  fp : 00000002
> r10: c1072d58  r9 : d82443d0  r8 : 00000000
> r7 : c1074180  r6 : c10ad460  r5 : c100746c  r4 : d8253980
> r3 : 00000000  r2 : d82539b8  r1 : 014000c0  r0 : d82443d0
> Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 4000406a  DAC: 00000051
> Process kworker/0:1 (pid: 25, stack limit = 0x(ptrval))
> Stack: (0xd95f1d10 to 0xd95f2000)
> ...
> [<c06976a8>] (usb_ep_alloc_request) from [<c0694744>]
> (composite_dev_prepare+0x20/0xec)
> [<c0694744>] (composite_dev_prepare) from [<c0694b4c>]
> (composite_bind+0x7c/0x1c8)
> [<c0694b4c>] (composite_bind) from [<c069a8e0>]
> (udc_bind_to_driver+0x74/0x134)
> [<c069a8e0>] (udc_bind_to_driver) from [<c069aa14>]
> (check_pending_gadget_drivers+0x74/0xac)
> [<c069aa14>] (check_pending_gadget_drivers) from [<c069abdc>]
> (usb_add_gadget_udc_release+0x190/0x1ec)
> [<c069abdc>] (usb_add_gadget_udc_release) from [<c0655a84>]
> (dwc2_gadget_init+0x274/0x464)
> [<c0655a84>] (dwc2_gadget_init) from [<c0642fb0>]
> (dwc2_driver_probe+0x488/0x50c)
> [<c0642fb0>] (dwc2_driver_probe) from [<c057a1e4>]
> (platform_drv_probe+0x48/0x9c)
> [<c057a1e4>] (platform_drv_probe) from [<c0577d54>]
> (driver_probe_device+0x2dc/0x4ac)
> [<c0577d54>] (driver_probe_device) from [<c0575d54>]
> (bus_for_each_drv+0x74/0xb8)
> [<c0575d54>] (bus_for_each_drv) from [<c057792c>]
> (__device_attach+0xd4/0x168)
> [<c057792c>] (__device_attach) from [<c0576bf8>]
> (bus_probe_device+0x88/0x90)
> [<c0576bf8>] (bus_probe_device) from [<c05771b0>]
> (deferred_probe_work_func+0x60/0x180)
> [<c05771b0>] (deferred_probe_work_func) from ad+0x28c/0x58c)
> [<c0147f2c>] (worker_thread) from [<c014dc5c>] (kthread+0x138/0x168)
> [<c014dc5c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> 
> 
> A proper fix for the potential memory leak is to add free to error path
> instead of reordering usb_add_gadget_udc() call. The issue happens if DWC2
> driver is initialized from deferred probe (in such case the gadget driver
> is already registered).
> 
> Felipe: please drop or revert this patch in your -next branch.
> 
> 
> Best regards
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
> 
> On 2018-04-16 12:16, 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 <stefan.wahren@i2se.com>
>> Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com>
>> Acked-by: Minas Harutyunyan <hminas@synopsys.com>
>> ---
>>    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(&hsotg->gadget.ep_list);
>>    	hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep;
>>    
>> +	ret = usb_add_gadget_udc(dev, &hsotg->gadget);
>> +	if (ret)
>> +		return ret;
>> +
>>    	/* allocate EP0 request */
>>    
>>    	hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep,
>> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
>>    					  epnum, 0);
>>    	}
>>    
>> -	ret = usb_add_gadget_udc(dev, &hsotg->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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Marek Szyprowski]

Hi All,

I've just noticed this patch has been applied to linux-next.

It breaks DWC2 driver initialization on Samsung Exynos-based boards
I've use for kernel daily tests.

Here is an example of the call stack:

Unable to handle kernel NULL pointer dereference at virtual address 0000000c
pgd = (ptrval)
[0000000c] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 25 Comm: kworker/0:1 Not tainted 4.17.0-rc5-next-20180517 #782
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: events deferred_probe_work_func
PC is at usb_ep_alloc_request+0x1c/0x200
LR is at composite_dev_prepare+0x20/0xec
pc : [<c06976a8>]    lr : [<c0694744>]    psr: 60000153
sp : d95f1d10  ip : 00000000  fp : 00000002
r10: c1072d58  r9 : d82443d0  r8 : 00000000
r7 : c1074180  r6 : c10ad460  r5 : c100746c  r4 : d8253980
r3 : 00000000  r2 : d82539b8  r1 : 014000c0  r0 : d82443d0
Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000406a  DAC: 00000051
Process kworker/0:1 (pid: 25, stack limit = 0x(ptrval))
Stack: (0xd95f1d10 to 0xd95f2000)
...
[<c06976a8>] (usb_ep_alloc_request) from [<c0694744>] 
(composite_dev_prepare+0x20/0xec)
[<c0694744>] (composite_dev_prepare) from [<c0694b4c>] 
(composite_bind+0x7c/0x1c8)
[<c0694b4c>] (composite_bind) from [<c069a8e0>] 
(udc_bind_to_driver+0x74/0x134)
[<c069a8e0>] (udc_bind_to_driver) from [<c069aa14>] 
(check_pending_gadget_drivers+0x74/0xac)
[<c069aa14>] (check_pending_gadget_drivers) from [<c069abdc>] 
(usb_add_gadget_udc_release+0x190/0x1ec)
[<c069abdc>] (usb_add_gadget_udc_release) from [<c0655a84>] 
(dwc2_gadget_init+0x274/0x464)
[<c0655a84>] (dwc2_gadget_init) from [<c0642fb0>] 
(dwc2_driver_probe+0x488/0x50c)
[<c0642fb0>] (dwc2_driver_probe) from [<c057a1e4>] 
(platform_drv_probe+0x48/0x9c)
[<c057a1e4>] (platform_drv_probe) from [<c0577d54>] 
(driver_probe_device+0x2dc/0x4ac)
[<c0577d54>] (driver_probe_device) from [<c0575d54>] 
(bus_for_each_drv+0x74/0xb8)
[<c0575d54>] (bus_for_each_drv) from [<c057792c>] 
(__device_attach+0xd4/0x168)
[<c057792c>] (__device_attach) from [<c0576bf8>] 
(bus_probe_device+0x88/0x90)
[<c0576bf8>] (bus_probe_device) from [<c05771b0>] 
(deferred_probe_work_func+0x60/0x180)
[<c05771b0>] (deferred_probe_work_func) from ad+0x28c/0x58c)
[<c0147f2c>] (worker_thread) from [<c014dc5c>] (kthread+0x138/0x168)
[<c014dc5c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)


A proper fix for the potential memory leak is to add free to error path
instead of reordering usb_add_gadget_udc() call. The issue happens if DWC2
driver is initialized from deferred probe (in such case the gadget driver
is already registered).

Felipe: please drop or revert this patch in your -next branch.


Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland

On 2018-04-16 12:16, 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 <stefan.wahren@i2se.com>
> Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com>
> Acked-by: Minas Harutyunyan <hminas@synopsys.com>
> ---
>   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(&hsotg->gadget.ep_list);
>   	hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep;
>   
> +	ret = usb_add_gadget_udc(dev, &hsotg->gadget);
> +	if (ret)
> +		return ret;
> +
>   	/* allocate EP0 request */
>   
>   	hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep,
> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
>   					  epnum, 0);
>   	}
>   
> -	ret = usb_add_gadget_udc(dev, &hsotg->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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <stefan.wahren@i2se.com>
> Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com>
> ---

Acked-by: Minas Harutyunyan <hminas@synopsys.com>

>   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(&hsotg->gadget.ep_list);
>   	hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep;
>   
> +	ret = usb_add_gadget_udc(dev, &hsotg->gadget);
> +	if (ret)
> +		return ret;
> +
>   	/* allocate EP0 request */
>   
>   	hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep,
> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
>   					  epnum, 0);
>   	}
>   
> -	ret = usb_add_gadget_udc(dev, &hsotg->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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <Grigor.Tovmasyan@synopsys.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <Grigor.Tovmasyan@synopsys.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc2: gadget: Fix memory leak in dwc2_gadget_init()

[Stefan Wahren]

Hi Grigor,

> Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html