[PATCH v2 1/2] Btrfs: add more valid checks for superblock

[PATCH v2 1/2] Btrfs: add more valid checks for superblock

[Liu Bo]

This adds valid checks for super_total_bytes, super_bytes_used and
super_stripesize, super_num_devices.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2:
 - Check super_num_devices and super_total_bytes after loading chunk
   tree.
 - Check super_bytes_used against the minimum space usage of a fresh
   mkfs.btrfs.
 - Fix super_stripesize to be sectorsize instead of 4096

 fs/btrfs/disk-io.c | 11 +++++++++++
 fs/btrfs/volumes.c | 24 ++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6628fca..ea78d77 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4130,6 +4130,17 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
 	 * Hint to catch really bogus numbers, bitflips or so, more exact checks are
 	 * done later
 	 */
+	if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) {
+		printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n",
+		       btrfs_super_bytes_used(sb));
+		ret = -EINVAL;
+	}
+	if (!is_power_of_2(btrfs_super_stripesize(sb)) ||
+	    btrfs_super_stripesize(sb) != sectorsize) {
+		printk(KERN_ERR "BTRFS: invalid stripesize %u\n",
+		       btrfs_super_stripesize(sb));
+		ret = -EINVAL;
+	}
 	if (btrfs_super_num_devices(sb) > (1UL << 31))
 		printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n",
 				btrfs_super_num_devices(sb));
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bdc6256..d403ab6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6648,6 +6648,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
 	struct btrfs_key found_key;
 	int ret;
 	int slot;
+	u64 total_dev = 0;
 
 	root = root->fs_info->chunk_root;
 
@@ -6689,6 +6690,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
 			ret = read_one_dev(root, leaf, dev_item);
 			if (ret)
 				goto error;
+			total_dev++;
 		} else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) {
 			struct btrfs_chunk *chunk;
 			chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
@@ -6698,6 +6700,28 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
 		}
 		path->slots[0]++;
 	}
+
+	/*
+	 * After loading chunk tree, we've got all device information,
+	 * do another round of validation check.
+	 */
+	if (total_dev != root->fs_info->fs_devices->total_devices) {
+		btrfs_err(root->fs_info,
+	   "super_num_devices(%llu) mismatch with num_devices(%llu) found here",
+			  btrfs_super_num_devices(root->fs_info->super_copy),
+			  total_dev);
+		ret = -EINVAL;
+		goto error;
+	}
+	if (btrfs_super_total_bytes(root->fs_info->super_copy) <
+	    root->fs_info->fs_devices->total_rw_bytes) {
+		btrfs_err(root->fs_info,
+	"super_total_bytes(%llu) mismatch with fs_devices total_rw_bytes(%llu)",
+			  btrfs_super_total_bytes(root->fs_info->super_copy),
+			  root->fs_info->fs_devices->total_rw_bytes);
+		ret = -EINVAL;
+		goto error;
+	}
 	ret = 0;
 error:
 	unlock_chunks(root);
-- 
2.5.5

[PATCH v2 2/2] Btrfs: add valid checks for chunk loading

[Liu Bo]

To prevent fuzz filesystem images from panic the whole system,
we need various validation checks to refuse to mount such an image
if btrfs finds any invalid value during loading chunks, including
both sys_array and regular chunks.

Note that these checks may not be sufficient to cover all corner cases,
feel free to add more checks.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: 
 - Fix several typos.

 fs/btrfs/volumes.c | 81 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d403ab6..7a169de 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6250,27 +6250,23 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 	return dev;
 }
 
-static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
-			  struct extent_buffer *leaf,
-			  struct btrfs_chunk *chunk)
+/* Return -EIO if any error, otherwise return 0. */
+static int btrfs_check_chunk_valid(struct btrfs_root *root,
+				   struct extent_buffer *leaf,
+				   struct btrfs_chunk *chunk, u64 logical)
 {
-	struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
-	struct map_lookup *map;
-	struct extent_map *em;
-	u64 logical;
 	u64 length;
 	u64 stripe_len;
-	u64 devid;
-	u8 uuid[BTRFS_UUID_SIZE];
-	int num_stripes;
-	int ret;
-	int i;
+	u16 num_stripes;
+	u16 sub_stripes;
+	u64 type;
 
-	logical = key->offset;
 	length = btrfs_chunk_length(leaf, chunk);
 	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
 	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
-	/* Validation check */
+	sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
+	type = btrfs_chunk_type(leaf, chunk);
+
 	if (!num_stripes) {
 		btrfs_err(root->fs_info, "invalid chunk num_stripes: %u",
 			  num_stripes);
@@ -6281,6 +6277,11 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 			  "invalid chunk logical %llu", logical);
 		return -EIO;
 	}
+	if (btrfs_chunk_sector_size(leaf, chunk) != root->sectorsize) {
+		btrfs_err(root->fs_info, "invalid chunk sectorsize %u",
+			  btrfs_chunk_sector_size(leaf, chunk));
+		return -EIO;
+	}
 	if (!length || !IS_ALIGNED(length, root->sectorsize)) {
 		btrfs_err(root->fs_info,
 			"invalid chunk length %llu", length);
@@ -6292,13 +6293,53 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 		return -EIO;
 	}
 	if (~(BTRFS_BLOCK_GROUP_TYPE_MASK | BTRFS_BLOCK_GROUP_PROFILE_MASK) &
-	    btrfs_chunk_type(leaf, chunk)) {
+	    type) {
 		btrfs_err(root->fs_info, "unrecognized chunk type: %llu",
 			  ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
 			    BTRFS_BLOCK_GROUP_PROFILE_MASK) &
 			  btrfs_chunk_type(leaf, chunk));
 		return -EIO;
 	}
+	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
+	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
+	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
+	     num_stripes != 1)) {
+		btrfs_err(root->fs_info, "invalid num_stripes:sub_stripes %u:%u for profile %llu",
+			  num_stripes, sub_stripes,
+			  type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
+			  struct extent_buffer *leaf,
+			  struct btrfs_chunk *chunk)
+{
+	struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
+	struct map_lookup *map;
+	struct extent_map *em;
+	u64 logical;
+	u64 length;
+	u64 stripe_len;
+	u64 devid;
+	u8 uuid[BTRFS_UUID_SIZE];
+	int num_stripes;
+	int ret;
+	int i;
+
+	logical = key->offset;
+	length = btrfs_chunk_length(leaf, chunk);
+	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
+	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
+
+	ret = btrfs_check_chunk_valid(root, leaf, chunk, logical);
+	if (ret)
+		return ret;
 
 	read_lock(&map_tree->map_tree.lock);
 	em = lookup_extent_mapping(&map_tree->map_tree, logical, 1);
@@ -6546,6 +6587,7 @@ int btrfs_read_sys_array(struct btrfs_root *root)
 	u32 array_size;
 	u32 len = 0;
 	u32 cur_offset;
+	u64 type;
 	struct btrfs_key key;
 
 	ASSERT(BTRFS_SUPER_INFO_SIZE <= root->nodesize);
@@ -6612,6 +6654,15 @@ int btrfs_read_sys_array(struct btrfs_root *root)
 				break;
 			}
 
+			type = btrfs_chunk_type(sb, chunk);
+			if ((type & BTRFS_BLOCK_GROUP_SYSTEM) == 0) {
+				printk(KERN_ERR
+	    "BTRFS: invalid chunk type %llu in sys_array at offset %u\n",
+					type, cur_offset);
+				ret = -EIO;
+				break;
+			}
+
 			len = btrfs_chunk_item_size(num_stripes);
 			if (cur_offset + len > array_size)
 				goto out_short_read;
-- 
2.5.5

Re: [PATCH v2 2/2] Btrfs: add valid checks for chunk loading

[David Sterba]

On Fri, Jun 03, 2016 at 12:05:15PM -0700, Liu Bo wrote:
> To prevent fuzz filesystem images from panic the whole system,
> we need various validation checks to refuse to mount such an image
> if btrfs finds any invalid value during loading chunks, including
> both sys_array and regular chunks.
> 
> Note that these checks may not be sufficient to cover all corner cases,
> feel free to add more checks.
> 
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

(switched the printk to btrfs_*)

Re: [PATCH v2 1/2] Btrfs: add more valid checks for superblock

[David Sterba]

On Fri, Jun 03, 2016 at 12:05:14PM -0700, Liu Bo wrote:
> This adds valid checks for super_total_bytes, super_bytes_used and
> super_stripesize, super_num_devices.
> 
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

I'll switch the printk(KERN_ to the btrfs_* helpers and do minor tweaks
in the messages, but otherwise this version looks good to me. I don't
think we need the helper for stripe value checks as I proposed.

Re: [PATCH v2 1/2] Btrfs: add more valid checks for superblock

[David Sterba]

On Fri, Jun 03, 2016 at 12:05:14PM -0700, Liu Bo wrote:
> @@ -6648,6 +6648,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>  	struct btrfs_key found_key;
>  	int ret;
>  	int slot;
> +	u64 total_dev = 0;
>  
>  	root = root->fs_info->chunk_root;
>  
> @@ -6689,6 +6690,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>  			ret = read_one_dev(root, leaf, dev_item);
>  			if (ret)
>  				goto error;
> +			total_dev++;
>  		} else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) {
>  			struct btrfs_chunk *chunk;
>  			chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
> @@ -6698,6 +6700,28 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>  		}
>  		path->slots[0]++;
>  	}
> +
> +	/*
> +	 * After loading chunk tree, we've got all device information,
> +	 * do another round of validation check.
> +	 */
> +	if (total_dev != root->fs_info->fs_devices->total_devices) {
> +		btrfs_err(root->fs_info,
> +	   "super_num_devices(%llu) mismatch with num_devices(%llu) found here",
> +			  btrfs_super_num_devices(root->fs_info->super_copy),
> +			  total_dev);
> +		ret = -EINVAL;

Turns out this check is too strict in combination with an intermediate
state and a bug in device deletion.

We've had several reports where a filesystem becomes unmountable due to
the above check, where the wrong value is just stored in the superblock
and is fixable by simply setting it to the right value. The right value
is number of dev items found in the dev tree, which is exactly the
total_dev that we read here.

The intermediate state can be caused by removing the device item in
btrfs_rm_device (see comment before btrfs_rm_dev_item call). This
apparently happens, when the device deletion is not finished, ie. the
superblock is not overwritten with a new copy.

So: do you agree to make it just a warning, and fixup the superblock
num_devices here? A userspace fix is also possible, but when this
happens and the filesystem is not mountable, a fix from outside of the
filesystem might be hard.

> +		goto error;
> +	}
> +	if (btrfs_super_total_bytes(root->fs_info->super_copy) <
> +	    root->fs_info->fs_devices->total_rw_bytes) {
> +		btrfs_err(root->fs_info,
> +	"super_total_bytes(%llu) mismatch with fs_devices total_rw_bytes(%llu)",
> +			  btrfs_super_total_bytes(root->fs_info->super_copy),
> +			  root->fs_info->fs_devices->total_rw_bytes);
> +		ret = -EINVAL;
> +		goto error;
> +	}
>  	ret = 0;
>  error:
>  	unlock_chunks(root);

Re: [PATCH v2 1/2] Btrfs: add more valid checks for superblock

[Liu Bo]

On Fri, Nov 25, 2016 at 05:50:19PM +0100, David Sterba wrote:
> On Fri, Jun 03, 2016 at 12:05:14PM -0700, Liu Bo wrote:
> > @@ -6648,6 +6648,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
> >  	struct btrfs_key found_key;
> >  	int ret;
> >  	int slot;
> > +	u64 total_dev = 0;
> >  
> >  	root = root->fs_info->chunk_root;
> >  
> > @@ -6689,6 +6690,7 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
> >  			ret = read_one_dev(root, leaf, dev_item);
> >  			if (ret)
> >  				goto error;
> > +			total_dev++;
> >  		} else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) {
> >  			struct btrfs_chunk *chunk;
> >  			chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
> > @@ -6698,6 +6700,28 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
> >  		}
> >  		path->slots[0]++;
> >  	}
> > +
> > +	/*
> > +	 * After loading chunk tree, we've got all device information,
> > +	 * do another round of validation check.
> > +	 */
> > +	if (total_dev != root->fs_info->fs_devices->total_devices) {
> > +		btrfs_err(root->fs_info,
> > +	   "super_num_devices(%llu) mismatch with num_devices(%llu) found here",
> > +			  btrfs_super_num_devices(root->fs_info->super_copy),
> > +			  total_dev);
> > +		ret = -EINVAL;
> 
> Turns out this check is too strict in combination with an intermediate
> state and a bug in device deletion.
> 
> We've had several reports where a filesystem becomes unmountable due to
> the above check, where the wrong value is just stored in the superblock
> and is fixable by simply setting it to the right value. The right value
> is number of dev items found in the dev tree, which is exactly the
> total_dev that we read here.
> 
> The intermediate state can be caused by removing the device item in
> btrfs_rm_device (see comment before btrfs_rm_dev_item call). This
> apparently happens, when the device deletion is not finished, ie. the
> superblock is not overwritten with a new copy.

I see.

Looks like currently it does commit_transaction in btrfs_rm_devi_item() while it
doesn't in btrfs_add_device, so it turns out that the logics in adding a device
and removing a device are a little bit different,

btrfs_rm_device                              btrfs_init_new_devices
  ...                                          ...
  ->btrfs_rm_dev_item                          ->btrfs_start_transaction()
    ->btrfs_start_transaction                  ->mutex_lock(&device_list_mutex)
    ->#rm dev item in chunk tree               ->list_add
    ->btrfs_commit_transaction                 ->btrfs_set_super_num_device
  ->mutex_lock(&device_list_mutex)             ->mutex_unlock(&device_list_mutex)
  ->list_del()                                 ->btrfs_add_device
  ->btrfs_set_super_num_device                 ->btrfs_commit_transaction()
  ->mutext_unlock(&device_list_mutex)


Not sure why we made it different around commit_transaction, but seems that we
can avoid the situation described above by changing how we play with transaction
in rm_device.

> 
> So: do you agree to make it just a warning, and fixup the superblock
> num_devices here? A userspace fix is also possible, but when this
> happens and the filesystem is not mountable, a fix from outside of the
> filesystem might be hard.

I'm OK with setting up a warning.

Thanks,

-liubo

> 
> > +		goto error;
> > +	}
> > +	if (btrfs_super_total_bytes(root->fs_info->super_copy) <
> > +	    root->fs_info->fs_devices->total_rw_bytes) {
> > +		btrfs_err(root->fs_info,
> > +	"super_total_bytes(%llu) mismatch with fs_devices total_rw_bytes(%llu)",
> > +			  btrfs_super_total_bytes(root->fs_info->super_copy),
> > +			  root->fs_info->fs_devices->total_rw_bytes);
> > +		ret = -EINVAL;
> > +		goto error;
> > +	}
> >  	ret = 0;
> >  error:
> >  	unlock_chunks(root);