From: Sumit Semwal <sumit.semwal@linaro.org> To: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org Cc: Chenbo Feng <fengc@google.com>, linux-kernel@vger.kernel.org, linaro-mm-sig@lists.linaro.org, Charan Teja Reddy <charante@codeaurora.org>, Sumit Semwal <sumit.semwal@linaro.org>, syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com, stable@vger.kernel.org, Arnd Bergmann <arnd@arndb.de> Subject: [PATCH] dma-buf: Move dma_buf_release() from fops to dentry_ops Date: Wed, 10 Jun 2020 14:03:33 +0530 [thread overview] Message-ID: <20200610083333.455-1-sumit.semwal@linaro.org> (raw) Charan Teja reported a 'use-after-free' in dmabuffs_dname [1], which happens if the dma_buf_release() is called while the userspace is accessing the dma_buf pseudo fs's dmabuffs_dname() in another process, and dma_buf_release() releases the dmabuf object when the last reference to the struct file goes away. I discussed with Arnd Bergmann, and he suggested that rather than tying the dma_buf_release() to the file_operations' release(), we can tie it to the dentry_operations' d_release(), which will be called when the last ref to the dentry is removed. The path exercised by __fput() calls f_op->release() first, and then calls dput, which eventually calls d_op->d_release(). In the 'normal' case, when no userspace access is happening via dma_buf pseudo fs, there should be exactly one fd, file, dentry and inode, so closing the fd will kill of everything right away. In the presented case, the dentry's d_release() will be called only when the dentry's last ref is released. Therefore, lets move dma_buf_release() from fops->release() to d_ops->d_release(). Many thanks to Arnd for his FS insights :) [1]: https://lore.kernel.org/patchwork/patch/1238278/ Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") Reported-by: syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com Cc: <stable@vger.kernel.org> [5.3+] Cc: Arnd Bergmann <arnd@arndb.de> Reported-by: Charan Teja Reddy <charante@codeaurora.org> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org> --- drivers/dma-buf/dma-buf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 01ce125f8e8d..92ba4b6ef3e7 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -54,8 +54,11 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) dentry->d_name.name, ret > 0 ? name : ""); } +static void dma_buf_release(struct dentry *dentry); + static const struct dentry_operations dma_buf_dentry_ops = { .d_dname = dmabuffs_dname, + .d_release = dma_buf_release, }; static struct vfsmount *dma_buf_mnt; @@ -77,14 +80,14 @@ static struct file_system_type dma_buf_fs_type = { .kill_sb = kill_anon_super, }; -static int dma_buf_release(struct inode *inode, struct file *file) +static void dma_buf_release(struct dentry *dentry) { struct dma_buf *dmabuf; - if (!is_dma_buf_file(file)) - return -EINVAL; + if (dentry->d_op != &dma_buf_dentry_ops) + return; - dmabuf = file->private_data; + dmabuf = dentry->d_fsdata; BUG_ON(dmabuf->vmapping_counter); @@ -110,7 +113,6 @@ static int dma_buf_release(struct inode *inode, struct file *file) module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); - return 0; } static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) @@ -412,7 +414,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) } static const struct file_operations dma_buf_fops = { - .release = dma_buf_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, -- 2.27.0
WARNING: multiple messages have this Message-ID (diff)
From: Sumit Semwal <sumit.semwal@linaro.org> To: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org Cc: Arnd Bergmann <arnd@arndb.de>, Chenbo Feng <fengc@google.com>, linux-kernel@vger.kernel.org, stable@vger.kernel.org, linaro-mm-sig@lists.linaro.org, syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com, Charan Teja Reddy <charante@codeaurora.org> Subject: [PATCH] dma-buf: Move dma_buf_release() from fops to dentry_ops Date: Wed, 10 Jun 2020 14:03:33 +0530 [thread overview] Message-ID: <20200610083333.455-1-sumit.semwal@linaro.org> (raw) Charan Teja reported a 'use-after-free' in dmabuffs_dname [1], which happens if the dma_buf_release() is called while the userspace is accessing the dma_buf pseudo fs's dmabuffs_dname() in another process, and dma_buf_release() releases the dmabuf object when the last reference to the struct file goes away. I discussed with Arnd Bergmann, and he suggested that rather than tying the dma_buf_release() to the file_operations' release(), we can tie it to the dentry_operations' d_release(), which will be called when the last ref to the dentry is removed. The path exercised by __fput() calls f_op->release() first, and then calls dput, which eventually calls d_op->d_release(). In the 'normal' case, when no userspace access is happening via dma_buf pseudo fs, there should be exactly one fd, file, dentry and inode, so closing the fd will kill of everything right away. In the presented case, the dentry's d_release() will be called only when the dentry's last ref is released. Therefore, lets move dma_buf_release() from fops->release() to d_ops->d_release(). Many thanks to Arnd for his FS insights :) [1]: https://lore.kernel.org/patchwork/patch/1238278/ Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") Reported-by: syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com Cc: <stable@vger.kernel.org> [5.3+] Cc: Arnd Bergmann <arnd@arndb.de> Reported-by: Charan Teja Reddy <charante@codeaurora.org> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org> --- drivers/dma-buf/dma-buf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 01ce125f8e8d..92ba4b6ef3e7 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -54,8 +54,11 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) dentry->d_name.name, ret > 0 ? name : ""); } +static void dma_buf_release(struct dentry *dentry); + static const struct dentry_operations dma_buf_dentry_ops = { .d_dname = dmabuffs_dname, + .d_release = dma_buf_release, }; static struct vfsmount *dma_buf_mnt; @@ -77,14 +80,14 @@ static struct file_system_type dma_buf_fs_type = { .kill_sb = kill_anon_super, }; -static int dma_buf_release(struct inode *inode, struct file *file) +static void dma_buf_release(struct dentry *dentry) { struct dma_buf *dmabuf; - if (!is_dma_buf_file(file)) - return -EINVAL; + if (dentry->d_op != &dma_buf_dentry_ops) + return; - dmabuf = file->private_data; + dmabuf = dentry->d_fsdata; BUG_ON(dmabuf->vmapping_counter); @@ -110,7 +113,6 @@ static int dma_buf_release(struct inode *inode, struct file *file) module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); - return 0; } static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) @@ -412,7 +414,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) } static const struct file_operations dma_buf_fops = { - .release = dma_buf_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next reply other threads:[~2020-06-10 8:33 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-10 8:33 Sumit Semwal [this message] 2020-06-10 8:33 ` [PATCH] dma-buf: Move dma_buf_release() from fops to dentry_ops Sumit Semwal 2020-06-10 9:26 ` Arnd Bergmann 2020-06-10 9:26 ` Arnd Bergmann 2020-06-10 15:14 ` Sumit Semwal 2020-06-10 15:14 ` Sumit Semwal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200610083333.455-1-sumit.semwal@linaro.org \ --to=sumit.semwal@linaro.org \ --cc=arnd@arndb.de \ --cc=charante@codeaurora.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=fengc@google.com \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=stable@vger.kernel.org \ --cc=syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.