On Wed, 13 May 2020 at 21:16, Daniel Vetter <dan...@ffwll.ch> wrote:
>
> On Wed, May 13, 2020 at 02:51:12PM +0200, Greg KH wrote:
> > On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote:
> > >
> > > Thank you Greg for the comments.
> > > On 5/12/2020 2:22 PM, Greg KH wrote:
> > > > On Fri, May 08, 2020 at 12:11:03PM +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->d_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.
> > > >>
> > > >> We are reading the dmabuf object stored in the dentry->d_fsdata but
> > > >> there is no binding between the dentry and the dmabuf which means that
> > > >> the dmabuf can be freed while it is being read from ->d_fsdata and
> > > >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse
> > > >> with an extra refcount is not a viable solution as the exported dmabuf
> > > >> is already under file's refcount and keeping the multiple refcounts on
> > > >> the same object coordinated is not possible.
> > > >>
> > > >> As we are reading the dmabuf in ->d_fsdata just to get the user passed
> > > >> name, we can directly store the name in d_fsdata thus can avoid the
> > > >> reading of dmabuf altogether.
> > > >>
> > > >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
> > > >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com
> > > >> Cc: <sta...@vger.kernel.org> [5.3+]
> > > >> Signed-off-by: Charan Teja Reddy <chara...@codeaurora.org>
> > > >> ---
> > > >>
> > > >> Changes in v2:
> > > >>
> > > >> - Pass the user passed name in ->d_fsdata instead of dmabuf
> > > >> - Improve the commit message
> > > >>
> > > >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/)
> > > >>
> > > >> drivers/dma-buf/dma-buf.c | 17 ++++++++++-------
> > > >> 1 file changed, 10 insertions(+), 7 deletions(-)
> > > >>
> > > >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > >> index 01ce125..0071f7d 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>
> > > >> @@ -40,15 +41,13 @@ struct dma_buf_list {
> > > >>
> > > >> 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;
> > > >>
> > > >> - dmabuf = dentry->d_fsdata;
> > > >> - dma_resv_lock(dmabuf->resv, NULL);
> > > >> - if (dmabuf->name)
> > > >> - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
> > > >> - dma_resv_unlock(dmabuf->resv);
> > > >> + spin_lock(&dentry->d_lock);
> > > >
> > > > Are you sure this lock always protects d_fsdata?
> > >
> > > I think yes. In the dma-buf.c, I have to make sure that d_fsdata should
> > > always be under d_lock thus it will be protected. (In this posted patch
> > > there is one place(in dma_buf_set_name) that is missed, will update this
> > > in V3).
> > >
> > > >
> > > >> + if (dentry->d_fsdata)
> > > >> + ret = strlcpy(name, dentry->d_fsdata,
> > > >> DMA_BUF_NAME_LEN);
> > > >> + spin_unlock(&dentry->d_lock);
> > > >>
> > > >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
> > > >> dentry->d_name.name, ret > 0 ? name :
> > > >> "");
> > > >
> > > > If the above check fails the name will be what? How could d_name.name
> > > > be valid but d_fsdata not be valid?
> > >
> > > In case of check fails, empty string "" is appended to the name by the
> > > code, ret > 0 ? name : "", ret is initialized to zero. Thus the name
> > > string will be like "/dmabuf:".
> >
> > So multiple objects can have the same "name" if this happens to multiple
> > ones at once?
> >
> > > Regarding the validity of d_fsdata, we are setting the dmabuf's
> > > dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if
> > > that dmabuf is in the free path.
> >
> > Why are we allowing the name to be set if the dmabuf is on the free path
> > at all? Shouldn't that be the real fix here?
> >
> > > >> @@ -80,12 +79,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);
> > > >>
> > > >> /*
> > > >> @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf
> > > >> *dmabuf, const char __user *buf)
> > > >> }
> > > >> kfree(dmabuf->name);
> > > >> dmabuf->name = name;
> > > >> + dmabuf->file->f_path.dentry->d_fsdata = name;
> > > >
> > > > You are just changing the use of d_fsdata from being a pointer to the
> > > > dmabuf to being a pointer to the name string? What's to keep that name
> > > > string around and not have the same reference counting issues that the
> > > > dmabuf structure itself has? Who frees that string memory?
> > > >
> > >
> > > Yes, I am just storing the name string in the d_fsdata in place of
> > > dmabuf and this helps to get rid of any extra refcount requirement.
> > > Because the user passed name carried in the d_fsdata is copied to the
> > > local buffer in dmabuffs_dname under spin_lock(d_lock) and the same
> > > d_fsdata is set to NULL(under the d_lock only) when that dmabuf is in
> > > the release path. So, when d_fsdata is NULL, name string is not accessed
> > > from the dmabuffs_dname thus extra count is not required.
> > >
> > > String memory, stored in the dmabuf->name, is released from the
> > > dma_buf_release(). Flow will be like, It fist sets d_fsdata=NULL and
> > > then free the dmabuf->name.
> > >
> > > However from your comments I have realized that there is a race in this
> > > patch when using the name string between dma_buf_set_name() and
> > > dmabuffs_dname(). But, If the idea of passing the name string inplace of
> > > dmabuf in d_fsdata looks fine, I can update this next patch.
> >
> > I'll leave that to the dmabuf authors/maintainers, but it feels odd to
> > me...
>
> I have zero clue about fs internals. This all scares me, that's all. I
> know enough about lifetime bugs that if you don't deeply understand a
> subsystem, all that's guaranteed is that you will get it wrong.
Likewise, and that made me realise that the 'fix' may not be as
innocuous or quick.
I will try to check with some folks more experienced than me in the fs
domain and see what is the logical way to handle it.
>
> /me out
>
> Cheers, Daniel
>
Best,
Sumit.
