Re: [PATCH -next] btrfs: fix use-after-free in btrfs_get_global_root
From: yebin
Date: Tue Aug 23 2022 - 10:13:30 EST
On 2022/8/23 17:06, Filipe Manana wrote:
On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote:
Syzkaller reported UAF as follows:
==================================================================
BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
Workqueue: btrfs-qgroup-rescan btrfs_work_helper
Call Trace:
<TASK>
dump_stack_lvl+0x6e/0x91
print_report.cold+0xb2/0x6bb
kasan_report+0xa8/0x130
kasan_check_range+0x13f/0x1d0
btrfs_get_global_root+0x663/0xa10
btrfs_get_fs_root_commit_root+0xa5/0x150
find_parent_nodes+0x92f/0x2990
btrfs_find_all_roots_safe+0x12d/0x220
btrfs_find_all_roots+0xbb/0xd0
btrfs_qgroup_rescan_worker+0x600/0xc30
btrfs_work_helper+0xff/0x750
process_one_work+0x52c/0x930
worker_thread+0x352/0x8c0
kthread+0x1b9/0x200
ret_from_fork+0x22/0x30
</TASK>
Allocated by task 1895:
kasan_save_stack+0x1e/0x40
__kasan_kmalloc+0xa9/0xe0
btrfs_alloc_root+0x40/0x820
btrfs_create_tree+0xf8/0x500
btrfs_quota_enable+0x30a/0x1120
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Freed by task 1895:
kasan_save_stack+0x1e/0x40
kasan_set_track+0x21/0x30
kasan_set_free_info+0x20/0x40
__kasan_slab_free+0x127/0x1c0
kfree+0xa8/0x2d0
btrfs_put_root+0x1ca/0x230
btrfs_quota_enable+0x87c/0x1120
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
==================================================================
Above issue may happens as follows:
p1 p2
btrfs_quota_enable
spin_lock(&fs_info->qgroup_lock);
fs_info->quota_root = quota_root;
spin_unlock(&fs_info->qgroup_lock);
ret = qgroup_rescan_init -> return error
if (ret)
btrfs_put_root(quota_root);
kfree(root);
if (ret) {
ulist_free(fs_info->qgroup_ulist);
fs_info->qgroup_ulist = NULL;
btrfs_sysfs_del_qgroups(fs_info);
} btrfs_qgroup_rescan_worker
btrfs_find_all_roots
btrfs_find_all_roots_safe
find_parent_nodes
btrfs_get_fs_root_commit_root
btrfs_grab_root(fs_info->quota_root)
-> quota_root already freed
I have described the process of the issue here. I can write it in more
detail.
Syzkaller also reported another issue:
==================================================================
BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
Read of size 8 at addr ffff88811413d048 by task rep/2921
CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
rep[2921] cmdline: ./rep
Call Trace:
<TASK>
dump_stack_lvl+0x6e/0x91
print_report.cold+0xb2/0x6bb
kasan_report+0xa8/0x130
ulist_release+0x30/0xb3
ulist_reinit+0x16/0x56
btrfs_qgroup_free_refroot+0x288/0x3f0
btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
commit_fs_roots+0x28c/0x430
btrfs_commit_transaction+0x9a6/0x1b40
btrfs_qgroup_rescan+0x7e/0x130
btrfs_ioctl+0x48ed/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
</TASK>
Allocated by task 2900:
kasan_save_stack+0x1e/0x40
__kasan_kmalloc+0xa9/0xe0
ulist_alloc+0x5c/0xe0
btrfs_quota_enable+0x1b2/0x1160
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Freed by task 2900:
kasan_save_stack+0x1e/0x40
kasan_set_track+0x21/0x30
kasan_set_free_info+0x20/0x40
__kasan_slab_free+0x127/0x1c0
kfree+0xa8/0x2d0
ulist_free.cold+0x15/0x1a
btrfs_quota_enable+0x8bf/0x1160
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
==================================================================
To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
return success.
Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
---
fs/btrfs/qgroup.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index db723c0026bd..16f0b038295a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
if (ret)
goto out_free_path;
- /*
- * Set quota enabled flag after committing the transaction, to avoid
- * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
- * creation.
- */
- spin_lock(&fs_info->qgroup_lock);
- fs_info->quota_root = quota_root;
- set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
- spin_unlock(&fs_info->qgroup_lock);
-
ret = qgroup_rescan_init(fs_info, 0, 1);
if (!ret) {
+ /*
+ * Set quota enabled flag after committing the transaction, to
+ * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
+ * snapshot creation.
+ */
+ spin_lock(&fs_info->qgroup_lock);
+ fs_info->quota_root = quota_root;
+ set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+ spin_unlock(&fs_info->qgroup_lock);
+
But how can the race happen? The changelog should explain that.
To me this suggests that after we set BTRFS_FS_QUOTA_ENABLED and set the
quota root, but before we called qgroup_rescan_init() at btrfs_quota_enable(),
some other task started the rescan worker first - I can only think of
someone else calling the ioctl to start the rescan worker (btrfs_ioctl_quota_rescan()).
Yes, rescan worker is triggered by ioctl(btrfs_ioctl_quota_rescan()).
In that case we get "ret == -EINPROGRESS" at btrfs_quota_enable().
So please provide a detailed explanation in the log of how the race can
happen.
This solution is also buggy. Because in case of an error, we will leave the
quota tree created, the qgroup relation, etc. That is, we don't undo
what btrfs_create_tree(), add_qgroup_item(), add_qgroup_rb(), etc did
Which means a future btrfs_quota_enable() call would fail, and calling
btrfs_quota_disable() to undo all those things will not work either,
because fs_info->quota_root is NULL.
As 'ret' is non-zero will release resources which you mentioned.
btrfs_quota_enable
...
out_free_path:
btrfs_free_path(path);
out_free_root:
if (ret)
btrfs_put_root(quota_root);
out:
if (ret) {
ulist_free(fs_info->qgroup_ulist);
fs_info->qgroup_ulist = NULL;
btrfs_sysfs_del_qgroups(fs_info);
}
mutex_unlock(&fs_info->qgroup_ioctl_lock);
if (ret && trans)
btrfs_end_transaction(trans);
else if (trans)
ret = btrfs_end_transaction(trans);
ulist_free(ulist);
...
I would suggest ignoring the error of qgroup_rescan_init() if it's
-EINPROGRESS, and ASSERT if it's anything different from 0 or
-EINPROGRESS. Also add a comment mentioning we can get -EINPROGRESS
because someone may have called the qgroup rescan ioctl.
Thanks.
qgroup_rescan_zero_tracking(fs_info);
fs_info->qgroup_rescan_running = true;
btrfs_queue_work(fs_info->qgroup_rescan_workers,
--
2.31.1
.