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

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

[Charan Teja Reddy]

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")
Reported-by: syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com
Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
---
 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);
+	}
+}
+
 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;
+	}
+	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;
 
 	/* poll support */
 	wait_queue_head_t poll;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

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

[Greg KH]

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")

> 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?

And finally, no cc: stable in the s-o-b area for an issue that needs to
be backported to older kernels?


> ---
>  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?

> +}
> +
>  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?

> +	}
> +	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?

thanks,

greg k-h

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

[Charan Teja Kalla]

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

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

[Charan Teja Kalla]

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

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

[Greg KH]

On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
> 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.

Ok, but watch out, now you have 2 different reference counts for the
same structure.  Keeping them coordinated is almost always an impossible
task so you need to only rely on one.  If you can't use the file api,
just drop all of the reference counting logic in there and only use the
kref one.

good luck!

greg k-h

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

[Greg KH]

On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
> 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.

Ok, but watch out, now you have 2 different reference counts for the
same structure.  Keeping them coordinated is almost always an impossible
task so you need to only rely on one.  If you can't use the file api,
just drop all of the reference counting logic in there and only use the
kref one.

good luck!

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Charan Teja Kalla]

Thank you Greg for the comments.

On 5/6/2020 2:30 PM, Greg KH wrote:
> On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
>> 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.
> Ok, but watch out, now you have 2 different reference counts for the
> same structure.  Keeping them coordinated is almost always an impossible
> task so you need to only rely on one.  If you can't use the file api,
> just drop all of the reference counting logic in there and only use the
> kref one.

I feel that changing the refcount logic now to dma-buf objects involve 
changes in

the core dma-buf framework. NO? Instead, how about passing the user 
passed name directly

in the ->d_fsdata inplace of dmabuf object? Because we just need user 
passed name in the

dmabuffs_dname(). With this we can avoid the need for extra refcount on 
dmabuf.

Posted patch-V2: https://lkml.org/lkml/2020/5/8/158


>
> good luck!
>
> greg k-h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

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

[Charan Teja Kalla]

Thank you Greg for the comments.

On 5/6/2020 2:30 PM, Greg KH wrote:
> On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
>> 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.
> Ok, but watch out, now you have 2 different reference counts for the
> same structure.  Keeping them coordinated is almost always an impossible
> task so you need to only rely on one.  If you can't use the file api,
> just drop all of the reference counting logic in there and only use the
> kref one.

I feel that changing the refcount logic now to dma-buf objects involve 
changes in

the core dma-buf framework. NO? Instead, how about passing the user 
passed name directly

in the ->d_fsdata inplace of dmabuf object? Because we just need user 
passed name in the

dmabuffs_dname(). With this we can avoid the need for extra refcount on 
dmabuf.

Posted patch-V2: https://lkml.org/lkml/2020/5/8/158


>
> good luck!
>
> greg k-h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Greg KH]

On Tue, May 12, 2020 at 10:43:18AM +0530, Charan Teja Kalla wrote:
> > Ok, but watch out, now you have 2 different reference counts for the
> > same structure.  Keeping them coordinated is almost always an impossible
> > task so you need to only rely on one.  If you can't use the file api,
> > just drop all of the reference counting logic in there and only use the
> > kref one.
> 
> I feel that changing the refcount logic now to dma-buf objects involve
> changes in
> 
> the core dma-buf framework. NO? Instead, how about passing the user passed
> name directly
> 
> in the ->d_fsdata inplace of dmabuf object? Because we just need user passed
> name in the
> 
> dmabuffs_dname(). With this we can avoid the need for extra refcount on
> dmabuf.

Odd formatting :(

> Posted patch-V2: https://lkml.org/lkml/2020/5/8/158

Please just post links to lore.kernel.org, we have no control over
lkml.org at all.

I'll go review that patch now...

greg k-h

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

[Greg KH]

On Tue, May 12, 2020 at 10:43:18AM +0530, Charan Teja Kalla wrote:
> > Ok, but watch out, now you have 2 different reference counts for the
> > same structure.  Keeping them coordinated is almost always an impossible
> > task so you need to only rely on one.  If you can't use the file api,
> > just drop all of the reference counting logic in there and only use the
> > kref one.
> 
> I feel that changing the refcount logic now to dma-buf objects involve
> changes in
> 
> the core dma-buf framework. NO? Instead, how about passing the user passed
> name directly
> 
> in the ->d_fsdata inplace of dmabuf object? Because we just need user passed
> name in the
> 
> dmabuffs_dname(). With this we can avoid the need for extra refcount on
> dmabuf.

Odd formatting :(

> Posted patch-V2: https://lkml.org/lkml/2020/5/8/158

Please just post links to lore.kernel.org, we have no control over
lkml.org at all.

I'll go review that patch now...

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel