Messages in this thread | | | From | Charan Teja Kalla <> | Subject | Re: [PATCH] dma-buf: fix use-after-free in dmabuffs_dname | Date | Wed, 6 May 2020 14:00:10 +0530 |
| |
Thank you Greg for the reply.
On 5/5/2020 3:38 PM, Greg KH wrote: > On Tue, Apr 28, 2020 at 01:24:02PM +0530, Charan Teja Reddy wrote: >> The following race occurs while accessing the dmabuf object exported as >> file: >> P1 P2 >> dma_buf_release() dmabuffs_dname() >> [say lsof reading /proc/<P1 pid>/fd/<num>] >> >> read dmabuf stored in dentry->fsdata >> Free the dmabuf object >> Start accessing the dmabuf structure >> >> In the above description, the dmabuf object freed in P1 is being >> accessed from P2 which is resulting into the use-after-free. Below is >> the dump stack reported. >> >> Call Trace: >> kasan_report+0x12/0x20 >> __asan_report_load8_noabort+0x14/0x20 >> dmabuffs_dname+0x4f4/0x560 >> tomoyo_realpath_from_path+0x165/0x660 >> tomoyo_get_realpath >> tomoyo_check_open_permission+0x2a3/0x3e0 >> tomoyo_file_open >> tomoyo_file_open+0xa9/0xd0 >> security_file_open+0x71/0x300 >> do_dentry_open+0x37a/0x1380 >> vfs_open+0xa0/0xd0 >> path_openat+0x12ee/0x3490 >> do_filp_open+0x192/0x260 >> do_sys_openat2+0x5eb/0x7e0 >> do_sys_open+0xf2/0x180 >> >> Fixes: bb2bb90 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > Nit, please read the documentation for how to do a Fixes: line properly, > you need more digits: > Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
Will update the patch
>> Reported-by:syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com >> Signed-off-by: Charan Teja Reddy<charante@codeaurora.org> > Also, any reason you didn't include the other mailing lists that > get_maintainer.pl said to?
Really sorry for not sending to complete list. Added now.
> And finally, no cc: stable in the s-o-b area for an issue that needs to > be backported to older kernels?
Will update the patch.
> >> --- >> drivers/dma-buf/dma-buf.c | 25 +++++++++++++++++++++++-- >> include/linux/dma-buf.h | 1 + >> 2 files changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 570c923..069d8f78 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -25,6 +25,7 @@ >> #include <linux/mm.h> >> #include <linux/mount.h> >> #include <linux/pseudo_fs.h> >> +#include <linux/dcache.h> >> >> #include <uapi/linux/dma-buf.h> >> #include <uapi/linux/magic.h> >> @@ -38,18 +39,34 @@ struct dma_buf_list { >> >> static struct dma_buf_list db_list; >> >> +static void dmabuf_dent_put(struct dma_buf *dmabuf) >> +{ >> + if (atomic_dec_and_test(&dmabuf->dent_count)) { >> + kfree(dmabuf->name); >> + kfree(dmabuf); >> + } > Why not just use a kref instead of an open-coded atomic value?
Kref approach looks cleaner. will update the patch accordingly.
>> +} >> + >> static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) >> { >> struct dma_buf *dmabuf; >> char name[DMA_BUF_NAME_LEN]; >> size_t ret = 0; >> >> + spin_lock(&dentry->d_lock); >> dmabuf = dentry->d_fsdata; >> + if (!dmabuf || !atomic_add_unless(&dmabuf->dent_count, 1, 0)) { >> + spin_unlock(&dentry->d_lock); >> + goto out; > How can dmabuf not be valid here? > > And isn't there already a usecount for the buffer?
dmabuf exported as file simply relies on that file's refcount, thus fput() releases the dmabuf.
We are storing the dmabuf in the dentry->d_fsdata but there is no binding between the dentry and the dmabuf. So, flow will be like
1) P1 calls fput(dmabuf_fd)
2) P2 trying to access the file information of P1. Eg: lsof command trying to list out the dmabuf_fd information using /proc/<P1 pid>/fd/dmabuf_fd
3) P1 calls the file->f_op->release(dmabuf_fd_file)(ends up in calling dma_buf_release()), thus frees up the dmabuf buffer.
4) P2 access the dmabuf stored in the dentry->d_fsdata which was freed in step 3.
So we need to have some refcount mechanism to avoid the use-after-free in step 4.
>> + } >> + spin_unlock(&dentry->d_lock); >> dma_resv_lock(dmabuf->resv, NULL); >> if (dmabuf->name) >> ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); >> dma_resv_unlock(dmabuf->resv); >> + dmabuf_dent_put(dmabuf); >> >> +out: >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", >> dentry->d_name.name, ret > 0 ? name : ""); >> } >> @@ -80,12 +97,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) >> static int dma_buf_release(struct inode *inode, struct file *file) >> { >> struct dma_buf *dmabuf; >> + struct dentry *dentry = file->f_path.dentry; >> >> if (!is_dma_buf_file(file)) >> return -EINVAL; >> >> dmabuf = file->private_data; >> >> + spin_lock(&dentry->d_lock); >> + dentry->d_fsdata = NULL; >> + spin_unlock(&dentry->d_lock); >> BUG_ON(dmabuf->vmapping_counter); >> >> /* >> @@ -108,8 +129,7 @@ static int dma_buf_release(struct inode *inode, struct file *file) >> dma_resv_fini(dmabuf->resv); >> >> module_put(dmabuf->owner); >> - kfree(dmabuf->name); >> - kfree(dmabuf); >> + dmabuf_dent_put(dmabuf); >> return 0; >> } >> >> @@ -548,6 +568,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) >> init_waitqueue_head(&dmabuf->poll); >> dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll; >> dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0; >> + atomic_set(&dmabuf->dent_count, 1); >> >> if (!resv) { >> resv = (struct dma_resv *)&dmabuf[1]; >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index 82e0a4a..a259042 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -315,6 +315,7 @@ struct dma_buf { >> struct list_head list_node; >> void *priv; >> struct dma_resv *resv; >> + atomic_t dent_count; > Isn't there other usage counters here that can support this? Adding > another one feels wrong as now we have multiple use counts, right?
I couldn't see any refcount exist and couldn't find the need either, earlier.
But the change bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") requires to have one.
> thanks, > > greg k-h
| |