| Message ID | 20200114060920.4527-2-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] btrfs: add NO_FS_INFO to btrfs_printk | expand |
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); >
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 >
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);
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(-)