The coverity checker spotted that this was a NULL pointer dereference in
the "if (copy_from_user(...))" case.
Signed-off-by: Adrian Bunk <[email protected]>
--- 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) {
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
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 <[email protected]>
>
> --- 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
On 11/21/05, Adrian Bunk <[email protected]> wrote:
> The coverity checker spotted that this was a NULL pointer dereference in
> the "if (copy_from_user(...))" case.
>
>
> Signed-off-by: Adrian Bunk <[email protected]>
>
> --- 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 <[email protected]>
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
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
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 <[email protected]>
> >
> > --- 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 <[email protected]>
--- 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;
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
On Mon, Nov 21, 2005 at 12:45:14AM +0100, Jesper Juhl wrote:
> On 11/21/05, Adrian Bunk <[email protected]> wrote:
> > The coverity checker spotted that this was a NULL pointer dereference in
> > the "if (copy_from_user(...))" case.
> >
> >
> > Signed-off-by: Adrian Bunk <[email protected]>
> >
> > --- 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
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 <[email protected]>
>
> --- 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/
> >- 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/
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 [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel
--
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 <[email protected]>
>
> --- 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 [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel
--
[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 <[email protected]>
--- 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,
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 <[email protected]>
>
> --- 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,
>
--