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

[Greg KH <gregkh@linuxfoundation.org>]

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