Messages in this thread | | | Subject | Re: [PATCH] configfs: Fix use-after-free issue in __configfs_open_file | From | Chao Yu <> | Date | Fri, 5 Mar 2021 11:29:20 +0800 |
| |
+Cc fsdevel
Ping,
Any comments one this patch?
On 2021/3/1 14:10, Chao Yu wrote: > From: Daiyue Zhang <zhangdaiyue1@huawei.com> > > Commit b0841eefd969 ("configfs: provide exclusion between IO and removals") > uses ->frag_dead to mark the fragment state, thus no bothering with extra > refcount on config_item when opening a file. The configfs_get_config_item > was removed in __configfs_open_file, but not with config_item_put. So the > refcount on config_item will lost its balance, causing use-after-free > issues in some occasions like this: > > Test: > 1. Mount configfs on /config with read-only items: > drwxrwx--- 289 root root 0 2021-04-01 11:55 /config > drwxr-xr-x 2 root root 0 2021-04-01 11:54 /config/a > --w--w--w- 1 root root 4096 2021-04-01 11:53 /config/a/1.txt > ...... > > 2. Then run: > for file in /config > do > echo $file > grep -R 'key' $file > done > > 3. __configfs_open_file will be called in parallel, the first one > got called will do: > if (file->f_mode & FMODE_READ) { > if (!(inode->i_mode & S_IRUGO)) > goto out_put_module; > config_item_put(buffer->item); > kref_put() > package_details_release() > kfree() > > the other one will run into use-after-free issues like this: > BUG: KASAN: use-after-free in __configfs_open_file+0x1bc/0x3b0 > Read of size 8 at addr fffffff155f02480 by task grep/13096 > CPU: 0 PID: 13096 Comm: grep VIP: 00 Tainted: G W 4.14.116-kasan #1 > TGID: 13096 Comm: grep > Call trace: > dump_stack+0x118/0x160 > kasan_report+0x22c/0x294 > __asan_load8+0x80/0x88 > __configfs_open_file+0x1bc/0x3b0 > configfs_open_file+0x28/0x34 > do_dentry_open+0x2cc/0x5c0 > vfs_open+0x80/0xe0 > path_openat+0xd8c/0x2988 > do_filp_open+0x1c4/0x2fc > do_sys_open+0x23c/0x404 > SyS_openat+0x38/0x48 > > Allocated by task 2138: > kasan_kmalloc+0xe0/0x1ac > kmem_cache_alloc_trace+0x334/0x394 > packages_make_item+0x4c/0x180 > configfs_mkdir+0x358/0x740 > vfs_mkdir2+0x1bc/0x2e8 > SyS_mkdirat+0x154/0x23c > el0_svc_naked+0x34/0x38 > > Freed by task 13096: > kasan_slab_free+0xb8/0x194 > kfree+0x13c/0x910 > package_details_release+0x524/0x56c > kref_put+0xc4/0x104 > config_item_put+0x24/0x34 > __configfs_open_file+0x35c/0x3b0 > configfs_open_file+0x28/0x34 > do_dentry_open+0x2cc/0x5c0 > vfs_open+0x80/0xe0 > path_openat+0xd8c/0x2988 > do_filp_open+0x1c4/0x2fc > do_sys_open+0x23c/0x404 > SyS_openat+0x38/0x48 > el0_svc_naked+0x34/0x38 > > To fix this issue, remove the config_item_put in > __configfs_open_file to balance the refcount of config_item. > > Fixes: b0841eefd969 ("configfs: provide exclusion between IO and removals") > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Joel Becker <jlbec@evilplan.org> > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Daiyue Zhang <zhangdaiyue1@huawei.com> > Signed-off-by: Yi Chen <chenyi77@huawei.com> > Signed-off-by: Ge Qiu <qiuge@huawei.com> > Reviewed-by: Chao Yu <yuchao0@huawei.com> > --- > fs/configfs/file.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/configfs/file.c b/fs/configfs/file.c > index 1f0270229d7b..da8351d1e455 100644 > --- a/fs/configfs/file.c > +++ b/fs/configfs/file.c > @@ -378,7 +378,7 @@ static int __configfs_open_file(struct inode *inode, struct file *file, int type > > attr = to_attr(dentry); > if (!attr) > - goto out_put_item; > + goto out_free_buffer; > > if (type & CONFIGFS_ITEM_BIN_ATTR) { > buffer->bin_attr = to_bin_attr(dentry); > @@ -391,7 +391,7 @@ static int __configfs_open_file(struct inode *inode, struct file *file, int type > /* Grab the module reference for this attribute if we have one */ > error = -ENODEV; > if (!try_module_get(buffer->owner)) > - goto out_put_item; > + goto out_free_buffer; > > error = -EACCES; > if (!buffer->item->ci_type) > @@ -435,8 +435,6 @@ static int __configfs_open_file(struct inode *inode, struct file *file, int type > > out_put_module: > module_put(buffer->owner); > -out_put_item: > - config_item_put(buffer->item); > out_free_buffer: > up_read(&frag->frag_sem); > kfree(buffer); >
| |