lkml.org 
[lkml]   [2019]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] driver core: Fix use-after-free and double free on glue directory
From
Date
++

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);
> --

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