| Message ID | 1538384164-3030-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: harden agaist duplicate fsid | expand |
On 2018-10-01 04:56, Anand Jain wrote: > Its not that impossible to imagine that a device OR a btrfs image is > been copied just by using the dd or the cp command. Which in case both > the copies of the btrfs will have the same fsid. If on the system with > automount enabled, the copied FS gets scanned. > > We have a known bug in btrfs, that we let the device path be changed > after the device has been mounted. So using this loop hole the new > copied device would appears as if its mounted immediately after its > been copied. > > For example: > > Initially.. /dev/mmcblk0p4 is mounted as / > > lsblk > NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT > mmcblk0 179:0 0 29.2G 0 disk > |-mmcblk0p4 179:4 0 4G 0 part / > |-mmcblk0p2 179:2 0 500M 0 part /boot > |-mmcblk0p3 179:3 0 256M 0 part [SWAP] > `-mmcblk0p1 179:1 0 256M 0 part /boot/efi > > btrfs fi show > Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba > Total devices 1 FS bytes used 1.40GiB > devid 1 size 4.00GiB used 3.00GiB path /dev/mmcblk0p4 > > Copy mmcblk0 to sda > dd if=/dev/mmcblk0 of=/dev/sda > > And immediately after the copy completes the change in the device > superblock is notified which the automount scans using > btrfs device scan and the new device sda becomes the mounted root > device. > > lsblk > NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT > sda 8:0 1 14.9G 0 disk > |-sda4 8:4 1 4G 0 part / > |-sda2 8:2 1 500M 0 part > |-sda3 8:3 1 256M 0 part > `-sda1 8:1 1 256M 0 part > mmcblk0 179:0 0 29.2G 0 disk > |-mmcblk0p4 179:4 0 4G 0 part > |-mmcblk0p2 179:2 0 500M 0 part /boot > |-mmcblk0p3 179:3 0 256M 0 part [SWAP] > `-mmcblk0p1 179:1 0 256M 0 part /boot/efi > > btrfs fi show / > Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba > Total devices 1 FS bytes used 1.40GiB > devid 1 size 4.00GiB used 3.00GiB path /dev/sda4 > > The bug is quite nasty that you can't either unmount /dev/sda4 or > /dev/mmcblk0p4. And the problem does not get solved until you take > sda out of the system on to another system to change its fsid > using the 'btrfstune -u' command. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > > Hi, > > There was previous attempt to fix this bug ref: > www.spinics.net/lists/linux-btrfs/msg37466.html > > which broke the Ubuntu subvol mount at boot. The reason > for that is, Ubuntu changes the device path in the boot > process, and the earlier fix checked for the device-path > instead of block_device as in here and so we failed the > subvol mount request and thus the bootup process. > > I have tested this with Oracle Linux with btrfs as boot device > with a subvol to be mounted at boot. And also have verified > with new test case btrfs/173. > > It will be good if someone run this through Ubuntu boot test case. > > fs/btrfs/volumes.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index f4405e430da6..62173a3abcc4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -850,6 +850,29 @@ static noinline struct btrfs_device *device_list_add(const char *path, > return ERR_PTR(-EEXIST); > } > > + /* > + * we are going to replace the device path, make sure its the > + * same device if the device mounted > + */ > + if (device->bdev) { > + struct block_device *path_bdev; > + > + path_bdev = lookup_bdev(path); > + if (IS_ERR(path_bdev)) { > + mutex_unlock(&fs_devices->device_list_mutex); > + return ERR_CAST(path_bdev); > + } > + > + if (device->bdev != path_bdev) { > + bdput(path_bdev); > + mutex_unlock(&fs_devices->device_list_mutex); > + return ERR_PTR(-EEXIST); It would be _really_ nice to have an informative error message printed here. Aside from the possibility of an admin accidentally making a block-level copy of the volume, this code triggering could represent an attempted attack against the system, so it's arguably something that should be reported as happening. Personally, I think a WARN_ON_ONCE for this would make sense, ideally per-volume if possible. > + } > + bdput(path_bdev); > + pr_info("BTRFS: device fsid:devid %pU:%llu old path:%s new path:%s\n", > + disk_super->fsid, devid, rcu_str_deref(device->name), path); > + } > + > name = rcu_string_strdup(path, GFP_NOFS); > if (!name) { > mutex_unlock(&fs_devices->device_list_mutex); >
On 10/01/2018 07:17 PM, Austin S. Hemmelgarn wrote: > On 2018-10-01 04:56, Anand Jain wrote: >> Its not that impossible to imagine that a device OR a btrfs image is >> been copied just by using the dd or the cp command. Which in case both >> the copies of the btrfs will have the same fsid. If on the system with >> automount enabled, the copied FS gets scanned. >> >> We have a known bug in btrfs, that we let the device path be changed >> after the device has been mounted. So using this loop hole the new >> copied device would appears as if its mounted immediately after its >> been copied. >> >> For example: >> >> Initially.. /dev/mmcblk0p4 is mounted as / >> >> lsblk >> NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT >> mmcblk0 179:0 0 29.2G 0 disk >> |-mmcblk0p4 179:4 0 4G 0 part / >> |-mmcblk0p2 179:2 0 500M 0 part /boot >> |-mmcblk0p3 179:3 0 256M 0 part [SWAP] >> `-mmcblk0p1 179:1 0 256M 0 part /boot/efi >> >> btrfs fi show >> Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba >> Total devices 1 FS bytes used 1.40GiB >> devid 1 size 4.00GiB used 3.00GiB path /dev/mmcblk0p4 >> >> Copy mmcblk0 to sda >> dd if=/dev/mmcblk0 of=/dev/sda >> >> And immediately after the copy completes the change in the device >> superblock is notified which the automount scans using >> btrfs device scan and the new device sda becomes the mounted root >> device. >> >> lsblk >> NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT >> sda 8:0 1 14.9G 0 disk >> |-sda4 8:4 1 4G 0 part / >> |-sda2 8:2 1 500M 0 part >> |-sda3 8:3 1 256M 0 part >> `-sda1 8:1 1 256M 0 part >> mmcblk0 179:0 0 29.2G 0 disk >> |-mmcblk0p4 179:4 0 4G 0 part >> |-mmcblk0p2 179:2 0 500M 0 part /boot >> |-mmcblk0p3 179:3 0 256M 0 part [SWAP] >> `-mmcblk0p1 179:1 0 256M 0 part /boot/efi >> >> btrfs fi show / >> Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba >> Total devices 1 FS bytes used 1.40GiB >> devid 1 size 4.00GiB used 3.00GiB path /dev/sda4 >> >> The bug is quite nasty that you can't either unmount /dev/sda4 or >> /dev/mmcblk0p4. And the problem does not get solved until you take >> sda out of the system on to another system to change its fsid >> using the 'btrfstune -u' command. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> >> Hi, >> >> There was previous attempt to fix this bug ref: >> www.spinics.net/lists/linux-btrfs/msg37466.html >> >> which broke the Ubuntu subvol mount at boot. The reason >> for that is, Ubuntu changes the device path in the boot >> process, and the earlier fix checked for the device-path >> instead of block_device as in here and so we failed the >> subvol mount request and thus the bootup process. >> >> I have tested this with Oracle Linux with btrfs as boot device >> with a subvol to be mounted at boot. And also have verified >> with new test case btrfs/173. >> >> It will be good if someone run this through Ubuntu boot test case. >> >> fs/btrfs/volumes.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index f4405e430da6..62173a3abcc4 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -850,6 +850,29 @@ static noinline struct btrfs_device >> *device_list_add(const char *path, >> return ERR_PTR(-EEXIST); >> } >> + /* >> + * we are going to replace the device path, make sure its the >> + * same device if the device mounted >> + */ >> + if (device->bdev) { >> + struct block_device *path_bdev; >> + >> + path_bdev = lookup_bdev(path); >> + if (IS_ERR(path_bdev)) { >> + mutex_unlock(&fs_devices->device_list_mutex); >> + return ERR_CAST(path_bdev); >> + } >> + >> + if (device->bdev != path_bdev) { >> + bdput(path_bdev); >> + mutex_unlock(&fs_devices->device_list_mutex); >> + return ERR_PTR(-EEXIST); > It would be _really_ nice to have an informative error message printed > here. Aside from the possibility of an admin accidentally making a > block-level copy of the volume, > this code triggering could represent an > attempted attack against the system, so it's arguably something that > should be reported as happening. > Personally, I think a WARN_ON_ONCE for > this would make sense, ideally per-volume if possible. Ah. Will add an warn. Thanks, Anand >> + } >> + bdput(path_bdev); >> + pr_info("BTRFS: device fsid:devid %pU:%llu old path:%s >> new path:%s\n", >> + disk_super->fsid, devid, rcu_str_deref(device->name), >> path); >> + } >> + >> name = rcu_string_strdup(path, GFP_NOFS); >> if (!name) { >> mutex_unlock(&fs_devices->device_list_mutex); >> >
On Mon, Oct 01, 2018 at 09:31:04PM +0800, Anand Jain wrote: > >> + /* > >> + * we are going to replace the device path, make sure its the > >> + * same device if the device mounted > >> + */ > >> + if (device->bdev) { > >> + struct block_device *path_bdev; > >> + > >> + path_bdev = lookup_bdev(path); > >> + if (IS_ERR(path_bdev)) { > >> + mutex_unlock(&fs_devices->device_list_mutex); > >> + return ERR_CAST(path_bdev); > >> + } > >> + > >> + if (device->bdev != path_bdev) { > >> + bdput(path_bdev); > >> + mutex_unlock(&fs_devices->device_list_mutex); > >> + return ERR_PTR(-EEXIST); > > It would be _really_ nice to have an informative error message printed > > here. Aside from the possibility of an admin accidentally making a > > block-level copy of the volume, > > > this code triggering could represent an > > attempted attack against the system, so it's arguably something that > > should be reported as happening. > > > Personally, I think a WARN_ON_ONCE for > > this would make sense, ideally per-volume if possible. > > Ah. Will add an warn. Thanks, Anand The requested error message is not in the patch you posted or I have missed that (https://patchwork.kernel.org/patch/10641041/) . Austin, is the following ok for you? "BTRFS: duplicate device fsid:devid for %pU:%llu old:%s new:%s\n" BTRFS: duplicate device fsid:devid 7c667b96-59eb-43ad-9ae9-c878f6ad51d8:2 old:/dev/sda6 new:/dev/sdb6 As the UUID and paths are long I tried to squeeeze the rest so it's still comprehensible but this would be better confirmed. Thanks.
On 11/13/2018 10:31 AM, David Sterba wrote: > On Mon, Oct 01, 2018 at 09:31:04PM +0800, Anand Jain wrote: >>>> + /* >>>> + * we are going to replace the device path, make sure its the >>>> + * same device if the device mounted >>>> + */ >>>> + if (device->bdev) { >>>> + struct block_device *path_bdev; >>>> + >>>> + path_bdev = lookup_bdev(path); >>>> + if (IS_ERR(path_bdev)) { >>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>> + return ERR_CAST(path_bdev); >>>> + } >>>> + >>>> + if (device->bdev != path_bdev) { >>>> + bdput(path_bdev); >>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>> + return ERR_PTR(-EEXIST); >>> It would be _really_ nice to have an informative error message printed >>> here. Aside from the possibility of an admin accidentally making a >>> block-level copy of the volume, >> >>> this code triggering could represent an >>> attempted attack against the system, so it's arguably something that >>> should be reported as happening. >> >>> Personally, I think a WARN_ON_ONCE for >>> this would make sense, ideally per-volume if possible. >> >> Ah. Will add an warn. Thanks, Anand > > The requested error message is not in the patch you posted or I have > missed that (https://patchwork.kernel.org/patch/10641041/) . > > Austin, is the following ok for you? > > "BTRFS: duplicate device fsid:devid for %pU:%llu old:%s new:%s\n" > > BTRFS: duplicate device fsid:devid 7c667b96-59eb-43ad-9ae9-c878f6ad51d8:2 old:/dev/sda6 new:/dev/sdb6 > > As the UUID and paths are long I tried to squeeeze the rest so it's > still comprehensible but this would be better confirmed. Thanks. > Looks perfectly fine to me.
On 11/13/2018 11:31 PM, David Sterba wrote: > On Mon, Oct 01, 2018 at 09:31:04PM +0800, Anand Jain wrote: >>>> + /* >>>> + * we are going to replace the device path, make sure its the >>>> + * same device if the device mounted >>>> + */ >>>> + if (device->bdev) { >>>> + struct block_device *path_bdev; >>>> + >>>> + path_bdev = lookup_bdev(path); >>>> + if (IS_ERR(path_bdev)) { >>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>> + return ERR_CAST(path_bdev); >>>> + } >>>> + >>>> + if (device->bdev != path_bdev) { >>>> + bdput(path_bdev); >>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>> + return ERR_PTR(-EEXIST); >>> It would be _really_ nice to have an informative error message printed >>> here. Aside from the possibility of an admin accidentally making a >>> block-level copy of the volume, >> >>> this code triggering could represent an >>> attempted attack against the system, so it's arguably something that >>> should be reported as happening. >> >>> Personally, I think a WARN_ON_ONCE for >>> this would make sense, ideally per-volume if possible. >> >> Ah. Will add an warn. Thanks, Anand > > The requested error message is not in the patch you posted or I have > missed that (https://patchwork.kernel.org/patch/10641041/) . No you didn't miss. I missed it. When you integrated this into for-next, I should have sent out v2. Sorry. Thanks for taking care of it. > Austin, is the following ok for you? > > "BTRFS: duplicate device fsid:devid for %pU:%llu old:%s new:%s\n" > > BTRFS: duplicate device fsid:devid 7c667b96-59eb-43ad-9ae9-c878f6ad51d8:2 old:/dev/sda6 new:/dev/sdb6 > > As the UUID and paths are long I tried to squeeeze the rest so it's > still comprehensible but this would be better confirmed. Thanks. Thanks, Anand
On 11/13/2018 11:47 PM, Anand Jain wrote: > > > On 11/13/2018 11:31 PM, David Sterba wrote: >> On Mon, Oct 01, 2018 at 09:31:04PM +0800, Anand Jain wrote: >>>>> + /* >>>>> + * we are going to replace the device path, make sure its the >>>>> + * same device if the device mounted >>>>> + */ >>>>> + if (device->bdev) { >>>>> + struct block_device *path_bdev; >>>>> + >>>>> + path_bdev = lookup_bdev(path); >>>>> + if (IS_ERR(path_bdev)) { >>>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>>> + return ERR_CAST(path_bdev); >>>>> + } >>>>> + >>>>> + if (device->bdev != path_bdev) { >>>>> + bdput(path_bdev); >>>>> + mutex_unlock(&fs_devices->device_list_mutex); >>>>> + return ERR_PTR(-EEXIST); I have installed ubuntu with btrfs as root and with a subvol to be mounted at boot. Its working fine. Thanks, Anand
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index f4405e430da6..62173a3abcc4 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -850,6 +850,29 @@ static noinline struct btrfs_device *device_list_add(const char *path, return ERR_PTR(-EEXIST); } + /* + * we are going to replace the device path, make sure its the + * same device if the device mounted + */ + if (device->bdev) { + struct block_device *path_bdev; + + path_bdev = lookup_bdev(path); + if (IS_ERR(path_bdev)) { + mutex_unlock(&fs_devices->device_list_mutex); + return ERR_CAST(path_bdev); + } + + if (device->bdev != path_bdev) { + bdput(path_bdev); + mutex_unlock(&fs_devices->device_list_mutex); + return ERR_PTR(-EEXIST); + } + bdput(path_bdev); + pr_info("BTRFS: device fsid:devid %pU:%llu old path:%s new path:%s\n", + disk_super->fsid, devid, rcu_str_deref(device->name), path); + } + name = rcu_string_strdup(path, GFP_NOFS); if (!name) { mutex_unlock(&fs_devices->device_list_mutex);
Its not that impossible to imagine that a device OR a btrfs image is been copied just by using the dd or the cp command. Which in case both the copies of the btrfs will have the same fsid. If on the system with automount enabled, the copied FS gets scanned. We have a known bug in btrfs, that we let the device path be changed after the device has been mounted. So using this loop hole the new copied device would appears as if its mounted immediately after its been copied. For example: Initially.. /dev/mmcblk0p4 is mounted as / lsblk NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT mmcblk0 179:0 0 29.2G 0 disk |-mmcblk0p4 179:4 0 4G 0 part / |-mmcblk0p2 179:2 0 500M 0 part /boot |-mmcblk0p3 179:3 0 256M 0 part [SWAP] `-mmcblk0p1 179:1 0 256M 0 part /boot/efi btrfs fi show Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba Total devices 1 FS bytes used 1.40GiB devid 1 size 4.00GiB used 3.00GiB path /dev/mmcblk0p4 Copy mmcblk0 to sda dd if=/dev/mmcblk0 of=/dev/sda And immediately after the copy completes the change in the device superblock is notified which the automount scans using btrfs device scan and the new device sda becomes the mounted root device. lsblk NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT sda 8:0 1 14.9G 0 disk |-sda4 8:4 1 4G 0 part / |-sda2 8:2 1 500M 0 part |-sda3 8:3 1 256M 0 part `-sda1 8:1 1 256M 0 part mmcblk0 179:0 0 29.2G 0 disk |-mmcblk0p4 179:4 0 4G 0 part |-mmcblk0p2 179:2 0 500M 0 part /boot |-mmcblk0p3 179:3 0 256M 0 part [SWAP] `-mmcblk0p1 179:1 0 256M 0 part /boot/efi btrfs fi show / Label: none uuid: 07892354-ddaa-4443-90ea-f76a06accaba Total devices 1 FS bytes used 1.40GiB devid 1 size 4.00GiB used 3.00GiB path /dev/sda4 The bug is quite nasty that you can't either unmount /dev/sda4 or /dev/mmcblk0p4. And the problem does not get solved until you take sda out of the system on to another system to change its fsid using the 'btrfstune -u' command. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- Hi, There was previous attempt to fix this bug ref: www.spinics.net/lists/linux-btrfs/msg37466.html which broke the Ubuntu subvol mount at boot. The reason for that is, Ubuntu changes the device path in the boot process, and the earlier fix checked for the device-path instead of block_device as in here and so we failed the subvol mount request and thus the bootup process. I have tested this with Oracle Linux with btrfs as boot device with a subvol to be mounted at boot. And also have verified with new test case btrfs/173. It will be good if someone run this through Ubuntu boot test case. fs/btrfs/volumes.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)