lkml.org 
[lkml]   [2020]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: [PATCH] dmabuf: use spinlock to access dmabuf->name
    Date
    >-----Original Message-----
    >From: charante=codeaurora.org@mg.codeaurora.org
    ><charante=codeaurora.org@mg.codeaurora.org> On Behalf Of Charan Teja
    >Kalla
    >Sent: Wednesday, June 17, 2020 2:29 AM
    >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; Sumit Semwal
    ><sumit.semwal@linaro.org>; open list:DMA BUFFER SHARING FRAMEWORK
    ><linux-media@vger.kernel.org>; DRI mailing list <dri-
    >devel@lists.freedesktop.org>
    >Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>;
    >vinmenon@codeaurora.org; LKML <linux-kernel@vger.kernel.org>;
    >stable@vger.kernel.org
    >Subject: Re: [PATCH] dmabuf: use spinlock to access dmabuf->name
    >
    >Thanks Michael for the comments..
    >
    >On 6/16/2020 7:29 PM, Ruhl, Michael J wrote:
    >>> -----Original Message-----
    >>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
    >>> Ruhl, Michael J
    >>> Sent: Tuesday, June 16, 2020 9:51 AM
    >>> To: Charan Teja Kalla <charante@codeaurora.org>; Sumit Semwal
    >>> <sumit.semwal@linaro.org>; open list:DMA BUFFER SHARING
    >FRAMEWORK
    >>> <linux-media@vger.kernel.org>; DRI mailing list <dri-
    >>> devel@lists.freedesktop.org>
    >>> Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>;
    >>> vinmenon@codeaurora.org; LKML <linux-kernel@vger.kernel.org>;
    >>> stable@vger.kernel.org
    >>> Subject: RE: [PATCH] dmabuf: use spinlock to access dmabuf->name
    >>>
    >>>> -----Original Message-----
    >>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
    >>>> Charan Teja Kalla
    >>>> Sent: Thursday, June 11, 2020 9:40 AM
    >>>> To: Sumit Semwal <sumit.semwal@linaro.org>; open list:DMA BUFFER
    >>>> SHARING FRAMEWORK <linux-media@vger.kernel.org>; DRI mailing list
    ><dri-
    >>>> devel@lists.freedesktop.org>
    >>>> Cc: Linaro MM SIG <linaro-mm-sig@lists.linaro.org>;
    >>>> vinmenon@codeaurora.org; LKML <linux-kernel@vger.kernel.org>;
    >>>> stable@vger.kernel.org
    >>>> Subject: [PATCH] dmabuf: use spinlock to access dmabuf->name
    >>>>
    >>>> There exists a sleep-while-atomic bug while accessing the dmabuf->name
    >>>> under mutex in the dmabuffs_dname(). This is caused from the SELinux
    >>>> permissions checks on a process where it tries to validate the inherited
    >>>> files from fork() by traversing them through iterate_fd() (which
    >>>> traverse files under spin_lock) and call
    >>>> match_file(security/selinux/hooks.c) where the permission checks
    >happen.
    >>>> This audit information is logged using dump_common_audit_data()
    >where it
    >>>> calls d_path() to get the file path name. If the file check happen on
    >>>> the dmabuf's fd, then it ends up in ->dmabuffs_dname() and use mutex
    >to
    >>>> access dmabuf->name. The flow will be like below:
    >>>> flush_unauthorized_files()
    >>>> iterate_fd()
    >>>> spin_lock() --> Start of the atomic section.
    >>>> match_file()
    >>>> file_has_perm()
    >>>> avc_has_perm()
    >>>> avc_audit()
    >>>> slow_avc_audit()
    >>>> common_lsm_audit()
    >>>> dump_common_audit_data()
    >>>> audit_log_d_path()
    >>>> d_path()
    >>>> dmabuffs_dname()
    >>>> mutex_lock()--> Sleep while atomic.
    >>>>
    >>>> Call trace captured (on 4.19 kernels) is below:
    >>>> ___might_sleep+0x204/0x208
    >>>> __might_sleep+0x50/0x88
    >>>> __mutex_lock_common+0x5c/0x1068
    >>>> __mutex_lock_common+0x5c/0x1068
    >>>> mutex_lock_nested+0x40/0x50
    >>>> dmabuffs_dname+0xa0/0x170
    >>>> d_path+0x84/0x290
    >>>> audit_log_d_path+0x74/0x130
    >>>> common_lsm_audit+0x334/0x6e8
    >>>> slow_avc_audit+0xb8/0xf8
    >>>> avc_has_perm+0x154/0x218
    >>>> file_has_perm+0x70/0x180
    >>>> match_file+0x60/0x78
    >>>> iterate_fd+0x128/0x168
    >>>> selinux_bprm_committing_creds+0x178/0x248
    >>>> security_bprm_committing_creds+0x30/0x48
    >>>> install_exec_creds+0x1c/0x68
    >>>> load_elf_binary+0x3a4/0x14e0
    >>>> search_binary_handler+0xb0/0x1e0
    >>>>
    >>>> So, use spinlock to access dmabuf->name to avoid sleep-while-atomic.
    >>>>
    >>>> Cc: <stable@vger.kernel.org> [5.3+]
    >>>> Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
    >>>> ---
    >>>> drivers/dma-buf/dma-buf.c | 13 +++++++------
    >>>> include/linux/dma-buf.h | 1 +
    >>>> 2 files changed, 8 insertions(+), 6 deletions(-)
    >>>>
    >>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
    >>>> index 01ce125..2e0456c 100644
    >>>> --- a/drivers/dma-buf/dma-buf.c
    >>>> +++ b/drivers/dma-buf/dma-buf.c
    >>>> @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry
    >*dentry,
    >>>> char *buffer, int buflen)
    >>>> size_t ret = 0;
    >>>>
    >>>> dmabuf = dentry->d_fsdata;
    >>>> - dma_resv_lock(dmabuf->resv, NULL);
    >>>> + spin_lock(&dmabuf->name_lock);
    >>>> if (dmabuf->name)
    >>>> ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
    >>>> - dma_resv_unlock(dmabuf->resv);
    >>>> + spin_unlock(&dmabuf->name_lock);
    >>>
    >>> I am not really clear on why you need this lock.
    >>>
    >>> If name == NULL you have no issues.
    >>> If name is real, you have no issues.
    >
    >Yeah, ideal cases...
    >
    >>>
    >>> If name is freed you will copy garbage, but the only way
    >>> for that to happen is that _set_name or _release have to be called
    >>> at just the right time.
    >>>
    >>> And the above would probably only be an issue if the set_name
    >>> was called, so you will get NULL or a real name.
    >
    >And there exists a use-after-free to avoid which requires the lock. Say
    >that memcpy() in dmabuffs_dname is in progress and in parallel _set_name
    >will free the same buffer that memcpy is operating on.

    Hmm... I can see that.

    However, note that in dma_buf_set_name, you cannot use the spinlock
    to protect the dma_buf->attachements list.

    I think you need to do this:

    dma_resv_lock(dmabuf->resv, NULL);
    if (!list_empty(&dmabuf->attachments)) {
    ret = -EBUSY;
    kfree(name);
    }
    dma_resv_unlock(dmabuf->resv, NULL);
    if (ret)
    return ret;

    spinlock(nam_lock)
    namestuff;
    spinunlock

    return 0;

    Mike

    >>> Is there a reason for the lock here?
    >>>
    >>> Mike
    >>
    >> Maybe dmabuf->name = NULL after the kfree(dmabuf->name) in:
    >>
    >> dma_buf_release()
    >>
    >> Would be sufficient?
    >
    >I don't think that we will access the 'dmabuf'(thus dmabuf->name) once
    >it is in the dma_buf_release(). So, setting the NULL in the _release()
    >is not required at all.
    >
    >>
    >> M
    >>>> return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
    >>>> dentry->d_name.name, ret > 0 ? name : "");
    >>>> @@ -335,7 +335,7 @@ static long dma_buf_set_name(struct dma_buf
    >>>> *dmabuf, const char __user *buf)
    >>>> if (IS_ERR(name))
    >>>> return PTR_ERR(name);
    >>>>
    >>>> - dma_resv_lock(dmabuf->resv, NULL);
    >>>> + spin_lock(&dmabuf->name_lock);
    >>>> if (!list_empty(&dmabuf->attachments)) {
    >>>> ret = -EBUSY;
    >>>> kfree(name);
    >>>> @@ -345,7 +345,7 @@ static long dma_buf_set_name(struct dma_buf
    >>>> *dmabuf, const char __user *buf)
    >>>> dmabuf->name = name;
    >>>>
    >>>> out_unlock:
    >>>> - dma_resv_unlock(dmabuf->resv);
    >>>> + spin_unlock(&dmabuf->name_lock);
    >>>> return ret;
    >>>> }
    >>>>
    >>>> @@ -405,10 +405,10 @@ static void dma_buf_show_fdinfo(struct
    >seq_file
    >>>> *m, struct file *file)
    >>>> /* Don't count the temporary reference taken inside procfs seq_show
    >>>> */
    >>>> seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
    >>>> seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
    >>>> - dma_resv_lock(dmabuf->resv, NULL);
    >>>> + spin_lock(&dmabuf->name_lock);
    >>>> if (dmabuf->name)
    >>>> seq_printf(m, "name:\t%s\n", dmabuf->name);
    >>>> - dma_resv_unlock(dmabuf->resv);
    >>>> + spin_unlock(&dmabuf->name_lock);
    >>>> }
    >>>>
    >>>> static const struct file_operations dma_buf_fops = {
    >>>> @@ -546,6 +546,7 @@ struct dma_buf *dma_buf_export(const struct
    >>>> dma_buf_export_info *exp_info)
    >>>> dmabuf->size = exp_info->size;
    >>>> dmabuf->exp_name = exp_info->exp_name;
    >>>> dmabuf->owner = exp_info->owner;
    >>>> + spin_lock_init(&dmabuf->name_lock);
    >>>> init_waitqueue_head(&dmabuf->poll);
    >>>> dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
    >>>> dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
    >>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
    >>>> index ab0c156..93108fd 100644
    >>>> --- a/include/linux/dma-buf.h
    >>>> +++ b/include/linux/dma-buf.h
    >>>> @@ -311,6 +311,7 @@ struct dma_buf {
    >>>> void *vmap_ptr;
    >>>> const char *exp_name;
    >>>> const char *name;
    >>>> + spinlock_t name_lock;
    >>>> struct module *owner;
    >>>> struct list_head list_node;
    >>>> void *priv;
    >>>> --
    >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
    >>>> Forum, a Linux Foundation Collaborative Project
    >>>> _______________________________________________
    >>>> dri-devel mailing list
    >>>> dri-devel@lists.freedesktop.org
    >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
    >>> _______________________________________________
    >>> dri-devel mailing list
    >>> dri-devel@lists.freedesktop.org
    >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
    >
    >--
    >The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
    >Forum, a Linux Foundation Collaborative Project
    \
     
     \ /
      Last update: 2020-06-17 19:44    [W:8.453 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site