Messages in this thread |  | | Subject | Re: [PATCH] driver core: Fix use-after-free and double free on glue directory | From | Prateek Sood <> | Date | Tue, 14 May 2019 16:29:58 +0530 |
| |
On 5/14/19 4:26 PM, Mukesh Ojha wrote: > ++ > > On 5/4/2019 8:17 PM, Muchun Song wrote: >> Benjamin Herrenschmidt <benh@kernel.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
|  |