Messages in this thread | | | Date | Tue, 23 Aug 2022 15:26:00 +0100 | From | Filipe Manana <> | Subject | Re: [PATCH -next] btrfs: fix use-after-free in btrfs_get_global_root |
| |
On Tue, Aug 23, 2022 at 07:05:37PM +0800, yebin wrote: > > > 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@huawei.com> > > > --- > > > 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.
Nop, it won't. I'm talking about what was *done in the transaction*, which at this point was already committed.
Nothing below under the labels will undo that...
> 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 > > > > > . > > >
| |