[Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Eric Sesterhenn]

hi,

this fixes coverity bug #845, if group is NULL,
we dereference it when setting up dentry.

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.16/fs/configfs/dir.c.orig	2006-03-23 00:02:23.000000000 +0100
+++ linux-2.6.16/fs/configfs/dir.c	2006-03-23 00:03:49.000000000 +0100
@@ -500,7 +500,7 @@ static int create_default_group(struct c
 static int populate_groups(struct config_group *group)
 {
 	struct config_group *new_group;
-	struct dentry *dentry = group->cg_item.ci_dentry;
+	struct dentry *dentry;
 	int ret = 0;
 	int i;
 
@@ -512,6 +512,8 @@ static int populate_groups(struct config
 		 * parent to find us, let alone mess with our tree.
 		 * That said, taking our i_mutex is closer to mkdir
 		 * emulation, and shouldn't hurt. */
+		dentry = group->cg_item.ci_dentry;
+
 		mutex_lock(&dentry->d_inode->i_mutex);
 
 		for (i = 0; group->default_groups[i]; i++) {

Re: [Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Joel Becker]

On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> this fixes coverity bug #845, if group is NULL,
> we dereference it when setting up dentry.

	Is the converity checker merly looking at in-function patterns?
Where can I access the bug report (sorry for the question).
	group cannot be null here, we aren't called any other way.  So
while you are correct that the code below is needed in the presence of a
NULL group, really the "if (group" isn't necessary, just the "if
(group->default_groups)".  I could even BUG_ON() if you'd like.

Joel

> 
> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> 
> --- linux-2.6.16/fs/configfs/dir.c.orig	2006-03-23 00:02:23.000000000 +0100
> +++ linux-2.6.16/fs/configfs/dir.c	2006-03-23 00:03:49.000000000 +0100
> @@ -500,7 +500,7 @@ static int create_default_group(struct c
>  static int populate_groups(struct config_group *group)
>  {
>  	struct config_group *new_group;
> -	struct dentry *dentry = group->cg_item.ci_dentry;
> +	struct dentry *dentry;
>  	int ret = 0;
>  	int i;
>  
> @@ -512,6 +512,8 @@ static int populate_groups(struct config
>  		 * parent to find us, let alone mess with our tree.
>  		 * That said, taking our i_mutex is closer to mkdir
>  		 * emulation, and shouldn't hurt. */
> +		dentry = group->cg_item.ci_dentry;
> +
>  		mutex_lock(&dentry->d_inode->i_mutex);
>  
>  		for (i = 0; group->default_groups[i]; i++) {
> 
> 

-- 

"Win95 file and print sharing are for relatively friendly nets."
	- Paul Leach, Microsoft

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Eric Sesterhenn]

hi,

On Wed, 2006-03-22 at 15:27 -0800, Joel Becker wrote:
> On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> > this fixes coverity bug #845, if group is NULL,
> > we dereference it when setting up dentry.
> 
> 	Is the converity checker merly looking at in-function patterns?

afaik it also looks what the functions which get called do. If you call
a function that might free a pointer you pass, it warns if you use
it afterwards.

> Where can I access the bug report (sorry for the question).

I would guess scan-admin@coverity.com 

> 	group cannot be null here, we aren't called any other way.  So
> while you are correct that the code below is needed in the presence of a
> NULL group, really the "if (group" isn't necessary, just the "if
> (group->default_groups)".  I could even BUG_ON() if you'd like.

I would then propose the following patch, so the check can be
removed for people who like small kernels. I dont think gcc notices
that all callers use non-NULL values and optimizes it away.

--- linux-2.6.16/fs/configfs/dir.c.orig	2006-03-23 00:31:16.000000000 +0100
+++ linux-2.6.16/fs/configfs/dir.c	2006-03-23 00:32:07.000000000 +0100
@@ -504,7 +504,9 @@ static int populate_groups(struct config
 	int ret = 0;
 	int i;
 
-	if (group && group->default_groups) {
+	BUG_ON(!group);		/* group == NULL is not allowed */
+	
+	if (group->default_groups) {
 		/* FYI, we're faking mkdir here
 		 * I'm not sure we need this semaphore, as we're called
 		 * from our parent's mkdir.  That holds our parent's

Re: [Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Joel Becker]

On Thu, Mar 23, 2006 at 12:36:54AM +0100, Eric Sesterhenn wrote:
> I would then propose the following patch, so the check can be
> removed for people who like small kernels. I dont think gcc notices
> that all callers use non-NULL values and optimizes it away.
> 
> -	if (group && group->default_groups) {
> +	BUG_ON(!group);		/* group == NULL is not allowed */
> +	
> +	if (group->default_groups) {

	This is pretty much what I meant.  Thanks.

Joel
-- 

Life's Little Instruction Book #3

	"Watch a sunrise at least once a year."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Adrian Bunk]

On Wed, Mar 22, 2006 at 03:27:09PM -0800, Joel Becker wrote:
> On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> > this fixes coverity bug #845, if group is NULL,
> > we dereference it when setting up dentry.
> 
> 	Is the converity checker merly looking at in-function patterns?
> Where can I access the bug report (sorry for the question).
> 	group cannot be null here, we aren't called any other way.  So
> while you are correct that the code below is needed in the presence of a
> NULL group, really the "if (group" isn't necessary, just the "if
> (group->default_groups)".  I could even BUG_ON() if you'd like.

Coverity is correct that something is wrong:

<--  snip  -->

...
        struct dentry *dentry = group->cg_item.ci_dentry;
        int ret = 0;
        int i;

        if (group && group->default_groups) {
...

<--  snip  -->

The question is only whether the dereferencing is a real bug or there's 
only the harmless issue of a superfluous check.

> Joel

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: [Patch] Possible NULL pointer dereference in fs/configfs/dir.c

[Adrian Bunk]

On Thu, Mar 23, 2006 at 12:36:54AM +0100, Eric Sesterhenn wrote:
>...
> On Wed, 2006-03-22 at 15:27 -0800, Joel Becker wrote:
>...
> > 	group cannot be null here, we aren't called any other way.  So
> > while you are correct that the code below is needed in the presence of a
> > NULL group, really the "if (group" isn't necessary, just the "if
> > (group->default_groups)".  I could even BUG_ON() if you'd like.
> 
> I would then propose the following patch, so the check can be
> removed for people who like small kernels. I dont think gcc notices
> that all callers use non-NULL values and optimizes it away.
> 
> --- linux-2.6.16/fs/configfs/dir.c.orig	2006-03-23 00:31:16.000000000 +0100
> +++ linux-2.6.16/fs/configfs/dir.c	2006-03-23 00:32:07.000000000 +0100
> @@ -504,7 +504,9 @@ static int populate_groups(struct config
>  	int ret = 0;
>  	int i;
>  
> -	if (group && group->default_groups) {
> +	BUG_ON(!group);		/* group == NULL is not allowed */
> +	
> +	if (group->default_groups) {
>...

Why do we need a BUG_ON() if we already got an Oops?
Simply remove the check.

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