From: Daiyue Zhang <[email protected]>
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 <[email protected]>
Cc: Joel Becker <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Daiyue Zhang <[email protected]>
Signed-off-by: Yi Chen <[email protected]>
Signed-off-by: Ge Qiu <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
---
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);
--
2.29.2
+Cc fsdevel
Ping,
Any comments one this patch?
On 2021/3/1 14:10, Chao Yu wrote:
> From: Daiyue Zhang <[email protected]>
>
> 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 <[email protected]>
> Cc: Joel Becker <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Daiyue Zhang <[email protected]>
> Signed-off-by: Yi Chen <[email protected]>
> Signed-off-by: Ge Qiu <[email protected]>
> Reviewed-by: Chao Yu <[email protected]>
> ---
> 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);
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Daiyue Zhang <[email protected]>
>> Signed-off-by: Yi Chen <[email protected]>
>> Signed-off-by: Ge Qiu <[email protected]>
>> Reviewed-by: Chao Yu <[email protected]>
>> ---
>> 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);
>>
> .
>
On Thu, Mar 11, 2021 at 10:16:20AM +0800, Chao Yu wrote:
> Hi Joel, Christoph, Al
>
> Does any one have time to review this patch, ten days past...
Acked-by: Al Viro <[email protected]>