From f03a6783a0cd11a11ea565db271a3b1894c7d2aa Mon Sep 17 00:00:00 2001 From: Liam Mark Date: Fri, 28 Aug 2020 17:32:50 -0700 Subject: [PATCH] dma-buf: Cache the dma-buf inode number Now that dma-buf has switched to handling its release from the file_operations' release function to the dentry_operations' d_release function there is a potential use-after-free when accessing the buf_obj->file->f_inode as the path called by __fput eventually calls __dentry_kill which frees the inode before calling the dentry_operations' d_release function, so buf_obj->file->f_inode may no longer be valid. It doesn't look like there is a clean way to check if the inode is valid. For example using dentry->d_lock to look at the dentry inode would first involve getting the dentry via a call to file_dentry which potentially uses buf_obj->file->f_inode and it may no longer be valid. It looks safer to simply cache the inode number since it is a useful value to have as it identifies dma-buf objects. KASAN log gave the following: KASAN: use-after-free in dma_buf_debug_show+0x190/0x660 E : Read of size 8 at addr ffffff81051d7740 by task cat/19499 I Call trace: I : dump_backtrace+0x0/0x2c8 I : show_stack+0x20/0x2c I : dump_stack+0x154/0x1f4 I : print_address_description+0x84/0x580 I : __kasan_report+0x1b8/0x218 I : kasan_report+0x10/0x18 I : __asan_load8+0x90/0x98 I : dma_buf_debug_show+0x190/0x660 I : seq_read+0x2dc/0x830 I : full_proxy_read+0xbc/0x110 I : __vfs_read+0xd8/0x3d0 I : vfs_read+0x158/0x278 I : ksys_read+0x114/0x1e0 I : __arm64_sys_read+0x4c/0x5c I : el0_svc_common+0x130/0x294 I : el0_svc_handler+0xdc/0xec I : el0_svc+0x8/0xc E : Allocated by task 912: I : __kasan_kmalloc+0xfc/0x1b8 I : kasan_slab_alloc+0x14/0x1c I : kmem_cache_alloc+0x320/0x3c4 I : new_inode_pseudo+0xbc/0x168 I : alloc_anon_inode+0x28/0x170 I : dma_buf_export+0x1e0/0x3ec I : ion_dmabuf_alloc+0x168/0x1d4 I : ion_ioctl+0x24c/0x5c8 I : do_vfs_ioctl+0x55c/0xcb4 I : __arm64_sys_ioctl+0xa0/0xe4 I : el0_svc_common+0x130/0x294 I : el0_svc_handler+0xdc/0xec I : el0_svc+0x8/0xc E : Freed by task 40: I : __kasan_slab_free+0x160/0x22c I : kasan_slab_free+0x10/0x1c I : slab_free_freelist_hook+0xdc/0x15c I : kmem_cache_free+0xf8/0x350 I : i_callback+0x48/0x220 I : rcu_do_batch+0x2e8/0x594 I : nocb_cb_wait+0x158/0x930 I : rcu_nocb_cb_kthread+0x20/0x44 I : kthread+0x1ec/0x204 Change-Id: Ic3cfabf3524d2d34fe8b341e84f2ff259e4fad63 Signed-off-by: Liam Mark --- drivers/dma-buf/dma-buf.c | 6 +++--- include/linux/dma-buf.h | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 2976c7f139ef..063197afb1e4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -63,8 +63,7 @@ static int dma_buf_invoke_dtor(struct dma_buf *dmabuf) ret = dmabuf->dtor(dmabuf, dmabuf->dtor_data); if (ret < 0) pr_warn_ratelimited("Leaking dmabuf ino: %ld because destructor failed error: %d\n", - file_inode(dmabuf->file)->i_ino, - ret); + to_msm_dma_buf(dmabuf)->i_ino, ret); } return ret; @@ -587,6 +586,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) ret = PTR_ERR(file); goto err_dmabuf; } + msm_dma_buf->i_ino = file_inode(file)->i_ino; file->f_mode |= FMODE_LSEEK; dmabuf->file = file; @@ -1288,7 +1288,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) buf_obj->file->f_flags, buf_obj->file->f_mode, file_count(buf_obj->file), buf_obj->exp_name, - file_inode(buf_obj->file)->i_ino, + to_msm_dma_buf(buf_obj)->i_ino, buf_obj->name ?: ""); robj = buf_obj->resv; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 75420ce0bd81..a7da54d72150 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -427,10 +427,12 @@ struct dma_buf { * struct msm_dma_buf - Holds the meta data associated with a shared buffer * object, as well as the buffer object. * @refs: list entry for dma-buf reference tracking + * @i_ino: inode number * @dma_buf: the shared buffer object */ struct msm_dma_buf { struct list_head refs; + unsigned long i_ino; struct dma_buf dma_buf; };