staging: ft1000: fix error path

PATCH Superseded Vasiliy Kulikov 4 participants 6 messages Sep 26, 2010 – Oct 3, 2010

Subsystems (2)

Staging Kernel janitors

lore

Share & tools

lore

Vasiliy Kulikov

author

2010-09-26 08:59 UTC #0 lore raw

PATCH 1/1 staging: ft1000: fix error path

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(-)

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +16 -2

index 51ed0dd..3130843 100644
@@ -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;
     }

Signature · 1 line

1.7.0.4

Dan Carpenter

2010-09-26 13:11 UTC #1 lore raw

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c u16 init_ft1000_netdev(struct ft1000_device *ft1000dev) · line 1145

Quoted 5 lines · Vasiliy Kulikov

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +5 -0

+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

Quoted 3 lines

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +1 -0
+	return STATUS_FAILURE;
 }

Belisko Marek

2010-09-26 16:56 UTC #2 lore raw

Quoted 10 lines · Dan Carpenter

Quoted 5 lines · Vasiliy Kulikov
+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.

Quoted 11 lines

Also there should be spaces before and after the ">=".
regards,
dan carpenter
Quoted 3 lines
+ return STATUS_FAILURE;
}

marek

Signature · 10 lines

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

walter harms

2010-09-26 17:18 UTC #3 lore raw

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

Quoted 36 lines · Belisko Marek

Quoted 9 lines · Dan Carpenter
drivers/staging/ft1000/ft1000-usb/ft1000_hw.c u16 init_ft1000_netdev(struct ft1000_device *ft1000dev) · line 1145
Quoted 5 lines · Vasiliy Kulikov
drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +5 -0
+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.
Quoted 9 lines
Also there should be spaces before and after the ">=".
regards,
dan carpenter
Quoted 3 lines
+ return STATUS_FAILURE;
}
marek

Vasiliy Kulikov

author

2010-09-26 19:41 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c u16 init_ft1000_netdev(struct ft1000_device *ft1000dev) · line 1145
Quoted 5 lines · Vasiliy Kulikov
drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +5 -0
+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.

Signature · 1 line

Vasiliy

Vasiliy Kulikov

author

2010-10-03 17:58 UTC #5 lore raw

PATCH v2 1/1 staging: ft1000: fix error path

In reply to Dan Carpenter

Signed-off-by Vasiliy Kulikov <[email protected]>

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

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +19 -2

index 51ed0dd..8609add 100644
@@ -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;
     }

Signature · 1 line

1.7.0.4