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

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

[Johannes Thumshirn]

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.

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.

Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb1aa96e1233..507f6f17b3a9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -940,7 +940,7 @@ 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,
+				btrfs_warn_in_rcu(NULL,
 	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
 						  path, devid, found_transid,
 						  current->comm,
-- 
2.26.2

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

[Nikolay Borisov]

On 16.11.20 г. 9:52 ч., 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.
> 
> 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.
> 
> Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..507f6f17b3a9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -940,7 +940,7 @@ 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,
> +				btrfs_warn_in_rcu(NULL,
>  	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
>  						  path, devid, found_transid,
>  						  current->comm,
> 

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

[David Sterba]

On Mon, Nov 16, 2020 at 04:52:54PM +0900, 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.
> 
> 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.

This would be good to add some info about the evolution of the fix, also
a comment saying why we can't use fs_info.

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

[Anand Jain]

On 16/11/20 3:52 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.
> 
> 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.
> 
> Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>


Sorry for the delay due to my vacation.

  Johannes,

  This patch fixes the issue in a very gross way, as I mentioned.

  Instead, do we know more about what/how threads were racing,
  leading to the access of the freed fs_info?

Thanks, Anand


> ---
>   fs/btrfs/volumes.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..507f6f17b3a9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -940,7 +940,7 @@ 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,
> +				btrfs_warn_in_rcu(NULL,
>   	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
>   						  path, devid, found_transid,
>   						  current->comm,
> 

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

[Johannes Thumshirn]

On 17/11/2020 08:20, Anand Jain wrote:
>   This patch fixes the issue in a very gross way, as I mentioned.

I know but I've not found a better way.

>   Instead, do we know more about what/how threads were racing,
>   leading to the access of the freed fs_info?

If I read the reproducer code correctly it's just mounting a crafted 
image twice via different /dev/loop devices.

This image is rejected by the mount code, because it can't read the chunk
tree.

As far as I've debugged it down scan_one_device() is racing with 
deactivate_locked_super(), so fs_info->sb can already be freed, when 
device_list_add() calls btrfs_warn_in_rcu(device->fs_info,...) leading
to a use-after-free in btrfs_printk() accessing fs_info->sb_s_id.

It feels like we're missing a mutex_lock(&uuid_mutex) in btrfs_kill_super()
but this hasn't led me to anything.

I'm all ears for a pointer to the correct fix.

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

[Anand Jain]

On 17/11/20 4:19 pm, Johannes Thumshirn wrote:
> On 17/11/2020 08:20, Anand Jain wrote:
>>    This patch fixes the issue in a very gross way, as I mentioned.
> 
> I know but I've not found a better way.
> 

>>    Instead, do we know more about what/how threads were racing,
>>    leading to the access of the freed fs_info?
> 




> If I read the reproducer code correctly it's just mounting a crafted
> image twice via different /dev/loop devices.
> 
> This image is rejected by the mount code, because it can't read the chunk
> tree.
 >
> As far as I've debugged it down scan_one_device() is racing with
> deactivate_locked_super(), so fs_info->sb can already be freed, when
> device_list_add() calls btrfs_warn_in_rcu(device->fs_info,...) leading
> to a use-after-free in btrfs_printk() accessing fs_info->sb_s_id.
> 

  This explains the problem how it happened, IMO this should go into the
  change log.

> It feels like we're missing a mutex_lock(&uuid_mutex) in btrfs_kill_super()

  Yes.
  But uuid_mutex (or device_list_mutex) is too sever for a simple
  problem, and there are other constraints with device_list_mutex.

  Ok let us take out use of fs_info from the device_list_add().

  I am ok with either NO_FS_INFO approach or just NULL.

Thanks, Anand

> but this hasn't led me to anything.

> 
> I'm all ears for a pointer to the correct fix.
>