>-----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; } 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> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
