Re: [PATCH -next] btrfs: fix use-after-free in btrfs_get_global_root

[Filipe Manana <fdmanana@kernel.org>]

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
> 
> 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);

In addition to what I mentioned in the previous mail, this is also not
correct because the qgroup_rescan_init() call will always fail if
BTRFS_FS_QUOTA_ENABLED is not set (with an -EBUSY error).

Have you tested this at all? Every attempt to enable quotas will result
in a -EBUSY being returned from the ioctl.

All we need is:

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index db723c0026bd..67818b2f316f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1174,6 +1174,17 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
                fs_info->qgroup_rescan_running = true;
                btrfs_queue_work(fs_info->qgroup_rescan_workers,
                                 &fs_info->qgroup_rescan_work);
+       } else {
+               /*
+                * We have set both BTRFS_FS_QUOTA_ENABLED and
+                * BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with
+                * -EINPROGRESS because someone started the rescan worker with
+                * the ioctl before we attempted to initialize it.
+                * Ignore such error, and any other error would need to undo
+                * everything we did in the transaction we just committed.
+                */
+               ASSERT(ret == -EINPROGRESS);
+               ret = 0;
        }
 
 out_free_path:



>  	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);
> +
>  	        qgroup_rescan_zero_tracking(fs_info);
>  		fs_info->qgroup_rescan_running = true;
>  	        btrfs_queue_work(fs_info->qgroup_rescan_workers,
> -- 
> 2.31.1
>