| 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
| ||
|
Date: Tue, 23 Aug 2022 19:05:37 +0800
From: yebin <yebin10@...wei.com>
To: Filipe Manana <fdmanana@...nel.org>
CC: <clm@...com>, <josef@...icpanda.com>, <dsterba@...e.com>,
<linux-btrfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next] btrfs: fix use-after-free in btrfs_get_global_root
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@...wei.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.
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
>>
> .
>
Powered by blists - more mailing lists