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

[Muchun Song]

Prateek Sood <prsood@xxxxxxxxxxxxxx> ä2019å5æ14æåä äå7:00åéï
>
> On 5/14/19 4:26 PM, Mukesh Ojha wrote:
> > ++
> >
> > On 5/4/2019 8:17 PM, Muchun Song wrote:
> >> Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> ä2019å5æ2æåå äå2:25åéï
> >>
> >>>>> The basic idea yes, the whole bool *locked is horrid though.
> >>>>> Wouldn't it
> >>>>> work to have a get_device_parent_locked that always returns with
> >>>>> the mutex held,
> >>>>> or just move the mutex to the caller or something simpler like this
> >>>>> ?
> >>>>>
> >>>> Greg and Rafael, do you have any suggestions for this? Or you also
> >>>> agree with Ben?
> >>> Ping guys ? This is worth fixing...
> >> I also agree with you. But Greg and Rafael seem to be high latency right now.
> >>
> >>  From your suggestions, I think introduce get_device_parent_locked() may easy
> >> to fix. So, do you agree with the fix of the following code snippet
> >> (You can also
> >> view attachments)?
> >>
> >> I introduce a new function named get_device_parent_locked_if_glue_dir() which
> >> always returns with the mutex held only when we live in glue dir. We should call
> >> unlock_if_glue_dir() to release the mutex. The
> >> get_device_parent_locked_if_glue_dir()
> >> and unlock_if_glue_dir() should be called in pairs.
> >>
> >> ---
> >> drivers/base/core.c | 44 ++++++++++++++++++++++++++++++++++++--------
> >> 1 file changed, 36 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index 4aeaa0c92bda..5112755c43fa 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -1739,8 +1739,9 @@ 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)
> >> +static struct kobject *__get_device_parent(struct device *dev,
> >> +                    struct device *parent,
> >> +                    bool lock)
> >> {
> >>     if (dev->class) {
> >>         struct kobject *kobj = NULL;
> >> @@ -1779,14 +1780,16 @@ static struct kobject
> >> *get_device_parent(struct device *dev,
> >>             }
> >>         spin_unlock(&dev->class->p->glue_dirs.list_lock);
> >>         if (kobj) {
> >> -           mutex_unlock(&gdp_mutex);
> >> +           if (!lock)
> >> +               mutex_unlock(&gdp_mutex);
> >>             return kobj;
> >>         }
> >>         /* or create a new class-directory at the parent device */
> >>         k = class_dir_create_and_add(dev->class, parent_kobj);
> >>         /* do not emit an uevent for this simple "glue" directory */
> >> -       mutex_unlock(&gdp_mutex);
> >> +       if (!lock)
> >> +           mutex_unlock(&gdp_mutex);
> >>         return k;
> >>     }
> >> @@ -1799,6 +1802,19 @@ static struct kobject *get_device_parent(struct
> >> device *dev,
> >>     return NULL;
> >> }
> >> +static inline struct kobject *get_device_parent(struct device *dev,
> >> +                       struct device *parent)
> >> +{
> >> +   return __get_device_parent(dev, parent, false);
> >> +}
> >> +
> >> +static inline struct kobject *
> >> +get_device_parent_locked_if_glue_dir(struct device *dev,
> >> +                struct device *parent)
> >> +{
> >> +   return __get_device_parent(dev, parent, true);
> >> +}
> >> +
> >> static inline bool live_in_glue_dir(struct kobject *kobj,
> >>                  struct device *dev)
> >> {
> >> @@ -1831,6 +1847,16 @@ static void cleanup_glue_dir(struct device
> >> *dev, struct kobject *glue_dir)
> >>     mutex_unlock(&gdp_mutex);
> >> }
> >> +static inline void unlock_if_glue_dir(struct device *dev,
> >> +                struct kobject *glue_dir)
> >> +{
> >> +   /* see if we live in a "glue" directory */
> >> +   if (!live_in_glue_dir(glue_dir, dev))
> >> +       return;
> >> +
> >> +   mutex_unlock(&gdp_mutex);
> >> +}
> >> +
> >> static int device_add_class_symlinks(struct device *dev)
> >> {
> >>     struct device_node *of_node = dev_of_node(dev);
> >> @@ -2040,7 +2066,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_locked_if_glue_dir(dev, parent);
> >>     if (IS_ERR(kobj)) {
> >>         error = PTR_ERR(kobj);
> >>         goto parent_error;
> >> @@ -2055,10 +2081,12 @@ int device_add(struct device *dev)
> >>     /* first, register with generic layer. */
> >>     /* we require the name to be set before, and pass NULL */
> >>     error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> >> -   if (error) {
> >> -       glue_dir = get_glue_dir(dev);
> >> +
> >> +   glue_dir = get_glue_dir(dev);
> >> +   unlock_if_glue_dir(dev, glue_dir);
> >> +
> >> +   if (error)
> >>         goto Error;
> >> -   }
> >>     /* notify platform of device entry */
> >>     error = device_platform_notify(dev, KOBJ_ADD);
> >> --
>
> This change has been done in device_add(). AFAICT, locked
> version of get_device_parent should be used in device_move()
> also.
>

Yeah, I agree with you. I will send the v2 patch later to fix it also.
Thanks.

Yours,
Muchun