[PATCH] staging: ft1000: fix error path

[PATCH] staging: ft1000: fix error path

[Vasiliy Kulikov]

init_ft1000_netdev() calls kfree(netdev) instead of free_netdev(netdev).
It doesn't check kmalloc() return value.
ft1000_read_fifo_reg() doesn't free dr on error and calls kfree(urb)
instead of usb_free_urb(urb).  Also kfree(NULL) is OK.
---
 Compile tested.

 drivers/staging/ft1000/ft1000-usb/ft1000_hw.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
index 51ed0dd..3130843 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
@@ -1048,7 +1048,7 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
         }
         else {
             printk(KERN_ERR "ft1000: Invalid device name\n");
-            kfree(netdev);
+            free_netdev(netdev);
             return STATUS_FAILURE;
         }
     }
@@ -1121,8 +1121,14 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
         for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
             // Get memory for DPRAM_DATA link list
             pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
+	    if (pdpram_blk == NULL)
+	        goto err_free;
             // Get a block of memory to store command data
             pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
+	    if (pdpram_blk->pbuffer == NULL) {
+		kfree(pdpram_blk);
+		goto err_free;
+	    }
             // link provisioning data
             list_add_tail (&pdpram_blk->list, &freercvpool);
         }
@@ -1131,6 +1137,13 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
 
     return STATUS_SUCCESS;
 
+
+err_free:
+	for (i--; i>=0; i--) {
+		kfree(pdpram_blk->pbuffer);
+		kfree(pdpram_blk);
+	}
+	return STATUS_FAILURE;
 }
 
 
@@ -1287,7 +1300,8 @@ static int ft1000_read_fifo_reg(struct ft1000_device *ft1000dev,unsigned int pip
 
     if(!urb || !dr)
     {
-        if(urb) kfree(urb);
+	kfree(dr);
+	usb_free_urb(urb);
         spin_unlock(&ft1000dev->device_lock);
         return -ENOMEM;
     }
-- 
1.7.0.4

Re: [PATCH] staging: ft1000: fix error path

[Dan Carpenter]

On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
> +err_free:
> +	for (i--; i>=0; i--) {
> +		kfree(pdpram_blk->pbuffer);
> +		kfree(pdpram_blk);
> +	}

This is wrong.  I don't have linux-next so I can't see the context, why
are we looping here?  The second iteration through the loop will cause a
NULL dereference.

Also there should be spaces before and after the ">=".

regards,
dan carpenter

> +	return STATUS_FAILURE;
>  }
>  

Re: [PATCH] staging: ft1000: fix error path

[Belisko Marek]

On Sun, Sep 26, 2010 at 3:11 PM, Dan Carpenter <error27@gmail.com> wrote:
> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
>> +err_free:
>> +     for (i--; i>=0; i--) {
>> +             kfree(pdpram_blk->pbuffer);
>> +             kfree(pdpram_blk);
>> +     }
>
> This is wrong.  I don't have linux-next so I can't see the context, why
> are we looping here?  The second iteration through the loop will cause a
> NULL dereference.
Some lines upper there is allocation of structure and it's internal
buffer in loop:
for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
    // Get memory for DPRAM_DATA link list
    pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
    // Get a block of memory to store command data
    pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
    // link provisioning data
    list_add_tail (&pdpram_blk->list, &freercvpool);
}

Free loop is correct in my opinion but kfree should be extended by checking
of NULL pointer because allocation of pdpram_blk could fail and we free also
pdpram_blk->pbuffer.
>
> Also there should be spaces before and after the ">=".
>
> regards,
> dan carpenter
>
>> +     return STATUS_FAILURE;
>>  }
>>
>
>

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

Re: [PATCH] staging: ft1000: fix error path

[walter harms]

hi Marek,
IMHO the alloc sequencs is building a linked list of allocated buffer.
And if  kfree() does no magiclly follow the list this is not working.

re,
 wh


Belisko Marek schrieb:
> On Sun, Sep 26, 2010 at 3:11 PM, Dan Carpenter <error27@gmail.com> wrote:
>> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
>>> +err_free:
>>> +     for (i--; i>=0; i--) {
>>> +             kfree(pdpram_blk->pbuffer);
>>> +             kfree(pdpram_blk);
>>> +     }
>> This is wrong.  I don't have linux-next so I can't see the context, why
>> are we looping here?  The second iteration through the loop will cause a
>> NULL dereference.
> Some lines upper there is allocation of structure and it's internal
> buffer in loop:
> for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
>     // Get memory for DPRAM_DATA link list
>     pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
>     // Get a block of memory to store command data
>     pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
>     // link provisioning data
>     list_add_tail (&pdpram_blk->list, &freercvpool);
> }
> 
> Free loop is correct in my opinion but kfree should be extended by checking
> of NULL pointer because allocation of pdpram_blk could fail and we free also
> pdpram_blk->pbuffer.
>> Also there should be spaces before and after the ">=".
>>
>> regards,
>> dan carpenter
>>
>>> +     return STATUS_FAILURE;
>>>  }
>>>
>>
> 
> marek
> 

Re: [PATCH] staging: ft1000: fix error path

[Vasiliy Kulikov]

On Sun, Sep 26, 2010 at 15:11 +0200, Dan Carpenter wrote:
> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
> > +err_free:
> > +	for (i--; i>=0; i--) {
> > +		kfree(pdpram_blk->pbuffer);
> > +		kfree(pdpram_blk);
> > +	}
> 
> This is wrong.

Of course, I'm very careless.  Pointers are linked into the list and the right code
should loop the list and free both pointers.  I'll post patch v2 tomorrow.

-- 
Vasiliy

[PATCH v2] staging: ft1000: fix error path

[Vasiliy Kulikov]

init_ft1000_netdev() calls kfree(netdev) instead of free_netdev(netdev).
It doesn't check kmalloc() return value.
ft1000_read_fifo_reg() doesn't free dr on error and calls kfree(urb)
instead of usb_free_urb(urb).  Also kfree(NULL) is OK.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 drivers/staging/ft1000/ft1000-usb/ft1000_hw.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
index 51ed0dd..8609add 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
@@ -1013,6 +1013,7 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
     FT1000_INFO *pInfo = NULL;
     PDPRAM_BLK pdpram_blk;
     int i;
+	struct list_head *cur, *tmp;
 
 	gCardIndex=0; //mbelian
 
@@ -1048,7 +1049,7 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
         }
         else {
             printk(KERN_ERR "ft1000: Invalid device name\n");
-            kfree(netdev);
+	    free_netdev(netdev);
             return STATUS_FAILURE;
         }
     }
@@ -1121,8 +1122,14 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
         for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
             // Get memory for DPRAM_DATA link list
             pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
+	    if (pdpram_blk == NULL)
+		goto err_free;
             // Get a block of memory to store command data
             pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
+	    if (pdpram_blk->pbuffer == NULL) {
+		kfree(pdpram_blk);
+		goto err_free;
+	    }
             // link provisioning data
             list_add_tail (&pdpram_blk->list, &freercvpool);
         }
@@ -1131,6 +1138,15 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
 
     return STATUS_SUCCESS;
 
+
+err_free:
+	list_for_each_safe(cur, tmp, &pdpram_blk->list) {
+		pdpram_blk = list_entry(cur, DPRAM_BLK, list);
+		list_del(&pdpram_blk->list);
+		kfree(pdpram_blk->pbuffer);
+		kfree(pdpram_blk);
+	}
+	return STATUS_FAILURE;
 }
 
 
@@ -1287,7 +1303,8 @@ static int ft1000_read_fifo_reg(struct ft1000_device *ft1000dev,unsigned int pip
 
     if(!urb || !dr)
     {
-        if(urb) kfree(urb);
+	kfree(dr);
+	usb_free_urb(urb);
         spin_unlock(&ft1000dev->device_lock);
         return -ENOMEM;
     }
-- 
1.7.0.4