Message ID | 20211004125805.12662-1-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
Series | [focal/linux-azure,focal/linux-hwe-5.8] btrfs: fix NULL pointer dereference when deleting device by invalid id | expand |
This patch is already staged for focal:linux (from which focal:linux-azure is derived), so it really only need be applied to focal:linux-hwe-5.8. On 10/4/21 6:58 AM, Tim Gardner wrote: > From: Qu Wenruo <wqu@suse.com> > > BugLink: https://bugs.launchpad.net/bugs/1945987 > > [BUG] > It's easy to trigger NULL pointer dereference, just by removing a > non-existing device id: > > # mkfs.btrfs -f -m single -d single /dev/test/scratch1 \ > /dev/test/scratch2 > # mount /dev/test/scratch1 /mnt/btrfs > # btrfs device remove 3 /mnt/btrfs > > Then we have the following kernel NULL pointer dereference: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 9 PID: 649 Comm: btrfs Not tainted 5.14.0-rc3-custom+ #35 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > RIP: 0010:btrfs_rm_device+0x4de/0x6b0 [btrfs] > btrfs_ioctl+0x18bb/0x3190 [btrfs] > ? lock_is_held_type+0xa5/0x120 > ? find_held_lock.constprop.0+0x2b/0x80 > ? do_user_addr_fault+0x201/0x6a0 > ? lock_release+0xd2/0x2d0 > ? __x64_sys_ioctl+0x83/0xb0 > __x64_sys_ioctl+0x83/0xb0 > do_syscall_64+0x3b/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > [CAUSE] > Commit a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return > btrfs_device directly") moves the "missing" device path check into > btrfs_rm_device(). > > But btrfs_rm_device() itself can have case where it only receives > @devid, with NULL as @device_path. > > In that case, calling strcmp() on NULL will trigger the NULL pointer > dereference. > > Before that commit, we handle the "missing" case inside > btrfs_find_device_by_devspec(), which will not check @device_path at all > if @devid is provided, thus no way to trigger the bug. > > [FIX] > Before calling strcmp(), also make sure @device_path is not NULL. > > Fixes: a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly") > CC: stable@vger.kernel.org # 5.4+ > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com> > Reviewed-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: David Sterba <dsterba@suse.com> > Signed-off-by: David Sterba <dsterba@suse.com> > (cherry picked from commit e4571b8c5e9ffa1e85c0c671995bd4dcc5c75091) > Signed-off-by: Tim Gardner <tim.gardner@canonical.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 3e3529c600cb7..e882c790292f9 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2168,7 +2168,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > > if (IS_ERR(device)) { > if (PTR_ERR(device) == -ENOENT && > - strcmp(device_path, "missing") == 0) > + device_path && strcmp(device_path, "missing") == 0) > ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND; > else > ret = PTR_ERR(device); >
On 04.10.21 14:58, Tim Gardner wrote: > From: Qu Wenruo <wqu@suse.com> > > BugLink: https://bugs.launchpad.net/bugs/1945987 > > [BUG] > It's easy to trigger NULL pointer dereference, just by removing a > non-existing device id: > > # mkfs.btrfs -f -m single -d single /dev/test/scratch1 \ > /dev/test/scratch2 > # mount /dev/test/scratch1 /mnt/btrfs > # btrfs device remove 3 /mnt/btrfs > > Then we have the following kernel NULL pointer dereference: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 9 PID: 649 Comm: btrfs Not tainted 5.14.0-rc3-custom+ #35 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > RIP: 0010:btrfs_rm_device+0x4de/0x6b0 [btrfs] > btrfs_ioctl+0x18bb/0x3190 [btrfs] > ? lock_is_held_type+0xa5/0x120 > ? find_held_lock.constprop.0+0x2b/0x80 > ? do_user_addr_fault+0x201/0x6a0 > ? lock_release+0xd2/0x2d0 > ? __x64_sys_ioctl+0x83/0xb0 > __x64_sys_ioctl+0x83/0xb0 > do_syscall_64+0x3b/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > [CAUSE] > Commit a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return > btrfs_device directly") moves the "missing" device path check into > btrfs_rm_device(). > > But btrfs_rm_device() itself can have case where it only receives > @devid, with NULL as @device_path. > > In that case, calling strcmp() on NULL will trigger the NULL pointer > dereference. > > Before that commit, we handle the "missing" case inside > btrfs_find_device_by_devspec(), which will not check @device_path at all > if @devid is provided, thus no way to trigger the bug. > > [FIX] > Before calling strcmp(), also make sure @device_path is not NULL. > > Fixes: a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly") > CC: stable@vger.kernel.org # 5.4+ > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com> > Reviewed-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: David Sterba <dsterba@suse.com> > Signed-off-by: David Sterba <dsterba@suse.com> > (cherry picked from commit e4571b8c5e9ffa1e85c0c671995bd4dcc5c75091) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Not sure how much effort still should go into any 5.8 but since we have some cleaning up to be done soon and this looks simple enough lets get a second ack quick and I can squeeze it into the run I delayed for the revocation stuff. -Stefan > 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 3e3529c600cb7..e882c790292f9 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2168,7 +2168,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > > if (IS_ERR(device)) { > if (PTR_ERR(device) == -ENOENT && > - strcmp(device_path, "missing") == 0) > + device_path && strcmp(device_path, "missing") == 0) > ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND; > else > ret = PTR_ERR(device); >
On 04/10/2021 13:58, Tim Gardner wrote: > From: Qu Wenruo <wqu@suse.com> > > BugLink: https://bugs.launchpad.net/bugs/1945987 > > [BUG] > It's easy to trigger NULL pointer dereference, just by removing a > non-existing device id: > > # mkfs.btrfs -f -m single -d single /dev/test/scratch1 \ > /dev/test/scratch2 > # mount /dev/test/scratch1 /mnt/btrfs > # btrfs device remove 3 /mnt/btrfs > > Then we have the following kernel NULL pointer dereference: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 9 PID: 649 Comm: btrfs Not tainted 5.14.0-rc3-custom+ #35 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > RIP: 0010:btrfs_rm_device+0x4de/0x6b0 [btrfs] > btrfs_ioctl+0x18bb/0x3190 [btrfs] > ? lock_is_held_type+0xa5/0x120 > ? find_held_lock.constprop.0+0x2b/0x80 > ? do_user_addr_fault+0x201/0x6a0 > ? lock_release+0xd2/0x2d0 > ? __x64_sys_ioctl+0x83/0xb0 > __x64_sys_ioctl+0x83/0xb0 > do_syscall_64+0x3b/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > [CAUSE] > Commit a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return > btrfs_device directly") moves the "missing" device path check into > btrfs_rm_device(). > > But btrfs_rm_device() itself can have case where it only receives > @devid, with NULL as @device_path. > > In that case, calling strcmp() on NULL will trigger the NULL pointer > dereference. > > Before that commit, we handle the "missing" case inside > btrfs_find_device_by_devspec(), which will not check @device_path at all > if @devid is provided, thus no way to trigger the bug. > > [FIX] > Before calling strcmp(), also make sure @device_path is not NULL. > > Fixes: a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly") > CC: stable@vger.kernel.org # 5.4+ > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com> > Reviewed-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: David Sterba <dsterba@suse.com> > Signed-off-by: David Sterba <dsterba@suse.com> > (cherry picked from commit e4571b8c5e9ffa1e85c0c671995bd4dcc5c75091) > Signed-off-by: Tim Gardner <tim.gardner@canonical.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 3e3529c600cb7..e882c790292f9 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2168,7 +2168,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > > if (IS_ERR(device)) { > if (PTR_ERR(device) == -ENOENT && > - strcmp(device_path, "missing") == 0) > + device_path && strcmp(device_path, "missing") == 0) > ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND; > else > ret = PTR_ERR(device); > Looks good to me. Acked-by: Colin Ian King <colin.king@canonical.com>
On 04.10.21 14:58, Tim Gardner wrote: > From: Qu Wenruo <wqu@suse.com> > > BugLink: https://bugs.launchpad.net/bugs/1945987 > > [BUG] > It's easy to trigger NULL pointer dereference, just by removing a > non-existing device id: > > # mkfs.btrfs -f -m single -d single /dev/test/scratch1 \ > /dev/test/scratch2 > # mount /dev/test/scratch1 /mnt/btrfs > # btrfs device remove 3 /mnt/btrfs > > Then we have the following kernel NULL pointer dereference: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP NOPTI > CPU: 9 PID: 649 Comm: btrfs Not tainted 5.14.0-rc3-custom+ #35 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > RIP: 0010:btrfs_rm_device+0x4de/0x6b0 [btrfs] > btrfs_ioctl+0x18bb/0x3190 [btrfs] > ? lock_is_held_type+0xa5/0x120 > ? find_held_lock.constprop.0+0x2b/0x80 > ? do_user_addr_fault+0x201/0x6a0 > ? lock_release+0xd2/0x2d0 > ? __x64_sys_ioctl+0x83/0xb0 > __x64_sys_ioctl+0x83/0xb0 > do_syscall_64+0x3b/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > [CAUSE] > Commit a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return > btrfs_device directly") moves the "missing" device path check into > btrfs_rm_device(). > > But btrfs_rm_device() itself can have case where it only receives > @devid, with NULL as @device_path. > > In that case, calling strcmp() on NULL will trigger the NULL pointer > dereference. > > Before that commit, we handle the "missing" case inside > btrfs_find_device_by_devspec(), which will not check @device_path at all > if @devid is provided, thus no way to trigger the bug. > > [FIX] > Before calling strcmp(), also make sure @device_path is not NULL. > > Fixes: a27a94c2b0c7 ("btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly") > CC: stable@vger.kernel.org # 5.4+ > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com> > Reviewed-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: David Sterba <dsterba@suse.com> > Signed-off-by: David Sterba <dsterba@suse.com> > (cherry picked from commit e4571b8c5e9ffa1e85c0c671995bd4dcc5c75091) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- Applied to focal:linux-hwe-5.8/hwe-5.8. Thanks. -Stefan > 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 3e3529c600cb7..e882c790292f9 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2168,7 +2168,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, > > if (IS_ERR(device)) { > if (PTR_ERR(device) == -ENOENT && > - strcmp(device_path, "missing") == 0) > + device_path && strcmp(device_path, "missing") == 0) > ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND; > else > ret = PTR_ERR(device); >
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 3e3529c600cb7..e882c790292f9 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2168,7 +2168,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, if (IS_ERR(device)) { if (PTR_ERR(device) == -ENOENT && - strcmp(device_path, "missing") == 0) + device_path && strcmp(device_path, "missing") == 0) ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND; else ret = PTR_ERR(device);