Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Greg KH]

On Thu, Oct 01, 2009 at 05:50:48PM +0300, Phil Carmody wrote:
> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> In the absense of kobj->kset, new_parent would remain NULL.
> NULL-dereference shenanighans then ensues in the subsequent
> sysfs_move_dir(..., new_parent) call. Bail early instead.

What caller causes this to happen?

thanks,

greg k-h

[PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Phil Carmody]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

In the absense of kobj->kset, new_parent would remain NULL.
NULL-dereference shenanighans then ensues in the subsequent
sysfs_move_dir(..., new_parent) call. Bail early instead.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 lib/kobject.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..3574f94 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -471,8 +471,12 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
 		return -EINVAL;
 	new_parent = kobject_get(new_parent);
 	if (!new_parent) {
-		if (kobj->kset)
+		if (kobj->kset) {
 			new_parent = kobject_get(&kobj->kset->kobj);
+		} else {
+			error = -EINVAL;
+			goto out;
+		}
 	}
 	/* old object path */
 	devpath = kobject_get_path(kobj, GFP_KERNEL);
-- 
1.5.4.3

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Greg KH]

On Thu, Oct 01, 2009 at 06:01:50PM +0300, Phil Carmody wrote:
> On Thu, 2009-10-01 at 16:49 +0200, ext Greg KH wrote:
> > On Thu, Oct 01, 2009 at 05:50:48PM +0300, Phil Carmody wrote:
> > > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > > 
> > > In the absense of kobj->kset, new_parent would remain NULL.
> > > NULL-dereference shenanighans then ensues in the subsequent
> > > sysfs_move_dir(..., new_parent) call. Bail early instead.
> > 
> > What caller causes this to happen?
> 
> It was spotted by static code analysis. If there's no such path, 
> then the ``if (kobj->kset)'' condition itself would be unnecessary.

Hm, as there are only 2-3 callers of rename, I guess this isn't that big
of a deal.  I'll queue it up for .33.

thanks,

greg k-h

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Phil Carmody]

On Thu, 2009-10-01 at 16:49 +0200, ext Greg KH wrote:
> On Thu, Oct 01, 2009 at 05:50:48PM +0300, Phil Carmody wrote:
> > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > 
> > In the absense of kobj->kset, new_parent would remain NULL.
> > NULL-dereference shenanighans then ensues in the subsequent
> > sysfs_move_dir(..., new_parent) call. Bail early instead.
> 
> What caller causes this to happen?

It was spotted by static code analysis. If there's no such path, 
then the ``if (kobj->kset)'' condition itself would be unnecessary.

Phil
-- 
"They weren't designed to run on a computer, they were designed
to run on a Powerpoint slide projector." -- Peter Gutmann

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Cornelia Huck]

On Thu,  1 Oct 2009 17:50:48 +0300,
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> In the absense of kobj->kset, new_parent would remain NULL.
> NULL-dereference shenanighans then ensues in the subsequent
> sysfs_move_dir(..., new_parent) call. Bail early instead.

But sysfs_move_dir(..., NULL) should work and fall back to the sysfs
root...

<looks at code>

It seems that has been broken for some time. Should probably be
something like this instead (uncompiled); I can send it with proper
description and s-o-b once I gave it a test.

---
 fs/sysfs/dir.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.orig/fs/sysfs/dir.c
+++ linux-2.6/fs/sysfs/dir.c
@@ -894,7 +894,8 @@ int sysfs_move_dir(struct kobject *kobj,
 
 	mutex_lock(&sysfs_rename_mutex);
 	BUG_ON(!sd->s_parent);
-	new_parent_sd = new_parent_kobj->sd ? new_parent_kobj->sd : &sysfs_root;
+	new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
+		new_parent_kobj->sd : &sysfs_root;
 
 	error = 0;
 	if (sd->s_parent == new_parent_sd)

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Phil Carmody]

On Mon, 2009-10-05 at 18:37 +0200, ext Cornelia Huck wrote:
> On Thu,  1 Oct 2009 17:50:48 +0300,
> Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
> 
> > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > 
> > In the absense of kobj->kset, new_parent would remain NULL.
> > NULL-dereference shenanighans then ensues in the subsequent
> > sysfs_move_dir(..., new_parent) call. Bail early instead.
> 
> But sysfs_move_dir(..., NULL) should work and fall back to the sysfs
> root...

I guess there are two schools of thought - the strict and the
accommodating. I was contemplating resending an even stricter patch, but
there's nothing wrong with adopting sane safe fallbacks instead. 

> <looks at code>
> 
> It seems that has been broken for some time. Should probably be
> something like this instead (uncompiled); I can send it with proper
> description and s-o-b once I gave it a test.
> 
> ---
>  fs/sysfs/dir.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.orig/fs/sysfs/dir.c
> +++ linux-2.6/fs/sysfs/dir.c
> @@ -894,7 +894,8 @@ int sysfs_move_dir(struct kobject *kobj,
>  
>  	mutex_lock(&sysfs_rename_mutex);
>  	BUG_ON(!sd->s_parent);
> -	new_parent_sd = new_parent_kobj->sd ? new_parent_kobj->sd : &sysfs_root;
> +	new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
> +		new_parent_kobj->sd : &sysfs_root;
>  
>  	error = 0;
>  	if (sd->s_parent == new_parent_sd)

Looks like it knocks the NULL dereference squarely on the head. Yup.

Phil
-- 
"They weren't designed to run on a computer, they were designed
to run on a Powerpoint slide projector." -- Peter Gutmann

Re: [PATCH 1/1] Kobject: bail early if no new_parent in kobject_move()

[Cornelia Huck]

On Tue, 06 Oct 2009 11:52:09 +0300,
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> On Mon, 2009-10-05 at 18:37 +0200, ext Cornelia Huck wrote:
> > On Thu,  1 Oct 2009 17:50:48 +0300,
> > Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
> > 
> > > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > > 
> > > In the absense of kobj->kset, new_parent would remain NULL.
> > > NULL-dereference shenanighans then ensues in the subsequent
> > > sysfs_move_dir(..., new_parent) call. Bail early instead.
> > 
> > But sysfs_move_dir(..., NULL) should work and fall back to the sysfs
> > root...
> 
> I guess there are two schools of thought - the strict and the
> accommodating. I was contemplating resending an even stricter patch, but
> there's nothing wrong with adopting sane safe fallbacks instead. 

I made all the *_move() functions legal for a NULL destination (see
c744aeae9d173a953b771a7ad5c872f91fa99dec), so I'd like to have it back
that way :)

I've put the patch on my s390 system and ran through sysfs_move();
proper patch posting follows.

[PATCH] sysfs: Allow sysfs_move_dir(..., NULL) again.

[Cornelia Huck]

As device_move() and kobject_move() both handle a NULL destination,
sysfs_move_dir() should do this as well (again) and fall back to
sysfs_root in that case.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 fs/sysfs/dir.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.orig/fs/sysfs/dir.c
+++ linux-2.6/fs/sysfs/dir.c
@@ -894,7 +894,8 @@ int sysfs_move_dir(struct kobject *kobj,
 
 	mutex_lock(&sysfs_rename_mutex);
 	BUG_ON(!sd->s_parent);
-	new_parent_sd = new_parent_kobj->sd ? new_parent_kobj->sd : &sysfs_root;
+	new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
+		new_parent_kobj->sd : &sysfs_root;
 
 	error = 0;
 	if (sd->s_parent == new_parent_sd)