[PATCH] staging/rtl8712: fix potential memory leak

[PATCH] staging/rtl8712: fix potential memory leak

[Bernard Zhao]

This bug is found by google syzbot, the link is:
https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584
memory leak log:
BUG: memory leak
unreferenced object 0xffff88810ff9b3c0 (size 192):
  comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s)
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff  ................
  backtrace:
    [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0
    [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80
    [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210
    [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150
    [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0
    [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1
    [<000000001d449ce2>] usb_probe_interface+0x177/0x370
    [<00000000cd123d34>] really_probe+0x159/0x4a0
    [<00000000364585cc>] driver_probe_device+0x84/0x100
    [<0000000048b74bde>] __device_attach_driver+0xee/0x110
    [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100
    [<00000000bfa9b076>] __device_attach+0x122/0x250
    [<0000000048fe302a>] bus_probe_device+0xc6/0xe0
    [<000000002ceae175>] device_add+0x5be/0xc30
    [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90
    [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0

For this issue，I see that the following call sequence causing
some memory leaks:
usb_probe_interface
 r871xu_drv_init
  r8712_init_drv_sw
   _r8712_init_recv_priv
    r8712_init_recv_priv//void type function
     for (i = 0; i < NR_RECVBUFF;
      if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
       r8712_os_recvbuf_resource_alloc
        precvbuf->purb = usb_alloc_urb
         kmalloc

       break;// if error branch. Here may be some memory leak,
             // break directly after r8712_os_recvbuf_resource_alloc
             // fail, and no cleanup operation is done.

And also the size of the memory leak can be seen in the log is
192 bytes, I check the size of the usb_alloc_urb application is
usb_alloc_urb(0,
 -> kmalloc(struct_size(urb, iso_frame_desc, iso_packets))
  -> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc)
iso_packets is 0, so the size of the actual application is
sizeof(struct urb) -> the calculation result is 192, which matches
the size of the leak point.

After that cleanup, the precvbuf->purb maybe used for long time
So I add kmemleak_not_leak to avoid false positive report.

This patch syzbot test OK:
2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK

Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com>
Signed-off-by: Bernard Zhao <bernard@vivo.com>
---
 drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index 0ffb30f1af7e..8bf8e6d5b005 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -19,6 +19,7 @@
 #include <linux/if_ether.h>
 #include <linux/ip.h>
 #include <net/cfg80211.h>
+#include <linux/kmemleak.h>
 
 #include "osdep_service.h"
 #include "drv_types.h"
@@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
 	for (i = 0; i < NR_RECVBUFF; i++) {
 		INIT_LIST_HEAD(&precvbuf->list);
 		spin_lock_init(&precvbuf->recvbuf_lock);
-		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
+		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) {
+			int j = i;
+
+			while (j-- > 0) {
+				r8712_os_recvbuf_resource_free(padapter, precvbuf);
+				precvbuf--;
+			}
 			break;
+		}
 		precvbuf->ref_cnt = 0;
 		precvbuf->adapter = padapter;
 		list_add_tail(&precvbuf->list,
 			      &(precvpriv->free_recv_buf_queue.queue));
+		kmemleak_not_leak(precvbuf->purb);
 		precvbuf++;
 	}
 	precvpriv->free_recv_buf_queue_cnt = NR_RECVBUFF;
-- 
2.33.1

Re: [PATCH] staging/rtl8712: fix potential memory leak

[Greg Kroah-Hartman]

On Wed, May 11, 2022 at 04:21:44AM -0700, Bernard Zhao wrote:
> This bug is found by google syzbot, the link is:
> https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584
> memory leak log:
> BUG: memory leak
> unreferenced object 0xffff88810ff9b3c0 (size 192):
>   comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff  ................
>   backtrace:
>     [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0
>     [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80
>     [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210
>     [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150
>     [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0
>     [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1
>     [<000000001d449ce2>] usb_probe_interface+0x177/0x370
>     [<00000000cd123d34>] really_probe+0x159/0x4a0
>     [<00000000364585cc>] driver_probe_device+0x84/0x100
>     [<0000000048b74bde>] __device_attach_driver+0xee/0x110
>     [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100
>     [<00000000bfa9b076>] __device_attach+0x122/0x250
>     [<0000000048fe302a>] bus_probe_device+0xc6/0xe0
>     [<000000002ceae175>] device_add+0x5be/0xc30
>     [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90
>     [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0
> 
> For this issue，I see that the following call sequence causing
> some memory leaks:
> usb_probe_interface
>  r871xu_drv_init
>   r8712_init_drv_sw
>    _r8712_init_recv_priv
>     r8712_init_recv_priv//void type function
>      for (i = 0; i < NR_RECVBUFF;
>       if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
>        r8712_os_recvbuf_resource_alloc
>         precvbuf->purb = usb_alloc_urb
>          kmalloc
> 
>        break;// if error branch. Here may be some memory leak,
>              // break directly after r8712_os_recvbuf_resource_alloc
>              // fail, and no cleanup operation is done.
> 
> And also the size of the memory leak can be seen in the log is
> 192 bytes, I check the size of the usb_alloc_urb application is
> usb_alloc_urb(0,
>  -> kmalloc(struct_size(urb, iso_frame_desc, iso_packets))
>   -> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc)
> iso_packets is 0, so the size of the actual application is
> sizeof(struct urb) -> the calculation result is 192, which matches
> the size of the leak point.
> 
> After that cleanup, the precvbuf->purb maybe used for long time
> So I add kmemleak_not_leak to avoid false positive report.
> 
> This patch syzbot test OK:
> 2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK
> 
> Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com>
> Signed-off-by: Bernard Zhao <bernard@vivo.com>

You can not sign off on the same patch by the same person multiple times
as this is a legal statement.

> ---
>  drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
> index 0ffb30f1af7e..8bf8e6d5b005 100644
> --- a/drivers/staging/rtl8712/rtl8712_recv.c
> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> @@ -19,6 +19,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/ip.h>
>  #include <net/cfg80211.h>
> +#include <linux/kmemleak.h>
>  
>  #include "osdep_service.h"
>  #include "drv_types.h"
> @@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
>  	for (i = 0; i < NR_RECVBUFF; i++) {
>  		INIT_LIST_HEAD(&precvbuf->list);
>  		spin_lock_init(&precvbuf->recvbuf_lock);
> -		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
> +		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) {
> +			int j = i;
> +
> +			while (j-- > 0) {
> +				r8712_os_recvbuf_resource_free(padapter, precvbuf);
> +				precvbuf--;
> +			}
>  			break;
> +		}
>  		precvbuf->ref_cnt = 0;
>  		precvbuf->adapter = padapter;
>  		list_add_tail(&precvbuf->list,
>  			      &(precvpriv->free_recv_buf_queue.queue));
> +		kmemleak_not_leak(precvbuf->purb);

This should not be needed, that's an indication that something is really
wrong in the driver.  Where is the urb really freed?

You should not have to say that this urb has not leaked if it really has
not leaked.  Clean it up properly if it needs to be cleaned up here, but
that's not usually where an urb is cleaned up at all.

This feels wrong, sorry.

thanks,

greg k-h

Re:Re: [PATCH] staging/rtl8712: fix potential memory leak

[z]

At 2022-05-11 19:43:30, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote:
>On Wed, May 11, 2022 at 04:21:44AM -0700, Bernard Zhao wrote:
>> This bug is found by google syzbot, the link is:
>> https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584
>> memory leak log:
>> BUG: memory leak
>> unreferenced object 0xffff88810ff9b3c0 (size 192):
>>   comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s)
>>   hex dump (first 32 bytes):
>>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>     00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff  ................
>>   backtrace:
>>     [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0
>>     [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80
>>     [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210
>>     [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150
>>     [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0
>>     [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1
>>     [<000000001d449ce2>] usb_probe_interface+0x177/0x370
>>     [<00000000cd123d34>] really_probe+0x159/0x4a0
>>     [<00000000364585cc>] driver_probe_device+0x84/0x100
>>     [<0000000048b74bde>] __device_attach_driver+0xee/0x110
>>     [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100
>>     [<00000000bfa9b076>] __device_attach+0x122/0x250
>>     [<0000000048fe302a>] bus_probe_device+0xc6/0xe0
>>     [<000000002ceae175>] device_add+0x5be/0xc30
>>     [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90
>>     [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0
>> 
>> For this issue，I see that the following call sequence causing
>> some memory leaks:
>> usb_probe_interface
>>  r871xu_drv_init
>>   r8712_init_drv_sw
>>    _r8712_init_recv_priv
>>     r8712_init_recv_priv//void type function
>>      for (i = 0; i < NR_RECVBUFF;
>>       if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
>>        r8712_os_recvbuf_resource_alloc
>>         precvbuf->purb = usb_alloc_urb
>>          kmalloc
>> 
>>        break;// if error branch. Here may be some memory leak,
>>              // break directly after r8712_os_recvbuf_resource_alloc
>>              // fail, and no cleanup operation is done.
>> 
>> And also the size of the memory leak can be seen in the log is
>> 192 bytes, I check the size of the usb_alloc_urb application is
>> usb_alloc_urb(0,
>>  -> kmalloc(struct_size(urb, iso_frame_desc, iso_packets))
>>   -> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc)
>> iso_packets is 0, so the size of the actual application is
>> sizeof(struct urb) -> the calculation result is 192, which matches
>> the size of the leak point.
>> 
>> After that cleanup, the precvbuf->purb maybe used for long time
>> So I add kmemleak_not_leak to avoid false positive report.
>> 
>> This patch syzbot test OK:
>> 2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK
>> 
>> Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com>
>> Signed-off-by: Bernard Zhao <bernard@vivo.com>
>
>You can not sign off on the same patch by the same person multiple times
>as this is a legal statement.
Hi greg k-h:

Thanks for pointing out my mistake, I will correct it in my future submissions.

>> ---
>>  drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
>> index 0ffb30f1af7e..8bf8e6d5b005 100644
>> --- a/drivers/staging/rtl8712/rtl8712_recv.c
>> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/if_ether.h>
>>  #include <linux/ip.h>
>>  #include <net/cfg80211.h>
>> +#include <linux/kmemleak.h>
>>  
>>  #include "osdep_service.h"
>>  #include "drv_types.h"
>> @@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
>>  	for (i = 0; i < NR_RECVBUFF; i++) {
>>  		INIT_LIST_HEAD(&precvbuf->list);
>>  		spin_lock_init(&precvbuf->recvbuf_lock);
>> -		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
>> +		if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) {
>> +			int j = i;
>> +
>> +			while (j-- > 0) {
>> +				r8712_os_recvbuf_resource_free(padapter, precvbuf);
>> +				precvbuf--;
>> +			}
>>  			break;
>> +		}
>>  		precvbuf->ref_cnt = 0;
>>  		precvbuf->adapter = padapter;
>>  		list_add_tail(&precvbuf->list,
>>  			      &(precvpriv->free_recv_buf_queue.queue));
>> +		kmemleak_not_leak(precvbuf->purb);
>
>This should not be needed, that's an indication that something is really
>wrong in the driver.  Where is the urb really freed?
>
>You should not have to say that this urb has not leaked if it really has
>not leaked.  Clean it up properly if it needs to be cleaned up here, but
>that's not usually where an urb is cleaned up at all.
>
The really free call sequence is done in r8712_free_drv_sw, like the follow error branch:
r871xu_drv_init
	if (status)
		goto dvobj_deinit
			r8712_free_drv_sw
                             _r8712_free_recv_priv
	                           r8712_free_recv_priv
		                         for (i = 0; i < NR_RECVBUFF; i++)
			                        r8712_os_recvbuf_resource_free
				                if (precvbuf->pskb)
					              dev_kfree_skb_any(precvbuf->pskb)
				                if (precvbuf->purb)
					              usb_kill_urb(precvbuf->purb)
					              usb_free_urb(precvbuf->purb)
I checked the  caller's error branch, they call r8712_free_drv_sw to do the cleanup job, i throught the caller is OK.
And my test from sysbot shows that:
Before i add follow code, the memleak is almost 62 times,after my change, the memleak number change to 7.
before
[   93.070089][T10847] kmemleak: 62 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
after fix:
[   77.557355][ T4098] kmemleak: 7 new suspected memory leaks (see /sys/kernel/debug/kmemleak)

I throught the remain 7 is the right use, so i add kmemleak_not_leak.
I am not sure if there is some gap.
Kindly help to correct me if I'm missing something, thanks!

BR//Bernard
>This feels wrong, sorry.
>
>thanks,
>
>greg k-h