* [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
@ 2005-11-20 23:20 Adrian Bunk
2005-11-20 23:33 ` Jody McIntyre
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Adrian Bunk @ 2005-11-20 23:20 UTC (permalink / raw)
To: bcollins, dan; +Cc: linux1394-devel, scjody, linux-kernel, stable
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) {
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:20 [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference Adrian Bunk
@ 2005-11-20 23:33 ` Jody McIntyre
2005-11-20 23:46 ` Adrian Bunk
2005-11-20 23:40 ` Dave Jones
2005-11-20 23:45 ` Jesper Juhl
2 siblings, 1 reply; 14+ messages in thread
From: Jody McIntyre @ 2005-11-20 23:33 UTC (permalink / raw)
To: Adrian Bunk; +Cc: bcollins, dan, linux1394-devel, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:20 [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference Adrian Bunk
2005-11-20 23:33 ` Jody McIntyre
@ 2005-11-20 23:40 ` Dave Jones
2005-11-20 23:52 ` Adrian Bunk
2005-11-20 23:45 ` Jesper Juhl
2 siblings, 1 reply; 14+ messages in thread
From: Dave Jones @ 2005-11-20 23:40 UTC (permalink / raw)
To: Adrian Bunk; +Cc: bcollins, dan, linux1394-devel, scjody, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:20 [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference Adrian Bunk
2005-11-20 23:33 ` Jody McIntyre
2005-11-20 23:40 ` Dave Jones
@ 2005-11-20 23:45 ` Jesper Juhl
2005-11-20 23:54 ` Adrian Bunk
2 siblings, 1 reply; 14+ messages in thread
From: Jesper Juhl @ 2005-11-20 23:45 UTC (permalink / raw)
To: Adrian Bunk; +Cc: bcollins, dan, linux1394-devel, scjody, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:33 ` Jody McIntyre
@ 2005-11-20 23:46 ` Adrian Bunk
2005-11-20 23:54 ` Adrian Bunk
0 siblings, 1 reply; 14+ messages in thread
From: Adrian Bunk @ 2005-11-20 23:46 UTC (permalink / raw)
To: Jody McIntyre; +Cc: bcollins, dan, linux1394-devel, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:40 ` Dave Jones
@ 2005-11-20 23:52 ` Adrian Bunk
2005-11-21 19:13 ` Stefan Richter
2005-11-21 21:55 ` Jody McIntyre
0 siblings, 2 replies; 14+ messages in thread
From: Adrian Bunk @ 2005-11-20 23:52 UTC (permalink / raw)
To: Dave Jones, bcollins, dan, linux1394-devel, scjody, linux-kernel,
stable
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:45 ` Jesper Juhl
@ 2005-11-20 23:54 ` Adrian Bunk
0 siblings, 0 replies; 14+ messages in thread
From: Adrian Bunk @ 2005-11-20 23:54 UTC (permalink / raw)
To: Jesper Juhl; +Cc: bcollins, dan, linux1394-devel, scjody, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:46 ` Adrian Bunk
@ 2005-11-20 23:54 ` Adrian Bunk
0 siblings, 0 replies; 14+ messages in thread
From: Adrian Bunk @ 2005-11-20 23:54 UTC (permalink / raw)
To: Jody McIntyre; +Cc: bcollins, dan, linux1394-devel, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-21 19:13 ` Stefan Richter
@ 2005-11-21 18:56 ` Ben Collins
2005-11-21 21:52 ` Jody McIntyre
0 siblings, 1 reply; 14+ messages in thread
From: Ben Collins @ 2005-11-21 18:56 UTC (permalink / raw)
To: Stefan Richter
Cc: Adrian Bunk, Dave Jones, dan, linux1394-devel, scjody,
linux-kernel, stable
> >- 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/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:52 ` Adrian Bunk
@ 2005-11-21 19:13 ` Stefan Richter
2005-11-21 18:56 ` Ben Collins
2005-11-21 21:55 ` Jody McIntyre
1 sibling, 1 reply; 14+ messages in thread
From: Stefan Richter @ 2005-11-21 19:13 UTC (permalink / raw)
To: Adrian Bunk
Cc: Dave Jones, bcollins, dan, linux1394-devel, scjody, linux-kernel,
stable
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/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-21 18:56 ` Ben Collins
@ 2005-11-21 21:52 ` Jody McIntyre
2005-11-21 22:09 ` Stefan Richter
0 siblings, 1 reply; 14+ messages in thread
From: Jody McIntyre @ 2005-11-21 21:52 UTC (permalink / raw)
To: Ben Collins
Cc: Stefan Richter, Adrian Bunk, Dave Jones, dan, linux1394-devel,
linux-kernel, stable
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
--
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-20 23:52 ` Adrian Bunk
2005-11-21 19:13 ` Stefan Richter
@ 2005-11-21 21:55 ` Jody McIntyre
1 sibling, 0 replies; 14+ messages in thread
From: Jody McIntyre @ 2005-11-21 21:55 UTC (permalink / raw)
To: Adrian Bunk
Cc: Dave Jones, bcollins, dan, linux1394-devel, linux-kernel, stable
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
--
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-21 21:52 ` Jody McIntyre
@ 2005-11-21 22:09 ` Stefan Richter
2005-11-21 22:32 ` Jody McIntyre
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Richter @ 2005-11-21 22:09 UTC (permalink / raw)
To: scjody; +Cc: bcollins, bunk, davej, dan, linux1394-devel, linux-kernel, stable
[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,
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference
2005-11-21 22:09 ` Stefan Richter
@ 2005-11-21 22:32 ` Jody McIntyre
0 siblings, 0 replies; 14+ messages in thread
From: Jody McIntyre @ 2005-11-21 22:32 UTC (permalink / raw)
To: Stefan Richter
Cc: bcollins, bunk, davej, dan, linux1394-devel, linux-kernel, stable
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,
>
--
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2005-11-21 22:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-11-20 23:20 [2.6 patch] drivers/ieee1394/raw1394.c: fix a NULL pointer dereference Adrian Bunk
2005-11-20 23:33 ` Jody McIntyre
2005-11-20 23:46 ` Adrian Bunk
2005-11-20 23:54 ` Adrian Bunk
2005-11-20 23:40 ` Dave Jones
2005-11-20 23:52 ` Adrian Bunk
2005-11-21 19:13 ` Stefan Richter
2005-11-21 18:56 ` Ben Collins
2005-11-21 21:52 ` Jody McIntyre
2005-11-21 22:09 ` Stefan Richter
2005-11-21 22:32 ` Jody McIntyre
2005-11-21 21:55 ` Jody McIntyre
2005-11-20 23:45 ` Jesper Juhl
2005-11-20 23:54 ` Adrian Bunk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox