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

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

[Adrian Bunk]

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

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

[Jody McIntyre]

On Mon, Nov 21, 2005 at 12:20:09AM +0100, Adrian Bunk wrote:
> +	if(cache->filled_head)
> +		kfree(cache->filled_head);

Try again.  kfree() of a NULL pointer is perfectly fine.

Jody

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

[Adrian Bunk]

On Sun, Nov 20, 2005 at 06:33:51PM -0500, Jody McIntyre wrote:
> On Mon, Nov 21, 2005 at 12:20:09AM +0100, Adrian Bunk wrote:
> > +	if(cache->filled_head)
> > +		kfree(cache->filled_head);
> 
> Try again.  kfree() of a NULL pointer is perfectly fine.

The problem is that cache is NULL...

> Jody

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

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

[Adrian Bunk]

On Mon, Nov 21, 2005 at 12:46:12AM +0100, Adrian Bunk wrote:
> On Sun, Nov 20, 2005 at 06:33:51PM -0500, Jody McIntyre wrote:
> > On Mon, Nov 21, 2005 at 12:20:09AM +0100, Adrian Bunk wrote:
> > > +	if(cache->filled_head)
> > > +		kfree(cache->filled_head);
> > 
> > Try again.  kfree() of a NULL pointer is perfectly fine.
> 
> The problem is that cache is NULL...

And my patch didn't fix this...

What about the second try of my patch?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

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

[Dave Jones]

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.

		Dave

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

[Adrian Bunk]

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;

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

[Stefan Richter]

Adrian Bunk wrote:
[...]
> 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;

This looks OK to me. But there seems to be another bug. I think the line

	kfree(cache);

after the if and else blocks has to be replaced by

	CSR1212_FREE(cache);

-- 
Stefan Richter
-=====-=-=-= =-== =-=-=
http://arcgraph.de/sr/

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

[Ben Collins]

> >-		CSR1212_FREE(cache);
> > 		ret = -EFAULT;
> > 	} else {
> > 		cache->len = req->req.length;
> 
> This looks OK to me. But there seems to be another bug. I think the line
> 
> 	kfree(cache);
> 
> after the if and else blocks has to be replaced by
> 
> 	CSR1212_FREE(cache);

Yes, please. We are trying to keep the csr1212.[ch] files compatible for
use in userspace and kernel.

-- 
Ubuntu     - http://www.ubuntu.com/
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
SwissDisk  - http://www.swissdisk.com/

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

[Jody McIntyre]

On Mon, Nov 21, 2005 at 10:56:09AM -0800, Ben Collins wrote:
> > 
> > This looks OK to me. But there seems to be another bug. I think the line
> > 
> > 	kfree(cache);
> > 
> > after the if and else blocks has to be replaced by
> > 
> > 	CSR1212_FREE(cache);
> 
> Yes, please. We are trying to keep the csr1212.[ch] files compatible for
> use in userspace and kernel.

raw1394.c does not have to be kept compatible.  Stefan's suggestion
doesn't hurt though.  Anyone have a patch?

Cheers,
Jody


> 
> -- 
> Ubuntu     - http://www.ubuntu.com/
> Debian     - http://www.debian.org/
> Linux 1394 - http://www.linux1394.org/
> SwissDisk  - http://www.swissdisk.com/
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
> Register for a JBoss Training Course.  Free Certification Exam
> for All Training Attendees Through End of 2005. For more info visit:
> http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click
> _______________________________________________
> mailing list linux1394-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel

-- 

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

[Stefan Richter]

[PATCH 2.6.15-rc2] raw1394: fix memory deallocation in modify_config_rom

raw1394: use correct deallocation macro for CSR cache
 
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

--- linux-2.6.15-rc2/drivers/ieee1394/raw1394.c.orig	2005-11-21 22:17:13.000000000 +0100
+++ linux-2.6.15-rc2/drivers/ieee1394/raw1394.c	2005-11-21 22:29:19.000000000 +0100
@@ -2172,7 +2171,7 @@ static int modify_config_rom(struct file
 		}
 	}
 	kfree(cache->filled_head);
-	kfree(cache);
+	CSR1212_FREE(cache);
 
 	if (ret >= 0) {
 		/* we have to free the request, because we queue no response,

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

[Jody McIntyre]

On Mon, Nov 21, 2005 at 11:09:02PM +0100, Stefan Richter wrote:
> [PATCH 2.6.15-rc2] raw1394: fix memory deallocation in modify_config_rom
> 
> raw1394: use correct deallocation macro for CSR cache

Applied.

Cheers,
Jody

>  
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> 
> --- linux-2.6.15-rc2/drivers/ieee1394/raw1394.c.orig	2005-11-21 22:17:13.000000000 +0100
> +++ linux-2.6.15-rc2/drivers/ieee1394/raw1394.c	2005-11-21 22:29:19.000000000 +0100
> @@ -2172,7 +2171,7 @@ static int modify_config_rom(struct file
>  		}
>  	}
>  	kfree(cache->filled_head);
> -	kfree(cache);
> +	CSR1212_FREE(cache);
>  
>  	if (ret >= 0) {
>  		/* we have to free the request, because we queue no response,
> 

-- 

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

[Jody McIntyre]

On Mon, Nov 21, 2005 at 12:52:42AM +0100, Adrian Bunk wrote:

> 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.

Applied.

Cheers,
Jody

> 
> 
> 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;
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
> Register for a JBoss Training Course.  Free Certification Exam
> for All Training Attendees Through End of 2005. For more info visit:
> http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click
> _______________________________________________
> mailing list linux1394-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel

-- 

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

[Jesper Juhl]

On 11/21/05, Adrian Bunk <bunk@stusta.de> 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);
>
Hmmm,  kfree() deals with NULL pointers just fine, so there's no
problem if cache->filled_head is NULL. There is, however, a NULL
pointer deref problem if `cache' is NULL, but that's not what your
patch checks for.

Shouldn't your patch be doing something like this (that is if cache
can ever be NULL at this point)?  :

--- linux-2.6.15-rc2-orig/drivers/ieee1394/raw1394.c    2005-11-20
22:25:27.000000000 +0100
+++ linux-2.6.15-rc2/drivers/ieee1394/raw1394.c 2005-11-21
00:33:34.000000000 +0100
@@ -2171,8 +2171,10 @@ static int modify_config_rom(struct file
                        }
                }
        }
-       kfree(cache->filled_head);
-       kfree(cache);
+       if (cache) {
+               kfree(cache->filled_head);
+               kfree(cache);
+       }

        if (ret >= 0) {
                /* we have to free the request, because we queue no response,


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

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

[Adrian Bunk]

On Mon, Nov 21, 2005 at 12:45:14AM +0100, Jesper Juhl wrote:
> On 11/21/05, Adrian Bunk <bunk@stusta.de> 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);
> >
> Hmmm,  kfree() deals with NULL pointers just fine, so there's no
> problem if cache->filled_head is NULL. There is, however, a NULL
> pointer deref problem if `cache' is NULL, but that's not what your
> patch checks for.
>...

OK, I was blind...

I've just sent a better patch.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed