Re: [PATCH] dma-buf: fix use-after-free in dmabuffs_dname

[Charan Teja Kalla <charante@codeaurora.org>]

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