linux-kernel - Re: [PATCH] configfs: Fix use-after-free issue in __configfs_open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9aa911a-aebc-37b8-67b3-b7670424226b@huawei.com>
Date:   Thu, 11 Mar 2021 10:16:20 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Joel Becker <jlbec@...lplan.org>,
        Christoph Hellwig <hch@....de>
CC:     <linux-kernel@...r.kernel.org>, <chao@...nel.org>,
        Daiyue Zhang <zhangdaiyue1@...wei.com>,
        Yi Chen <chenyi77@...wei.com>, Ge Qiu <qiuge@...wei.com>,
        <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] configfs: Fix use-after-free issue in
 __configfs_open_file

Hi Joel, Christoph, Al

Does any one have time to review this patch, ten days past...

Thanks,

On 2021/3/5 11:29, Chao Yu wrote:
> +Cc fsdevel
> 
> Ping,
> 
> Any comments one this patch?
> 
> On 2021/3/1 14:10, Chao Yu wrote:
>> From: Daiyue Zhang <zhangdaiyue1@...wei.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@...iv.linux.org.uk>
>> Cc: Joel Becker <jlbec@...lplan.org>
>> Cc: Christoph Hellwig <hch@....de>
>> Signed-off-by: Daiyue Zhang <zhangdaiyue1@...wei.com>
>> Signed-off-by: Yi Chen <chenyi77@...wei.com>
>> Signed-off-by: Ge Qiu <qiuge@...wei.com>
>> Reviewed-by: Chao Yu <yuchao0@...wei.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);
>>
> .
>