-----Original Message----- From: charante=codeaurora.org@mg.codeaurora.org charante=codeaurora.org@mg.codeaurora.org On Behalf Of Charan Teja Kalla Sent: Friday, June 19, 2020 7:57 AM To: Sumit Semwal sumit.semwal@linaro.org; Ruhl, Michael J michael.j.ruhl@intel.com; David.Laight@ACULAB.COM; 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; LKML <linux- kernel@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@vger.kernel.org [5.3+] Signed-off-by: Charan Teja Reddy charante@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@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