lkml.org 
[lkml]   [2019]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] driver core: Fix use-after-free and double free on glue directory
Prateek Sood <prsood@codeaurora.org> 于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@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.
>

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

Yours,
Muchun

\
 
 \ /
  Last update: 2019-05-14 13:52    [W:0.050 / U:1.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site