Messages in this thread | | | Subject | Re: [Linaro-mm-sig] [PATCH] dmabuf: fix use-after-free of dmabuf's file->f_inode | From | Christian König <> | Date | Mon, 4 Jan 2021 12:52:44 +0100 |
| |
Am 04.01.21 um 12:36 schrieb Charan Teja Reddy: > It is observed 'use-after-free' on the dmabuf's file->f_inode with the > race between closing the dmabuf file and reading the dmabuf's debug > info. > > Consider the below scenario where P1 is closing the dma_buf file > and P2 is reading the dma_buf's debug info in the system: > > P1 P2 > dma_buf_debug_show() > dma_buf_put() > __fput() > file->f_op->release() > dput() > .... > dentry_unlink_inode() > iput(dentry->d_inode) > (where the inode is freed) > mutex_lock(&db_list.lock) > read 'dma_buf->file->f_inode' > (the same inode is freed by P1) > mutex_unlock(&db_list.lock) > dentry->d_op->d_release()--> > dma_buf_release() > ..... > mutex_lock(&db_list.lock) > removes the dmabuf from the list > mutex_unlock(&db_list.lock) > > In the above scenario, when dma_buf_put() is called on a dma_buf, it > first frees the dma_buf's file->f_inode(=dentry->d_inode) and then > removes this dma_buf from the system db_list. In between P2 traversing > the db_list tries to access this dma_buf's file->f_inode that was freed > by P1 which is a use-after-free case. > > Since, __fput() calls f_op->release first and then later calls the > d_op->d_release, move the dma_buf's db_list removal from d_release() to > f_op->release(). This ensures that dma_buf's file->f_inode is not > accessed after it is released. > > Fixes: 4ab59c3c638c ("dma-buf: Move dma_buf_release() from fops to dentry_ops") > Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
Not an expert on the debugfs stuff in DMA-buf, but the explanation sounds perfectly correct to me.
Acked-by: Christian König <christian.koenig@amd.com>
> --- > drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 0eb80c1..a14dcbb 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,10 +76,6 @@ static void dma_buf_release(struct dentry *dentry) > > dmabuf->ops->release(dmabuf); > > - mutex_lock(&db_list.lock); > - list_del(&dmabuf->list_node); > - mutex_unlock(&db_list.lock); > - > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > @@ -88,6 +84,22 @@ static void dma_buf_release(struct dentry *dentry) > kfree(dmabuf); > } > > +static int dma_buf_file_release(struct inode *inode, struct file *file) > +{ > + struct dma_buf *dmabuf; > + > + if (!is_dma_buf_file(file)) > + return -EINVAL; > + > + dmabuf = file->private_data; > + > + mutex_lock(&db_list.lock); > + list_del(&dmabuf->list_node); > + mutex_unlock(&db_list.lock); > + > + return 0; > +} > + > static const struct dentry_operations dma_buf_dentry_ops = { > .d_dname = dmabuffs_dname, > .d_release = dma_buf_release, > @@ -413,6 +425,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > } > > static const struct file_operations dma_buf_fops = { > + .release = dma_buf_file_release, > .mmap = dma_buf_mmap_internal, > .llseek = dma_buf_llseek, > .poll = dma_buf_poll,
| |