Re: [PATCH] btrfs: Fix UAF during concurrent mount and device scan

[David Sterba <dsterba@suse.cz>]

On Thu, Jan 09, 2020 at 01:02:10PM +0200, Nikolay Borisov wrote:
> This log shows how an fs  is being unmounted which causes device close
> routine to be invoked. It sets bdev to NULL but doesn't reset fs_info.
> Afterwards the fs_info itself is freed from btrfs_kill_super at the same
> time the device is still anchored at fs_devices list. Subsequently a
> mount is triggered which sets btrfs_fs_device::bdev to a valid value, yet
> btrfs_fs_device::fs_info is still stale/freed. Before btrfs_fill_super
> is called and re-initializes btrfs_fs_device::fs_info a concurrent device
> scan is triggered, it finds the device with its ->bdev pointer set to
> valid value and eventually calls btrfs_info_in_rcu in device_list_add
> which causes the crash.
> 
> Simply setting btrfs_fs_device::fs_info to NULL prevents the crash but
> doesn't fix the race. In fact the race cannot be solved because device
> scan is asynchronous in its nature so it makes no sense to try and
> synchronize it with pending mounts.

We've had bugs when mount and scan raced, I don't see why you think this
cannot be fixed and synchronized in this case. At minimum I'd think that
the device_list_mutex should be enough as it's the one thing that
excludes mount and scan for the new and removed members of the device
lists etc.

> Fixes: cc1824fcd334 ("btrfs: reset device back to allocation state when removing")

That's still it misc-next so I'd rather fold it into the patch than to
have this split fix.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> With this fix I can no longer get generic/085 to crash/generate the KASAN warning,
> even with  an mdelay added in btrfs_mount_root which triggered the issue reliably.
> 
>  fs/btrfs/volumes.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 65e78e59d5c4..ad8944cc4dd1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1086,6 +1086,7 @@ static void btrfs_close_one_device(struct btrfs_device *device)
> 
>  	atomic_set(&device->dev_stats_ccnt, 0);
>  	extent_io_tree_release(&device->alloc_state);
> +	device->fs_info = NULL;
> 
>  	/* Verify the device is back in a pristine state  */
>  	ASSERT(!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
> --
> 2.17.1