Hello Mike, On 6/19/2020 7:11 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: charante=codeaurora.org@xxxxxxxxxxxxxxxxx >> <charante=codeaurora.org@xxxxxxxxxxxxxxxxx> On Behalf Of Charan Teja >> Kalla >> Sent: Friday, June 19, 2020 7:57 AM >> To: Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Ruhl, Michael J >> <michael.j.ruhl@xxxxxxxxx>; David.Laight@xxxxxxxxxx; open list:DMA >> BUFFER SHARING FRAMEWORK <linux-media@xxxxxxxxxxxxxxx>; DRI mailing >> list <dri-devel@xxxxxxxxxxxxxxxxxxxxx> >> Cc: Linaro MM SIG <linaro-mm-sig@xxxxxxxxxxxxxxxx>; LKML <linux- >> kernel@xxxxxxxxxxxxxxx> >> 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@xxxxxxxxxxxxxxx> [5.3+] >> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx> >> --- >> >> 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; > } Is it that the name should be visible before importer attaches to the dmabuf,(using dma_buf_attach()), thus _buf_set_name() is under the _resv_lock() as well? > > 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@xxxxxxxxx> Thanks for the ACK. > > 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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
