[PATCH] staging: rtl8192u: add error handling for usb_alloc_urb

[PATCH] staging: rtl8192u: add error handling for usb_alloc_urb

[Zhouyang Jia]

When usb_alloc_urb fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling usb_alloc_urb.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
 drivers/staging/rtl8192u/r8192U_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 7a0dbc0..3f09615 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1666,6 +1666,9 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 		void *oldaddr, *newaddr;
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!priv->rx_urb[16])
+			return -ENOMEM;
+
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
 		if (!priv->oldaddr)
 			return -ENOMEM;
-- 
2.7.4

Re: [PATCH] staging: rtl8192u: add error handling for usb_alloc_urb

[Greg Kroah-Hartman]

On Mon, Jun 11, 2018 at 04:31:11PM +0800, Zhouyang Jia wrote:
> When usb_alloc_urb fails, the lack of error-handling code may
> cause unexpected results.
> 
> This patch adds error-handling code after calling usb_alloc_urb.
> 
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 7a0dbc0..3f09615 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1666,6 +1666,9 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>  		void *oldaddr, *newaddr;
>  
>  		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!priv->rx_urb[16])
> +			return -ENOMEM;
> +

You just leaked memory :(

Well, this whole function leaks memory on the error paths, like here:

>  		priv->oldaddr = kmalloc(16, GFP_KERNEL);
>  		if (!priv->oldaddr)
>  			return -ENOMEM;

So can you fix this all up at the same time?

thanks,

greg k-h

[PATCH v2] staging: rtl8192u: add error handling for usb_alloc_urb

[Zhouyang Jia]

When usb_alloc_urb fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling usb_alloc_urb.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
v1->v2:
- Fix memory leak.
---
 drivers/staging/rtl8192u/r8192U_core.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 7a0dbc0..6afab4e 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1648,13 +1648,17 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 #ifndef JACKSON_NEW_RX
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
-		if (!priv->rx_urb[i])
+		if (!priv->rx_urb[i]) {
+			kfree(priv->rx_urb);
 			return -ENOMEM;
+		}
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
-		if (!priv->rx_urb[i]->transfer_buffer)
+		if (!priv->rx_urb[i]->transfer_buffer) {
+			kfree(priv->rx_urb);
 			return -ENOMEM;
+		}
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}
@@ -1666,9 +1670,17 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 		void *oldaddr, *newaddr;
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!priv->rx_urb[16]) {
+			kfree(priv->rx_urb);
+			return -ENOMEM;
+		}
+
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
-		if (!priv->oldaddr)
+		if (!priv->oldaddr) {
+			kfree(priv->rx_urb);
 			return -ENOMEM;
+		}
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {
-- 
2.7.4

Re: [PATCH v2] staging: rtl8192u: add error handling for usb_alloc_urb

[Greg Kroah-Hartman]

On Sat, Jun 16, 2018 at 12:25:23AM +0800, Zhouyang Jia wrote:
> When usb_alloc_urb fails, the lack of error-handling code may
> cause unexpected results.
> 
> This patch adds error-handling code after calling usb_alloc_urb.
> 
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
> v1->v2:
> - Fix memory leak.
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 7a0dbc0..6afab4e 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1648,13 +1648,17 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>  #ifndef JACKSON_NEW_RX
>  	for (i = 0; i < (MAX_RX_URB + 1); i++) {
>  		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> -		if (!priv->rx_urb[i])
> +		if (!priv->rx_urb[i]) {
> +			kfree(priv->rx_urb);
>  			return -ENOMEM;
> +		}

{sigh}

No, you are still leaking memory on all of these changes that you just
made :(

greg k-h

Re: [PATCH v2] staging: rtl8192u: add error handling for usb_alloc_urb

[Kees Cook]

On Fri, Jun 15, 2018 at 9:33 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Jun 16, 2018 at 12:25:23AM +0800, Zhouyang Jia wrote:
>> When usb_alloc_urb fails, the lack of error-handling code may
>> cause unexpected results.
>>
>> This patch adds error-handling code after calling usb_alloc_urb.
>>
>> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
>> ---
>> v1->v2:
>> - Fix memory leak.
>> ---
>>  drivers/staging/rtl8192u/r8192U_core.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
>> index 7a0dbc0..6afab4e 100644
>> --- a/drivers/staging/rtl8192u/r8192U_core.c
>> +++ b/drivers/staging/rtl8192u/r8192U_core.c
>> @@ -1648,13 +1648,17 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>>  #ifndef JACKSON_NEW_RX
>>       for (i = 0; i < (MAX_RX_URB + 1); i++) {
>>               priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
>> -             if (!priv->rx_urb[i])
>> +             if (!priv->rx_urb[i]) {
>> +                     kfree(priv->rx_urb);

You're freeing rx_urb, which holds all the pointers to allocated
memory. You'll need to free each item of the array before freeing the
array itself:

for (i = 0; i < (MAX_RX_URB + 1); i++)
    kfree(priv->rx_urb[i]);
kfree(priv->rx_urb);

I think you need some kind of helper to do this, and you can call into
it from your error paths...

-Kees

> {sigh}
>
> No, you are still leaking memory on all of these changes that you just
> made :(
>
> greg k-h



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] staging: rtl8192u: add error handling for usb_alloc_urb

[Kees Cook]

On Fri, Jun 15, 2018 at 9:47 AM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jun 15, 2018 at 9:33 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Sat, Jun 16, 2018 at 12:25:23AM +0800, Zhouyang Jia wrote:
>>> When usb_alloc_urb fails, the lack of error-handling code may
>>> cause unexpected results.
>>>
>>> This patch adds error-handling code after calling usb_alloc_urb.
>>>
>>> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
>>> ---
>>> v1->v2:
>>> - Fix memory leak.
>>> ---
>>>  drivers/staging/rtl8192u/r8192U_core.c | 18 +++++++++++++++---
>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
>>> index 7a0dbc0..6afab4e 100644
>>> --- a/drivers/staging/rtl8192u/r8192U_core.c
>>> +++ b/drivers/staging/rtl8192u/r8192U_core.c
>>> @@ -1648,13 +1648,17 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>>>  #ifndef JACKSON_NEW_RX
>>>       for (i = 0; i < (MAX_RX_URB + 1); i++) {
>>>               priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
>>> -             if (!priv->rx_urb[i])
>>> +             if (!priv->rx_urb[i]) {
>>> +                     kfree(priv->rx_urb);
>
> You're freeing rx_urb, which holds all the pointers to allocated
> memory. You'll need to free each item of the array before freeing the
> array itself:
>
> for (i = 0; i < (MAX_RX_URB + 1); i++)
>     kfree(priv->rx_urb[i]);
> kfree(priv->rx_urb);
>
> I think you need some kind of helper to do this, and you can call into
> it from your error paths...

(Though if you do this, rx_urb must be zero-initialized, so change the
kmalloc_array() to kcalloc()...)

-Kees

>
> -Kees
>
>> {sigh}
>>
>> No, you are still leaking memory on all of these changes that you just
>> made :(
>>
>> greg k-h
>
>
>
> --
> Kees Cook
> Pixel Security



-- 
Kees Cook
Pixel Security

[PATCH v3] staging: rtl8192u: add error handling for usb_alloc_urb

[Zhouyang Jia]

When usb_alloc_urb fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling usb_alloc_urb.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
v1->v2:
- Fix memory leak.
v2->v3:
- Release memory in error path.
---
 drivers/staging/rtl8192u/r8192U_core.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 7a0dbc0..1c980e9 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1649,12 +1649,12 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!priv->rx_urb[i])
-			return -ENOMEM;
+			goto out_release_mem;
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
 		if (!priv->rx_urb[i]->transfer_buffer)
-			return -ENOMEM;
+			goto out_release_mem;
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}
@@ -1666,9 +1666,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 		void *oldaddr, *newaddr;
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!priv->rx_urb[16])
+			goto out_release_mem;
+
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
 		if (!priv->oldaddr)
-			return -ENOMEM;
+			goto out_release_mem;
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {
@@ -1686,17 +1690,19 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
 				 GFP_KERNEL);
 	if (!priv->pp_rxskb) {
-		kfree(priv->rx_urb);
-
-		priv->pp_rxskb = NULL;
-		priv->rx_urb = NULL;
-
 		DMESGE("Endpoint Alloc Failure");
-		return -ENOMEM;
+		goto out_release_mem;
 	}
 
 	netdev_dbg(dev, "End of initendpoints\n");
 	return 0;
+
+out_release_mem:
+	for (i = 0; i < (MAX_RX_URB + 1); i++)
+		kfree(priv->rx_urb[i]);
+	kfree(priv->rx_urb);
+	priv->rx_urb = NULL;
+	return -ENOMEM;
 }
 
 #ifdef THOMAS_BEACON
-- 
2.7.4

Re: [PATCH v3] staging: rtl8192u: add error handling for usb_alloc_urb

[kbuild test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.17 next-20180615]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/linux-kernel-owner-vger-kernel-org/staging-rtl8192u-add-error-handling-for-usb_alloc_urb/20180616-012944
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/staging//rtl8192u/r8192U_core.c: In function 'rtl8192_usb_initendpoints':
>> drivers/staging//rtl8192u/r8192U_core.c:1701:7: error: 'i' undeclared (first use in this function)
     for (i = 0; i < (MAX_RX_URB + 1); i++)
          ^
   drivers/staging//rtl8192u/r8192U_core.c:1701:7: note: each undeclared identifier is reported only once for each function it appears in

vim +/i +1701 drivers/staging//rtl8192u/r8192U_core.c

  1688	
  1689		memset(priv->rx_urb, 0, sizeof(struct urb *) * MAX_RX_URB);
  1690		priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
  1691					 GFP_KERNEL);
  1692		if (!priv->pp_rxskb) {
  1693			DMESGE("Endpoint Alloc Failure");
  1694			goto out_release_mem;
  1695		}
  1696	
  1697		netdev_dbg(dev, "End of initendpoints\n");
  1698		return 0;
  1699	
  1700	out_release_mem:
> 1701		for (i = 0; i < (MAX_RX_URB + 1); i++)
  1702			kfree(priv->rx_urb[i]);
  1703		kfree(priv->rx_urb);
  1704		priv->rx_urb = NULL;
  1705		return -ENOMEM;
  1706	}
  1707	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54140 bytes --]

Re: [PATCH v3] staging: rtl8192u: add error handling for usb_alloc_urb

[Kees Cook]

On Fri, Jun 15, 2018 at 10:28 AM, Zhouyang Jia <jiazhouyang09@gmail.com> wrote:
> When usb_alloc_urb fails, the lack of error-handling code may
> cause unexpected results.
>
> This patch adds error-handling code after calling usb_alloc_urb.
>
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
> v1->v2:
> - Fix memory leak.
> v2->v3:
> - Release memory in error path.
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 7a0dbc0..1c980e9 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1649,12 +1649,12 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>         for (i = 0; i < (MAX_RX_URB + 1); i++) {
>                 priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
>                 if (!priv->rx_urb[i])
> -                       return -ENOMEM;
> +                       goto out_release_mem;

You need to use kcalloc() above here for priv->rx_urb, otherwise you
may free random garbage. :)

-Kees

>
>                 priv->rx_urb[i]->transfer_buffer =
>                         kmalloc(RX_URB_SIZE, GFP_KERNEL);
>                 if (!priv->rx_urb[i]->transfer_buffer)
> -                       return -ENOMEM;
> +                       goto out_release_mem;
>
>                 priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
>         }
> @@ -1666,9 +1666,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>                 void *oldaddr, *newaddr;
>
>                 priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
> +               if (!priv->rx_urb[16])
> +                       goto out_release_mem;
> +
>                 priv->oldaddr = kmalloc(16, GFP_KERNEL);
>                 if (!priv->oldaddr)
> -                       return -ENOMEM;
> +                       goto out_release_mem;
> +
>                 oldaddr = priv->oldaddr;
>                 align = ((long)oldaddr) & 3;
>                 if (align) {
> @@ -1686,17 +1690,19 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>         priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
>                                  GFP_KERNEL);
>         if (!priv->pp_rxskb) {
> -               kfree(priv->rx_urb);
> -
> -               priv->pp_rxskb = NULL;
> -               priv->rx_urb = NULL;
> -
>                 DMESGE("Endpoint Alloc Failure");
> -               return -ENOMEM;
> +               goto out_release_mem;
>         }
>
>         netdev_dbg(dev, "End of initendpoints\n");
>         return 0;
> +
> +out_release_mem:
> +       for (i = 0; i < (MAX_RX_URB + 1); i++)
> +               kfree(priv->rx_urb[i]);
> +       kfree(priv->rx_urb);
> +       priv->rx_urb = NULL;
> +       return -ENOMEM;
>  }
>
>  #ifdef THOMAS_BEACON
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

[PATCH v4] staging: rtl8192u: add error handling for usb_alloc_urb

[Zhouyang Jia]

When usb_alloc_urb fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling usb_alloc_urb.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
v1->v2:
- Fix memory leak.
v2->v3:
- Release memory in error path.
v3->v4:
- Use kcalloc instead of kmalloc_array.
---
 drivers/staging/rtl8192u/r8192U_core.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 7a0dbc0..d15ee4f 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1639,8 +1639,9 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
 static short rtl8192_usb_initendpoints(struct net_device *dev)
 {
 	struct r8192_priv *priv = ieee80211_priv(dev);
+	int i;
 
-	priv->rx_urb = kmalloc(sizeof(struct urb *) * (MAX_RX_URB + 1),
+	priv->rx_urb = kcalloc(MAX_RX_URB + 1, sizeof(struct urb *),
 			       GFP_KERNEL);
 	if (!priv->rx_urb)
 		return -ENOMEM;
@@ -1649,12 +1650,12 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!priv->rx_urb[i])
-			return -ENOMEM;
+			goto out_release_mem;
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
 		if (!priv->rx_urb[i]->transfer_buffer)
-			return -ENOMEM;
+			goto out_release_mem;
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}
@@ -1666,9 +1667,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 		void *oldaddr, *newaddr;
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!priv->rx_urb[16])
+			goto out_release_mem;
+
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
 		if (!priv->oldaddr)
-			return -ENOMEM;
+			goto out_release_mem;
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {
@@ -1686,17 +1691,19 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
 				 GFP_KERNEL);
 	if (!priv->pp_rxskb) {
-		kfree(priv->rx_urb);
-
-		priv->pp_rxskb = NULL;
-		priv->rx_urb = NULL;
-
 		DMESGE("Endpoint Alloc Failure");
-		return -ENOMEM;
+		goto out_release_mem;
 	}
 
 	netdev_dbg(dev, "End of initendpoints\n");
 	return 0;
+
+out_release_mem:
+	for (i = 0; i < (MAX_RX_URB + 1); i++)
+		kfree(priv->rx_urb[i]);
+	kfree(priv->rx_urb);
+	priv->rx_urb = NULL;
+	return -ENOMEM;
 }
 
 #ifdef THOMAS_BEACON
-- 
2.7.4

Re: [PATCH v4] staging: rtl8192u: add error handling for usb_alloc_urb

[Dan Carpenter]

I was actually OK with v1 on the theory that everything else leaked and
so this didn't really introduce anything new... :P

On Sat, Jun 16, 2018 at 10:01:22AM +0800, Zhouyang Jia wrote:
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1639,8 +1639,9 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
>  static short rtl8192_usb_initendpoints(struct net_device *dev)
>  {
>  	struct r8192_priv *priv = ieee80211_priv(dev);
> +	int i;
>  
> -	priv->rx_urb = kmalloc(sizeof(struct urb *) * (MAX_RX_URB + 1),
> +	priv->rx_urb = kcalloc(MAX_RX_URB + 1, sizeof(struct urb *),
>  			       GFP_KERNEL);
>  	if (!priv->rx_urb)
>  		return -ENOMEM;
> @@ -1649,12 +1650,12 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
>  	for (i = 0; i < (MAX_RX_URB + 1); i++) {
>  		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
>  		if (!priv->rx_urb[i])
> -			return -ENOMEM;
> +			goto out_release_mem;
>  
>  		priv->rx_urb[i]->transfer_buffer =
>  			kmalloc(RX_URB_SIZE, GFP_KERNEL);


You need to free priv->rx_urb[i]->transfer_buffer as well and there are
several other resources which are also not freed.

regards,
dan carpenter

[PATCH v5] staging: rtl8192u: add error handling for usb_alloc_urb

[Zhouyang Jia]

When usb_alloc_urb fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling usb_alloc_urb,
and fixes memory leaks in error paths.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
v1->v2:
- Fix memory leak.
v2->v3:
- Release memory in error path.
v3->v4:
- Use kcalloc instead of kmalloc_array.
v4->v5:
- Free priv->rx_urb[i]->transfer_buffer and priv->oldaddr.
---
 drivers/staging/rtl8192u/r8192U_core.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 7a0dbc0..9413f29 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -1639,8 +1639,9 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
 static short rtl8192_usb_initendpoints(struct net_device *dev)
 {
 	struct r8192_priv *priv = ieee80211_priv(dev);
+	int i;
 
-	priv->rx_urb = kmalloc(sizeof(struct urb *) * (MAX_RX_URB + 1),
+	priv->rx_urb = kcalloc(MAX_RX_URB + 1, sizeof(struct urb *),
 			       GFP_KERNEL);
 	if (!priv->rx_urb)
 		return -ENOMEM;
@@ -1649,12 +1650,12 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!priv->rx_urb[i])
-			return -ENOMEM;
+			goto out_release_urb;
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
 		if (!priv->rx_urb[i]->transfer_buffer)
-			return -ENOMEM;
+			goto out_release_urb;
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}
@@ -1666,9 +1667,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 		void *oldaddr, *newaddr;
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!priv->rx_urb[16])
+			goto out_release_urb;
+
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
 		if (!priv->oldaddr)
-			return -ENOMEM;
+			goto out_release_urb;
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {
@@ -1686,17 +1691,26 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 	priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
 				 GFP_KERNEL);
 	if (!priv->pp_rxskb) {
-		kfree(priv->rx_urb);
-
-		priv->pp_rxskb = NULL;
-		priv->rx_urb = NULL;
-
 		DMESGE("Endpoint Alloc Failure");
-		return -ENOMEM;
+		goto out_release_oldaddr;
 	}
 
 	netdev_dbg(dev, "End of initendpoints\n");
 	return 0;
+
+out_release_oldaddr:
+	kfree(priv->oldaddr);
+
+out_release_urb:
+	for (i = 0; i < (MAX_RX_URB + 1); i++) {
+		if (priv->rx_urb[i]) {
+			kfree(priv->rx_urb[i]->transfer_buffer);
+			kfree(priv->rx_urb[i]);
+		}
+	}
+	kfree(priv->rx_urb);
+	priv->rx_urb = NULL;
+	return -ENOMEM;
 }
 
 #ifdef THOMAS_BEACON
-- 
2.7.4