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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx>
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
