| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 May 2020 14:00:10 +0530
From: Charan Teja Kalla <charante@...eaurora.org>
To: Greg KH <gregkh@...uxfoundation.org>,
dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
linux-kernel@...r.kernel.org
Cc: vinmenon@...eaurora.org, sumit.semwal@...aro.org,
ghackmann@...gle.com, fengc@...gle.com, linux-media@...r.kernel.org
Subject: Re: [PATCH] dma-buf: fix use-after-free in dmabuffs_dname
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@...kaller.appspotmail.com
>> Signed-off-by: Charan Teja Reddy<charante@...eaurora.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
Powered by blists - more mailing lists