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

[Prateek Sood]

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.

Thanks

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