Re: [PATCH v3] btrfs: don't access possibly stale fs_info data for printing duplicate device

[Anand Jain <anand.jain@oracle.com>]

On 18/11/20 5:03 pm, Johannes Thumshirn wrote:
> Syzbot reported a possible use-after-free when printing a duplicate device
> warning device_list_add().
> 
> At this point it can happen that a btrfs_device::fs_info is not correctly
> setup yet, so we're accessing stale data, when printing the warning
> message using the btrfs_printk() wrappers.
> 
> The syzkaller reproducer for this use-after-free crafts a filesystem image
> and loop mounts it twice in a loop. The mount will fail as the crafted
> image has an invalid chunk tree. When this happens btrfs_mount_root() will
> call deactivate_locked_super(), which then cleans up fs_info and
> fs_info::sb. If a second thread now adds the same block-device to the
> file-system, it will get detected as a duplicate device and
> device_list_add() will reject the duplicate and print a warning. But as
> the fs_info pointer passed in is non-NULL this will result in a
> use-after-free.
> 
> Instead of printing possibly uninitialized or already freed memory in
> btrfs_printk(), explicitly pass in a NULL fs_info so the printing of the
> device name will be skipped altogether.
> 
> Link: https://lore.kernel.org/linux-btrfs/000000000000c9e14b05afcc41ba@google.com
> Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> ---
> Changes to v2:
> - Add comment why we're passing NULL to btrfs_warn_in_rcu()
> - Clarify commit message
> 
> Changes to v1:
> - Use btrfs_warn_in_rcu(NULL,) instead of pr_warn()
> ---
>   fs/btrfs/volumes.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..3f2af8106d5b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -940,7 +940,13 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   			if (device->bdev != path_bdev) {
>   				bdput(path_bdev);
>   				mutex_unlock(&fs_devices->device_list_mutex);
> -				btrfs_warn_in_rcu(device->fs_info,
> +				/*
> +				 * device->fs_info may not be reliable here, so
> +				 * pass in a NULL fs_info. This avoids a
> +				 * possible use-after-free when the fs_info and
> +				 * fs_info->sb are already torn down.
> +				 */
> +				btrfs_warn_in_rcu(NULL,
>   	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
>   						  path, devid, found_transid,
>   						  current->comm,
> 

Few lines below, there is btrfs_info_in_rcu() and, we won't have
reliable device->fs_info even there. But we have seen those info
quite a lot of times. And so far, it's been ok. So

  Reviewed-by: Anand Jain <anand.jain@oracle.com>