Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference

[Adrian Bunk <bunk@stusta.de>]

On Sun, Nov 20, 2005 at 06:40:55PM -0500, Dave Jones wrote:
> On Mon, Nov 21, 2005 at 12:20:09AM +0100, Adrian Bunk wrote:
>  > The coverity checker spotted that this was a NULL pointer dereference in 
>  > the "if (copy_from_user(...))" case.
>  > 
>  > 
>  > Signed-off-by: Adrian Bunk <bunk@stusta.de>
>  > 
>  > --- linux-2.6.15-rc1-mm2-full/drivers/ieee1394/raw1394.c.old	2005-11-20 22:08:57.000000000 +0100
>  > +++ linux-2.6.15-rc1-mm2-full/drivers/ieee1394/raw1394.c	2005-11-20 22:09:34.000000000 +0100
>  > @@ -2166,7 +2166,8 @@
>  >  			}
>  >  		}
>  >  	}
>  > -	kfree(cache->filled_head);
>  > +	if(cache->filled_head)
>  > +		kfree(cache->filled_head);
>  >  	kfree(cache);
>  >  
>  >  	if (ret >= 0) {
>  > 
> 
> How do we get that far with a NULL filled_head ?
> If the kmalloc that fills cache->filled_head fails, we bail out early above.

The problem is not a NULL filled_head.

The problem is that in the "if (copy_from_user(...))" case, cache has 
already been freed.

But thinking about this, my patch is also wrong and creates a memory 
leak and the real bug is the freeing of cache above.

What about the patch below?

> 		Dave

cu
Adrian


<--  snip  -->


The coverity checker spotted that this was a NULL pointer dereference in 
the "if (copy_from_user(...))" case since the next step is to 
kfree(cache->filled_head).

There's no need to free cache at this point, and it's getting free'd 
later.


Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.15-rc1-mm2-full/drivers/ieee1394/raw1394.c.old	2005-11-20 22:08:57.000000000 +0100
+++ linux-2.6.15-rc1-mm2-full/drivers/ieee1394/raw1394.c	2005-11-21 00:49:38.000000000 +0100
@@ -2131,7 +2131,6 @@
 			   req->req.length)) {
 		csr1212_release_keyval(fi->csr1212_dirs[dr]);
 		fi->csr1212_dirs[dr] = NULL;
-		CSR1212_FREE(cache);
 		ret = -EFAULT;
 	} else {
 		cache->len = req->req.length;