lkml.org 
[lkml]   [2019]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] driver core: Fix use-after-free and double free on glue directory
Hi Cheers,

Benjamin Herrenschmidt <benh@kernel.crashing.org> 于2019年4月25日周四 下午5:24写道:
>
> On Tue, 2019-04-23 at 22:32 +0800, Muchun Song wrote:
> > There is a race condition between removing glue directory and adding a new
> > device under the glue directory. It can be reproduced in following test:
> >
>
> .../...
>
> > In order to avoid this happening, we we should not call kobject_del() on
> > path2 when the reference count of glue_dir is greater than 1. So we add a
> > conditional statement to fix it.
>
> Good catch ! However I'm not completely happy about the fix you
> propose.
>
> I find relying on the object count for such decisions rather fragile as
> it could be taken temporarily for other reasons, couldn't it ? In which
> case we would just fail...

It could be taken temporarily for other reasons, what reasons?
I also can not figure out which case could result in this.

>
> Ideally, the looking up of the glue dir and creation of its child
> should be protected by the same lock instance (the gdp_mutex in that
> case).
>
> That might require a bit of shuffling around though.
>
> Greg, thoughts ? This whole gluedir business is annoyingly racy still.
>
> My gut feeling is that the "right fix" is to ensure the lookup of the
> glue dir and creation of the child object(s) are done under a single
> instance of gdp_mutex so we never see a stale "empty" but still
> poentially used glue dir around.

I agree with you that the looking up of the glue dir and creation of its child
should be protected by the same lock of gdp_mutex. So, do you agree with
the fix of the following code snippet?

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1740,8 +1740,11 @@ class_dir_create_and_add(struct class *class,
struct kobject *parent_kobj)
static DEFINE_MUTEX(gdp_mutex);

static struct kobject *get_device_parent(struct device *dev,
- struct device *parent)
+ struct device *parent,
+ bool *locked)
{
+ *locked = false;
+
if (dev->class) {
struct kobject *kobj = NULL;
struct kobject *parent_kobj;
@@ -1779,7 +1782,7 @@ static struct kobject *get_device_parent(struct
device *dev,
}
spin_unlock(&dev->class->p->glue_dirs.list_lock);
if (kobj) {
- mutex_unlock(&gdp_mutex);
+ *locked = true;
return kobj;
}

@@ -2007,6 +2010,7 @@ int device_add(struct device *dev)
struct class_interface *class_intf;
int error = -EINVAL;
struct kobject *glue_dir = NULL;
+ bool locked;

dev = get_device(dev);
if (!dev)
@@ -2040,7 +2044,7 @@ int device_add(struct device *dev)
pr_debug("device: '%s': %s\n", dev_name(dev), __func__);

parent = get_device(dev->parent);
- kobj = get_device_parent(dev, parent);
+ kobj = get_device_parent(dev, parent, &locked);
if (IS_ERR(kobj)) {
error = PTR_ERR(kobj);
goto parent_error;
@@ -2057,9 +2061,14 @@ int device_add(struct device *dev)
error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
if (error) {
glue_dir = get_glue_dir(dev);
+ if (locked)
+ mutex_unlock(&gdp_mutex);
goto Error;
}

+ if (locked)
+ mutex_unlock(&gdp_mutex);
+
/* notify platform of device entry */
error = device_platform_notify(dev, KOBJ_ADD);
if (error)
Yours
Muchun

\
 
 \ /
  Last update: 2019-04-25 17:44    [W:0.100 / U:0.896 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site