From: Phil Carmody <[email protected]>
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 <[email protected]>
---
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
On Thu, Oct 01, 2009 at 05:50:48PM +0300, Phil Carmody wrote:
> From: Phil Carmody <[email protected]>
>
> 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
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 <[email protected]>
> > >
> > > 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
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 <[email protected]>
> >
> > 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
On Thu, 1 Oct 2009 17:50:48 +0300,
Phil Carmody <[email protected]> wrote:
> From: Phil Carmody <[email protected]>
>
> 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)
On Mon, 2009-10-05 at 18:37 +0200, ext Cornelia Huck wrote:
> On Thu, 1 Oct 2009 17:50:48 +0300,
> Phil Carmody <[email protected]> wrote:
>
> > From: Phil Carmody <[email protected]>
> >
> > 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
On Tue, 06 Oct 2009 11:52:09 +0300,
Phil Carmody <[email protected]> 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 <[email protected]> wrote:
> >
> > > From: Phil Carmody <[email protected]>
> > >
> > > 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.
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 <[email protected]>
---
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)