[2/4] btrfs: stop using uninitiazlised fs_info in device_list_add()
diff mbox series

Message ID 20200114060920.4527-2-anand.jain@oracle.com
State New
Headers show
Series
  • [1/4] btrfs: add NO_FS_INFO to btrfs_printk
Related show

Commit Message

Anand Jain Jan. 14, 2020, 6:09 a.m. UTC
fs_info is born during mount, and operations before the mount such as
scanning and assembling of the device volume should happen without any
reference to fs_info.

However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
Or even might UAF as reported in [1].

So do the right thing, don't use fs_info instead use NO_FS_INFO in
btrfs_warn_in_rcu() and btrfs_info_in_rcu() for the btrfs_printk()
to take care of it properly.

Link:
 [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Qu Wenruo Jan. 14, 2020, 6:58 a.m. UTC | #1
On 2020/1/14 下午2:09, Anand Jain wrote:
> fs_info is born during mount, and operations before the mount such as
> scanning and assembling of the device volume should happen without any
> reference to fs_info.
> 
> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
> leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
> Or even might UAF as reported in [1].

With your previous patch, which already checked NULL pointer, I didn't
see the need for NO_FS_INFO.

Or do you believe this calling site is a special?
If so, I still didn't get the point of NO_FS_INFO, just extra lines
using __func__ or "during scan: xxxxx" looks enough to me.

Thanks,
Qu

> 
> So do the right thing, don't use fs_info instead use NO_FS_INFO in
> btrfs_warn_in_rcu() and btrfs_info_in_rcu() for the btrfs_printk()
> to take care of it properly.
> 
> Link:
>  [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
> Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6fd90270e2c7..0301c3d693d8 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -889,14 +889,14 @@ 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(NO_FS_INFO,
>  			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
>  					disk_super->fsid, devid,
>  					rcu_str_deref(device->name), path);
>  				return ERR_PTR(-EEXIST);
>  			}
>  			bdput(path_bdev);
> -			btrfs_info_in_rcu(device->fs_info,
> +			btrfs_info_in_rcu(NO_FS_INFO,
>  				"device fsid %pU devid %llu moved old:%s new:%s",
>  				disk_super->fsid, devid,
>  				rcu_str_deref(device->name), path);
>
Nikolay Borisov Jan. 14, 2020, 7:30 a.m. UTC | #2
On 14.01.20 г. 8:58 ч., Qu Wenruo wrote:
> 
> 
> On 2020/1/14 下午2:09, Anand Jain wrote:
>> fs_info is born during mount, and operations before the mount such as
>> scanning and assembling of the device volume should happen without any
>> reference to fs_info.
>>
>> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
>> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
>> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions which
>> leads to btrfs_printk() which shall print "unknown" instead of sb->s_id.
>> Or even might UAF as reported in [1].
> 
> With your previous patch, which already checked NULL pointer, I didn't
> see the need for NO_FS_INFO.
> 
> Or do you believe this calling site is a special?
> If so, I still didn't get the point of NO_FS_INFO, just extra lines
> using __func__ or "during scan: xxxxx" looks enough to me.

I agree with this assessment. What value does NO_FS_INFO bring in
comparison to plain NULL that it warrants a special case?

> 
> Thanks,
> Qu
>

Patch
diff mbox series

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6fd90270e2c7..0301c3d693d8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -889,14 +889,14 @@  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(NO_FS_INFO,
 			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
 					disk_super->fsid, devid,
 					rcu_str_deref(device->name), path);
 				return ERR_PTR(-EEXIST);
 			}
 			bdput(path_bdev);
-			btrfs_info_in_rcu(device->fs_info,
+			btrfs_info_in_rcu(NO_FS_INFO,
 				"device fsid %pU devid %llu moved old:%s new:%s",
 				disk_super->fsid, devid,
 				rcu_str_deref(device->name), path);