Message-ID: <c0166ef7-ef76-56d8-6289-276573e3aea7@codeaurora.org>
Date:   Tue, 14 May 2019 16:29:58 +0530
From:   Prateek Sood <prsood@...eaurora.org>
To:     Mukesh Ojha <mojha@...eaurora.org>,
        Muchun Song <smuchun@...il.com>, gregkh@...uxfoundation.org,
        rafael@...nel.org
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] driver core: Fix use-after-free and double free on glue
 directory

On 5/14/19 4:26 PM, Mukesh Ojha wrote:
> ++
> 
> On 5/4/2019 8:17 PM, Muchun Song wrote:
>> Benjamin Herrenschmidt <benh@...nel.crashing.org> 于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.

Thanks

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

Powered by blists - more mailing lists