>-----Original Message-----
>From: charante=codeaurora.org at mg.codeaurora.org
><charante=codeaurora.org at mg.codeaurora.org> On Behalf Of Charan Teja
>Kalla
>Sent: Friday, June 19, 2020 7:57 AM
>To: Sumit Semwal <sumit.semwal at linaro.org>; Ruhl, Michael J
><michael.j.ruhl at intel.com>; David.Laight at ACULAB.COM; open list:DMA
>BUFFER SHARING FRAMEWORK <linux-media at vger.kernel.org>; DRI mailing
>list <dri-devel at lists.freedesktop.org>
>Cc: Linaro MM SIG <linaro-mm-sig at lists.linaro.org>; LKML <linux-
>kernel at vger.kernel.org>
>Subject: [PATCH v2] 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 at vger.kernel.org> [5.3+]
>Signed-off-by: Charan Teja Reddy <charante at codeaurora.org>
>---
>
>Changes in V2: Addressed review comments from Ruhl, Michael J
>
>Changes in V1: https://lore.kernel.org/patchwork/patch/1255055/
>
> drivers/dma-buf/dma-buf.c | 11 +++++++----
> include/linux/dma-buf.h | 1 +
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>index 01ce125..d81d298 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);
>
> return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
> dentry->d_name.name, ret > 0 ? name : "");
>@@ -341,8 +341,10 @@ static long dma_buf_set_name(struct dma_buf
>*dmabuf, const char __user *buf)
> kfree(name);
> goto out_unlock;
> }
>+ spin_lock(&dmabuf->name_lock);
> kfree(dmabuf->name);
> dmabuf->name = name;
>+ spin_unlock(&dmabuf->name_lock);
While this code path is ok, I would have separated the protection of the
attachment list and the name manipulation.
dma_resv_lock(resv)
if (!list_empty(attachment)
ret = -EBUSY
dma_resv_unlock(resv)
if (ret) {
kfree(name)
return ret;
}
spinlock(nam_lock)
...
Nesting locks that don't need to be nested always makes me nervous
for future use that misses the lock/unlock pattern.
However, this looks reasonable.
With this current code, or if you update to the above pattern:
Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
Mike
> out_unlock:
> dma_resv_unlock(dmabuf->resv);
>@@ -405,10 +407,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 +548,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