Message ID | 0e9eb675e0a199bf034f13c58fbe5678f4e94a3c.1605513154.git.johannes.thumshirn@wdc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] btrfs: don't access possibly stale fs_info data for printing duplicate device | expand |
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, >
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.
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, >
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.
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. >
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,
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(-)