>-----Original Message----- >From: charante=codeaurora.org@xxxxxxxxxxxxxxxxx ><charante=codeaurora.org@xxxxxxxxxxxxxxxxx> On Behalf Of Charan Teja >Kalla >Sent: Monday, June 22, 2020 5:26 AM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; Sumit Semwal ><sumit.semwal@xxxxxxxxxx>; 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: Re: [PATCH v2] dmabuf: use spinlock to access dmabuf->name > >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? That is the name that was being freed in the error path of the lock block. Alternatively: dma_resv_lock(resv) if (!list_empty(attachment) { ret = -EBUSY kfree(name) } dma_resv_unlock(resv) if (ret) return ret; I was limiting what was happening in the lock block. You have two distinct locks, that protect two distinct items: dmabuf->attachment dmabuf->name Nesting the locking is ok, but if the code ever changes you can get that nesting wrong, so: long ret = 0; if (IS_ERR(name)) return PTR_ERR(name); dma_resv_lock(dmabuf->resv, NULL); if (!list_empty(&dmabuf->attachments)) { ret = -EBUSY; kfree(name); } dma_resv_unlock(dmabuf->resv); if (ret) return ret; spinlock(dmabuf->name_lock) kfree(dmabuf->name); dmabuf->name = name; spinunlock(dmabuf->name_lock) return 0; } M > >> >> 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
