drivers/ieee1394/raw1394.c: fix a NULL pointer dereference

PATCH Applied Adrian Bunk 6 participants 14 messages Nov 20, 2005 – Nov 21, 2005

lore

Share & tools

lore

Adrian Bunk

author

2005-11-20 23:20 UTC #0 lore raw

PATCH 1/1 drivers/ieee1394/raw1394.c: fix a NULL pointer dereference

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

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -1

@@ -2166,7 +2166,8 @@
 			}
 		}
 	}
-	kfree(cache->filled_head);
+	if(cache->filled_head)
+		kfree(cache->filled_head);
 	kfree(cache);
 
 	if (ret >= 0) {

Jody McIntyre

2005-11-20 23:39 UTC #1 lore raw

drivers/ieee1394/raw1394.c line 2170

Quoted 2 lines · Adrian Bunk

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -0
+	if(cache->filled_head)
+		kfree(cache->filled_head);

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

Jody

Adrian Bunk

author

2005-11-20 23:46 UTC #2 lore raw

Quoted 5 lines · Jody McIntyre

drivers/ieee1394/raw1394.c line 2170
Quoted 2 lines · Adrian Bunk
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -0
+	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...

Quoted 1 line

Jody

cu
Adrian

Signature · 4 lines

       "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

author

2005-11-20 23:54 UTC #3 lore raw

Quoted 8 lines · Adrian Bunk

Quoted 5 lines · Jody McIntyre
drivers/ieee1394/raw1394.c line 2170
Quoted 2 lines · Adrian Bunk
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -0
+	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

Signature · 4 lines

       "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

Dave Jones

2005-11-20 23:42 UTC #4 lore raw

In reply to Adrian Bunk

Quoted 10 lines · 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 <[email protected]>

drivers/ieee1394/raw1394.c line 2173

Quoted 9 lines

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -1

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

Adrian Bunk

author

2005-11-20 23:52 UTC #5 lore raw

Quoted 18 lines · Dave Jones

Quoted 17 lines · 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 <[email protected]>
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +2 -1
@@ -2166,7 +2166,8 @@
 			}
 		}
 	}
-	kfree(cache->filled_head);
+	if(cache->filled_head)
+		kfree(cache->filled_head);
 	kfree(cache);
 

Quoted 5 lines

drivers/ieee1394/raw1394.c line 2173
Quoted 2 lines
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +0 -0
 	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?

Quoted 1 line

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

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +0 -1

@@ -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;

Stefan Richter

2005-11-21 19:15 UTC #6 lore raw

Adrian Bunk wrote:
[...]

Quoted 8 lines

There's no need to free cache at this point, and it's getting free'd later.
Signed-off-by Adrian Bunk <[email protected]>

drivers/ieee1394/raw1394.c line 2136

Quoted 7 lines

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +0 -1

@@ -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);

Signature · 3 lines

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

Ben Collins

2005-11-21 20:04 UTC #7 lore raw

Quoted 7 lines

Quoted 4 lines
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +0 -1
-		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

Quoted 5 lines

	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.

Signature · 4 lines

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

Jody McIntyre

2005-11-21 21:59 UTC #8 lore raw

Quoted 11 lines · Ben Collins

Quoted 8 lines
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

Quoted 16 lines

Signature · 15 lines

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

Stefan Richter

2005-11-21 22:10 UTC #9 lore raw

[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]>

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.orig +1 -1

@@ -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,

Jody McIntyre

Applied

2005-11-21 22:37 UTC #10 lore raw

Quoted 3 lines · Stefan Richter

[PATCH 2.6.15-rc2] raw1394: fix memory deallocation in modify_config_rom
raw1394: use correct deallocation macro for CSR cache

Applied.

Cheers,
Jody

Quoted 15 lines

Signed-off-by Stefan Richter <[email protected]>

drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.orig +1 -1

@@ -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,

Jody McIntyre

Applied

2005-11-21 22:01 UTC #11 lore raw

In reply to Adrian Bunk

Quoted 6 lines · Adrian Bunk

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

Quoted 25 lines

Signed-off-by Adrian Bunk <[email protected]>
drivers/ieee1394/raw1394.c renamed from drivers/ieee1394/raw1394.c.old +0 -2
@@ -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

Jesper Juhl

2005-11-20 23:45 UTC #12 lore raw

In reply to Adrian Bunk

Quoted 9 lines · 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 <[email protected]>

drivers/ieee1394/raw1394.c 2005-11-20 22:09:34.000000000 +0100 line 2172

Quoted 8 lines

drivers/ieee1394/raw1394.c 2005-11-20 22:09:34.000000000 +0100 renamed from drivers/ieee1394/raw1394.c.old 2005-11-20 22:08:57.000000000 +0100 +2 -1

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

Adrian Bunk

author

2005-11-20 23:54 UTC #13 lore raw

Quoted 7 lines · Jesper Juhl

Quoted 6 lines · 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 <[email protected]>

Quoted 16 lines

drivers/ieee1394/raw1394.c 2005-11-20 22:09:34.000000000 +0100 line 2172
Quoted 11 lines
drivers/ieee1394/raw1394.c 2005-11-20 22:09:34.000000000 +0100 renamed from drivers/ieee1394/raw1394.c.old 2005-11-20 22:08:57.000000000 +0100 +2 -1
@@ -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

Signature · 4 lines

       "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