Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5326B608A5
Subject: Re: [PATCH] driver core: Fix use-after-free and double free on glue
 directory
To:     Muchun Song <smuchun@gmail.com>, gregkh@linuxfoundation.org,
        rafael@kernel.org
Cc:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        prsood@codeaurora.org
References: <20190423143258.96706-1-smuchun@gmail.com>
 <24b0fff3775147c04b006282727d94fea7f408b4.camel@kernel.crashing.org>
 <CAPSr9jHhwASv7=83hU+81mC0JJyuyt2gGxLmyzpCOfmc9vKgGQ@mail.gmail.com>
 <a37e7a49c3e7fa6ece2be2b76798fef3e51ade4e.camel@kernel.crashing.org>
 <CAPSr9jHCVCHNK+AmKkUBgs4dPC0UC5KdYKqMinkauyL3OL6qrQ@mail.gmail.com>
 <79fbc203bc9fa09d88ab2c4bff8635be4c293d49.camel@kernel.crashing.org>
 <CAPSr9jHw9hgAZo2TuDAKdSLEG1c6EtJG005MWxsxfnbsk1AXow@mail.gmail.com>
From:   Mukesh Ojha <mojha@codeaurora.org>
Message-ID: <d9495ef6-17bc-dc50-f5fe-fb5ff20edfde@codeaurora.org>
Date:   Tue, 14 May 2019 16:26:24 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <CAPSr9jHw9hgAZo2TuDAKdSLEG1c6EtJG005MWxsxfnbsk1AXow@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

++

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