[v3] btrfs: don't access possibly stale fs_info data for printing duplicate device

Commit Message

Johannes Thumshirn Nov. 18, 2020, 9:03 a.m. UTC

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.

The syzkaller reproducer for this use-after-free crafts a filesystem image
and loop mounts it twice in a loop. The mount will fail as the crafted
image has an invalid chunk tree. When this happens btrfs_mount_root() will
call deactivate_locked_super(), which then cleans up fs_info and
fs_info::sb. If a second thread now adds the same block-device to the
file-system, it will get detected as a duplicate device and
device_list_add() will reject the duplicate and print a warning. But as
the fs_info pointer passed in is non-NULL this will result in a
use-after-free.

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.

Link: https://lore.kernel.org/linux-btrfs/[e-mail]
Reported-by: [e-mail]
Reviewed-by: Nikolay Borisov <[e-mail]>
Signed-off-by: Johannes Thumshirn <[e-mail]>

---
Changes to v2:
- Add comment why we're passing NULL to btrfs_warn_in_rcu()
- Clarify commit message

Changes to v1:
- Use btrfs_warn_in_rcu(NULL,) instead of pr_warn()
---
 fs/btrfs/volumes.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Anand Jain Nov. 18, 2020, 9:28 a.m. UTC | #1

On 18/11/20 5:03 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.
> 
> The syzkaller reproducer for this use-after-free crafts a filesystem image
> and loop mounts it twice in a loop. The mount will fail as the crafted
> image has an invalid chunk tree. When this happens btrfs_mount_root() will
> call deactivate_locked_super(), which then cleans up fs_info and
> fs_info::sb. If a second thread now adds the same block-device to the
> file-system, it will get detected as a duplicate device and
> device_list_add() will reject the duplicate and print a warning. But as
> the fs_info pointer passed in is non-NULL this will result in a
> use-after-free.
> 
> 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.
> 
> Link: https://lore.kernel.org/linux-btrfs/[e-mail]
> Reported-by: [e-mail]
> Reviewed-by: Nikolay Borisov <[e-mail]>
> Signed-off-by: Johannes Thumshirn <[e-mail]>
> 
> ---
> Changes to v2:
> - Add comment why we're passing NULL to btrfs_warn_in_rcu()
> - Clarify commit message
> 
> Changes to v1:
> - Use btrfs_warn_in_rcu(NULL,) instead of pr_warn()
> ---
>   fs/btrfs/volumes.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..3f2af8106d5b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -940,7 +940,13 @@ 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,
> +				/*
> +				 * device->fs_info may not be reliable here, so
> +				 * pass in a NULL fs_info. This avoids a
> +				 * possible use-after-free when the fs_info and
> +				 * fs_info->sb are already torn down.
> +				 */
> +				btrfs_warn_in_rcu(NULL,
>   	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
>   						  path, devid, found_transid,
>   						  current->comm,
> 

Few lines below, there is btrfs_info_in_rcu() and, we won't have
reliable device->fs_info even there. But we have seen those info
quite a lot of times. And so far, it's been ok. So

  Reviewed-by: Anand Jain <[e-mail]>

David Sterba Nov. 20, 2020, 4:21 p.m. UTC | #2

On Wed, Nov 18, 2020 at 05:28:57PM +0800, Anand Jain wrote:
> On 18/11/20 5:03 pm, Johannes Thumshirn wrote:
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -940,7 +940,13 @@ 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,
> > +				/*
> > +				 * device->fs_info may not be reliable here, so
> > +				 * pass in a NULL fs_info. This avoids a
> > +				 * possible use-after-free when the fs_info and
> > +				 * fs_info->sb are already torn down.
> > +				 */
> > +				btrfs_warn_in_rcu(NULL,
> >   	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
> >   						  path, devid, found_transid,
> >   						  current->comm,
> > 
> 
> Few lines below, there is btrfs_info_in_rcu() and, we won't have
> reliable device->fs_info even there. But we have seen those info
> quite a lot of times. And so far, it's been ok. So

Right, this also looks like potentially unsafe use but given that the
2nd print is still under device_list_mutex it think it will be harder
to hit.

David Sterba Nov. 20, 2020, 4:36 p.m. UTC | #3

On Wed, Nov 18, 2020 at 06:03:26PM +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.
> 
> The syzkaller reproducer for this use-after-free crafts a filesystem image
> and loop mounts it twice in a loop. The mount will fail as the crafted
> image has an invalid chunk tree. When this happens btrfs_mount_root() will
> call deactivate_locked_super(), which then cleans up fs_info and
> fs_info::sb. If a second thread now adds the same block-device to the
> file-system, it will get detected as a duplicate device and
> device_list_add() will reject the duplicate and print a warning. But as
> the fs_info pointer passed in is non-NULL this will result in a
> use-after-free.
> 
> 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.
> 
> Link: https://lore.kernel.org/linux-btrfs/[e-mail]

I've added the stacktrace from the link.

> Reported-by: [e-mail]
> Reviewed-by: Nikolay Borisov <[e-mail]>
> Signed-off-by: Johannes Thumshirn <[e-mail]>

Added to misc-next, thanks.

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb1aa96e1233..3f2af8106d5b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -940,7 +940,13 @@  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,
+				/*
+				 * device->fs_info may not be reliable here, so
+				 * pass in a NULL fs_info. This avoids a
+				 * possible use-after-free when the fs_info and
+				 * fs_info->sb are already torn down.
+				 */
+				btrfs_warn_in_rcu(NULL,
 	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
 						  path, devid, found_transid,
 						  current->comm,