Re: [PATCH] driver core: Fix use-after-free and double free on glue directory

[Muchun Song]

Hi Cheers,

Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> ä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