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

[PATCH 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.

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>
---
 fs/btrfs/disk-io.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4e47849..988d03f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
+		printk(KERN_ERR "BTRFS: total bytes is zero\n");
+		ret = -EINVAL;
+	}
+	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 (btrfs_super_stripesize(sb) != 4096) {
+		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));
-- 
2.5.5

[PATCH 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>
---
 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bd0f45f..1075573 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6206,27 +6206,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);
@@ -6237,24 +6233,70 @@ 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 %llu",
+			  (unsigned long long)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);
 		return -EIO;
 	}
-	if (!is_power_of_2(stripe_len)) {
+	if (stripe_len != BTRFS_STRIPE_LEN) {
 		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
 			  stripe_len);
 		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 == 0) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID5 && 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);
+	/* Validation check */
+	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);
@@ -6502,6 +6544,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);
@@ -6568,6 +6611,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 1/2] Btrfs: add more valid checks for superblock

[Liu Bo]

Oh Dave, I got to use a wrong email address of yours and got some failures.

Thanks,

-liubo

On Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote:
> This adds valid checks for super_total_bytes, super_bytes_used and
> super_stripesize.
> 
> 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>
> ---
>  fs/btrfs/disk-io.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 4e47849..988d03f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> +		ret = -EINVAL;
> +	}
> +	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 (btrfs_super_stripesize(sb) != 4096) {
> +		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));
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Qu Wenruo]

Liu Bo wrote on 2016/05/02 11:15 -0700:
> This adds valid checks for super_total_bytes, super_bytes_used and
> super_stripesize.
>
> 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>
> ---
>  fs/btrfs/disk-io.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 4e47849..988d03f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> +		ret = -EINVAL;
> +	}

Would it be better if using "6 * nodesize"?

I'd like to use a precious low limit on total bytes, but we don't have 
such value, so 6 nodesize would be good.

Thanks,
Qu

> +	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 (btrfs_super_stripesize(sb) != 4096) {
> +		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));
>

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

[Qu Wenruo]

Liu Bo wrote on 2016/05/02 11:15 -0700:
> 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.

Looks good for me.

But would you mind to do extra check on some minor members like owner, 
io_align, io_width and sub_stripes?
Since we have a dedicated function now, if not too hard, it's never a 
bad idea to check every member for best robust.

Especially sub_stripes, as it seems to be used by division in 
btrfs_rmap_block().

Thanks,
Qu
>
> 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>
> ---
>  fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 68 insertions(+), 16 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bd0f45f..1075573 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6206,27 +6206,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);
> @@ -6237,24 +6233,70 @@ 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 %llu",
> +			  (unsigned long long)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);
>  		return -EIO;
>  	}
> -	if (!is_power_of_2(stripe_len)) {
> +	if (stripe_len != BTRFS_STRIPE_LEN) {
>  		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
>  			  stripe_len);
>  		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 == 0) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && 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);
> +	/* Validation check */
> +	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);
> @@ -6502,6 +6544,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);
> @@ -6568,6 +6611,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;
>

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

[Anand Jain]

On 05/03/2016 02:15 AM, 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>
> ---
>   fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 68 insertions(+), 16 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bd0f45f..1075573 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6206,27 +6206,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);
> @@ -6237,24 +6233,70 @@ 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 %llu",
> +			  (unsigned long long)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);
>   		return -EIO;
>   	}
> -	if (!is_power_of_2(stripe_len)) {
> +	if (stripe_len != BTRFS_STRIPE_LEN) {
>   		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
>   			  stripe_len);
>   		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 == 0) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||


> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||

  It should be BTRFS_BLOCK_GROUP_RAID6

Thanks, Anand





> +	    (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);
> +	/* Validation check */
> +	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);
> @@ -6502,6 +6544,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);
> @@ -6568,6 +6611,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;
>

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

[Liu Bo]

On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote:
> 
> 
> Liu Bo wrote on 2016/05/02 11:15 -0700:
> >This adds valid checks for super_total_bytes, super_bytes_used and
> >super_stripesize.
> >
> >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>
> >---
> > fs/btrfs/disk-io.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> >diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >index 4e47849..988d03f 100644
> >--- a/fs/btrfs/disk-io.c
> >+++ b/fs/btrfs/disk-io.c
> >@@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> >+		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> >+		ret = -EINVAL;
> >+	}
> 
> Would it be better if using "6 * nodesize"?
> 
> I'd like to use a precious low limit on total bytes, but we don't have such
> value, so 6 nodesize would be good.

That's good, besides that I'm going to do another check between
btrfs_super_total_bytes(sb) and sb->dev_item.total_bytes.

Thanks,

-liubo

> 
> Thanks,
> Qu
> 
> >+	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 (btrfs_super_stripesize(sb) != 4096) {
> >+		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));
> >
> 
> 

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

[Liu Bo]

On Tue, May 03, 2016 at 01:53:02PM +0800, Anand Jain wrote:
> 
> 
> 
> On 05/03/2016 02:15 AM, 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>
> >---
> >  fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 68 insertions(+), 16 deletions(-)
> >
> >diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >index bd0f45f..1075573 100644
> >--- a/fs/btrfs/volumes.c
> >+++ b/fs/btrfs/volumes.c
> >@@ -6206,27 +6206,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);
> >@@ -6237,24 +6233,70 @@ 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 %llu",
> >+			  (unsigned long long)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);
> >  		return -EIO;
> >  	}
> >-	if (!is_power_of_2(stripe_len)) {
> >+	if (stripe_len != BTRFS_STRIPE_LEN) {
> >  		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
> >  			  stripe_len);
> >  		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 == 0) ||
> >+	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> >+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> 
> 
> >+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
> 
>  It should be BTRFS_BLOCK_GROUP_RAID6

NICE catching!

Thanks,

-liubo

> 
> Thanks, Anand
> 
> 
> 
> 
> 
> >+	    (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);
> >+	/* Validation check */
> >+	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);
> >@@ -6502,6 +6544,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);
> >@@ -6568,6 +6611,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;
> >

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

[Liu Bo]

On Tue, May 03, 2016 at 09:12:01AM +0800, Qu Wenruo wrote:
> 
> 
> Liu Bo wrote on 2016/05/02 11:15 -0700:
> >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.
> 
> Looks good for me.
> 
> But would you mind to do extra check on some minor members like owner,
> io_align, io_width and sub_stripes?
> Since we have a dedicated function now, if not too hard, it's never a bad
> idea to check every member for best robust.

OK.

> 
> Especially sub_stripes, as it seems to be used by division in
> btrfs_rmap_block().

It makes sense to check sub_stripes, but io_align/width are not used at
this moment, I'd leave it for the future.

What do you think?

Thanks,

-liubo

> 
> Thanks,
> Qu
> >
> >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>
> >---
> > fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 68 insertions(+), 16 deletions(-)
> >
> >diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >index bd0f45f..1075573 100644
> >--- a/fs/btrfs/volumes.c
> >+++ b/fs/btrfs/volumes.c
> >@@ -6206,27 +6206,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);
> >@@ -6237,24 +6233,70 @@ 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 %llu",
> >+			  (unsigned long long)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);
> > 		return -EIO;
> > 	}
> >-	if (!is_power_of_2(stripe_len)) {
> >+	if (stripe_len != BTRFS_STRIPE_LEN) {
> > 		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
> > 			  stripe_len);
> > 		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 == 0) ||
> >+	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> >+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> >+	    (type & BTRFS_BLOCK_GROUP_RAID5 && 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);
> >+	/* Validation check */
> >+	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);
> >@@ -6502,6 +6544,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);
> >@@ -6568,6 +6611,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;
> >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Sterba]

On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote:
> 
> 
> Liu Bo wrote on 2016/05/02 11:15 -0700:
> > This adds valid checks for super_total_bytes, super_bytes_used and
> > super_stripesize.
> >
> > 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>
> > ---
> >  fs/btrfs/disk-io.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 4e47849..988d03f 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> > +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> > +		ret = -EINVAL;
> > +	}
> 
> Would it be better if using "6 * nodesize"?
> 
> I'd like to use a precious low limit on total bytes, but we don't have 
> such value, so 6 nodesize would be good.

An early check can compare against some reasonable value, but the
total_bytes value must be equal to the sum of all device sizes
(disk_total_bytes). I'm not sure if we have enough information to verify
that at this point though.

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

[David Sterba]

On Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote:
> This adds valid checks for super_total_bytes, super_bytes_used and
> super_stripesize.
> 
> 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>
> ---
>  fs/btrfs/disk-io.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 4e47849..988d03f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> +		ret = -EINVAL;
> +	}
> +	if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) {

Similar to total_bytes (sum of device->total_bytes), bytes_used is sum
of of all device->used_bytes, which in turn is sum of all block group
sizes on the device.

> +		printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n",
> +		       btrfs_super_bytes_used(sb));
> +		ret = -EINVAL;
> +	}
> +	if (btrfs_super_stripesize(sb) != 4096) {

This is too strict. The stripesize is unused, but we not force it to be
4k, a multiple of nodesize/sectorsize should be enough.

> +		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));
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Sterba]

A few minor comments below

On Mon, May 02, 2016 at 11:15:51AM -0700, Liu Bo wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6206,27 +6206,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);
> @@ -6237,24 +6233,70 @@ 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 %llu",
> +			  (unsigned long long)btrfs_chunk_sector_size(leaf,

type cast not necessry

> +								      chunk));
> +		return -EIO;
> +	}
>  	if (!length || !IS_ALIGNED(length, root->sectorsize)) {
>  		btrfs_err(root->fs_info,
>  			"invalid chunk length %llu", length);
>  		return -EIO;
>  	}
> -	if (!is_power_of_2(stripe_len)) {
> +	if (stripe_len != BTRFS_STRIPE_LEN) {

Again too strict. As mentined elsewhere, add a helper to validate
stripe_len and use it so we don't open-code it.

>  		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
>  			  stripe_len);
>  		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 == 0) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
> +	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
> +	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&

I was looking if we could turn that into some generic checks using the
btrfs_raid_array but seems that the tests do not make a uniform pattern,
eg the DUP and SINGLE disguised as "mask == 0". As we don't add new
profiles too often I'm ok with that version.

> +	     num_stripes != 1)) {
> +		btrfs_err(root->fs_info, "Invalid num_stripes:sub_stripes %u:%u for profile %llu",

"invalid..." (no initial capital letter) and put the string on the next
line so it does not exceed 80 cols

> +			  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);
> +	/* Validation check */

Redundant comment (from the time when the validation was not in a
wrapper)

> +	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);
> @@ -6502,6 +6544,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);
> @@ -6568,6 +6611,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liu Bo]

On Wed, May 04, 2016 at 03:29:35PM +0200, David Sterba wrote:
> On Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote:
> > This adds valid checks for super_total_bytes, super_bytes_used and
> > super_stripesize.
> > 
> > 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>
> > ---
> >  fs/btrfs/disk-io.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 4e47849..988d03f 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> > +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> > +		ret = -EINVAL;
> > +	}
> > +	if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) {
> 
> Similar to total_bytes (sum of device->total_bytes), bytes_used is sum
> of of all device->used_bytes, which in turn is sum of all block group
> sizes on the device.

super_bytes_used has different meanings with device->used_bytes,
device->used_bytes is space that has been allocated to block groups,
super_bytes_used is space that has been consumed by data/metadata.

> 
> > +		printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n",
> > +		       btrfs_super_bytes_used(sb));
> > +		ret = -EINVAL;
> > +	}
> > +	if (btrfs_super_stripesize(sb) != 4096) {
> 
> This is too strict. The stripesize is unused, but we not force it to be
> 4k, a multiple of nodesize/sectorsize should be enough.

Hmm, in fact stripesize is used in find_free_extent(),

find_free_extent() {
	...
	search_start = ALIGN(offset, root->stripesize);
	...
}

and in open_ctree(),

open_ctree() {
	...
	stripesize = btrfs_super_stripesize(disk_super);
	...
	tree_root->stripesize = stripesize;
	...
}

btrfs_read_roots() {
	...
	btrfs_read_tree_root()  --> __setup_root(..., tree_root->stripesize, ...)
}

Thus, this stripesize has to be sectorsize at least.

Thanks,

-liubo

> 
> > +		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));
> > -- 
> > 2.5.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liu Bo]

On Wed, May 04, 2016 at 03:23:29PM +0200, David Sterba wrote:
> On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote:
> > 
> > 
> > Liu Bo wrote on 2016/05/02 11:15 -0700:
> > > This adds valid checks for super_total_bytes, super_bytes_used and
> > > super_stripesize.
> > >
> > > 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>
> > > ---
> > >  fs/btrfs/disk-io.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > > index 4e47849..988d03f 100644
> > > --- a/fs/btrfs/disk-io.c
> > > +++ b/fs/btrfs/disk-io.c
> > > @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
> > > +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
> > > +		ret = -EINVAL;
> > > +	}
> > 
> > Would it be better if using "6 * nodesize"?
> > 
> > I'd like to use a precious low limit on total bytes, but we don't have 
> > such value, so 6 nodesize would be good.
> 
> An early check can compare against some reasonable value, but the
> total_bytes value must be equal to the sum of all device sizes
> (disk_total_bytes). I'm not sure if we have enough information to verify
> that at this point though.

That's what I had in mind, the problem is that only the first device information is recorded in superblock.

At this moment We have device_num but we don't know the size of other devices.

Thanks,

-liubo

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

[Qu Wenruo]

Liu Bo wrote on 2016/05/03 16:36 -0700:
> On Tue, May 03, 2016 at 09:12:01AM +0800, Qu Wenruo wrote:
>>
>>
>> Liu Bo wrote on 2016/05/02 11:15 -0700:
>>> 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.
>>
>> Looks good for me.
>>
>> But would you mind to do extra check on some minor members like owner,
>> io_align, io_width and sub_stripes?
>> Since we have a dedicated function now, if not too hard, it's never a bad
>> idea to check every member for best robust.
>
> OK.
>
>>
>> Especially sub_stripes, as it seems to be used by division in
>> btrfs_rmap_block().
>
> It makes sense to check sub_stripes, but io_align/width are not used at
> this moment, I'd leave it for the future.
>
> What do you think?
>
> Thanks,
>
> -liubo
>

I'm OK if io_align/width is not used yet.
It's just a personal favor.

Adding such check when we really use io_align/width may be a better idea.

Thanks,
Qu

>>
>> Thanks,
>> Qu
>>>
>>> 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>
>>> ---
>>> fs/btrfs/volumes.c | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 68 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index bd0f45f..1075573 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -6206,27 +6206,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);
>>> @@ -6237,24 +6233,70 @@ 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 %llu",
>>> +			  (unsigned long long)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);
>>> 		return -EIO;
>>> 	}
>>> -	if (!is_power_of_2(stripe_len)) {
>>> +	if (stripe_len != BTRFS_STRIPE_LEN) {
>>> 		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
>>> 			  stripe_len);
>>> 		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 == 0) ||
>>> +	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
>>> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
>>> +	    (type & BTRFS_BLOCK_GROUP_RAID5 && 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);
>>> +	/* Validation check */
>>> +	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);
>>> @@ -6502,6 +6544,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);
>>> @@ -6568,6 +6611,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;
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

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

[Qu Wenruo]

Liu Bo wrote on 2016/05/04 10:44 -0700:
> On Wed, May 04, 2016 at 03:23:29PM +0200, David Sterba wrote:
>> On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote:
>>>
>>>
>>> Liu Bo wrote on 2016/05/02 11:15 -0700:
>>>> This adds valid checks for super_total_bytes, super_bytes_used and
>>>> super_stripesize.
>>>>
>>>> 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>
>>>> ---
>>>>  fs/btrfs/disk-io.c | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index 4e47849..988d03f 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -4120,6 +4120,20 @@ 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_total_bytes(sb) == 0) {
>>>> +		printk(KERN_ERR "BTRFS: total bytes is zero\n");
>>>> +		ret = -EINVAL;
>>>> +	}
>>>
>>> Would it be better if using "6 * nodesize"?
>>>
>>> I'd like to use a precious low limit on total bytes, but we don't have
>>> such value, so 6 nodesize would be good.
>>
>> An early check can compare against some reasonable value, but the
>> total_bytes value must be equal to the sum of all device sizes
>> (disk_total_bytes). I'm not sure if we have enough information to verify
>> that at this point though.
>
> That's what I had in mind, the problem is that only the first device information is recorded in superblock.
>
> At this moment We have device_num but we don't know the size of other devices.
>
> Thanks,
>
> -liubo
>
>
What about error out if we found sb->total_bytes < 
sb->dev_item->total_bytes?

As we are just doing early check, no need to be comprehensive, but spot 
obvious problem.

For exact device_num and sb->total_bytes, we may do post check when 
device tree are loaded?
Splitting early_check() and post_check() seems valid for me.
(Also I prefer post_check() just warning, not forced exit)

Thanks,
Qu

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

[David Sterba]

On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote:
> >> An early check can compare against some reasonable value, but the
> >> total_bytes value must be equal to the sum of all device sizes
> >> (disk_total_bytes). I'm not sure if we have enough information to verify
> >> that at this point though.
> >
> > That's what I had in mind, the problem is that only the first device information is recorded in superblock.
> >
> > At this moment We have device_num but we don't know the size of other devices.
> >
> > Thanks,
> >
> > -liubo
> >
> >
> What about error out if we found sb->total_bytes < 
> sb->dev_item->total_bytes?
> 
> As we are just doing early check, no need to be comprehensive, but spot 
> obvious problem.

Ok.

> For exact device_num and sb->total_bytes, we may do post check when 
> device tree are loaded?
> Splitting early_check() and post_check() seems valid for me.
> (Also I prefer post_check() just warning, not forced exit)

Why just warning? Superblock total_bytes and device sizes must be
correct, otherwise all sorts of operations can fail randomly.

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

[David Sterba]

On Wed, May 04, 2016 at 10:40:02AM -0700, Liu Bo wrote:
> > > +		printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n",
> > > +		       btrfs_super_bytes_used(sb));
> > > +		ret = -EINVAL;
> > > +	}
> > > +	if (btrfs_super_stripesize(sb) != 4096) {
> > 
> > This is too strict. The stripesize is unused, but we not force it to be
> > 4k, a multiple of nodesize/sectorsize should be enough.
> 
> Hmm, in fact stripesize is used in find_free_extent(),
> 
> find_free_extent() {
> 	...
> 	search_start = ALIGN(offset, root->stripesize);
> 	...
> }
> 
> and in open_ctree(),
> 
> open_ctree() {
> 	...
> 	stripesize = btrfs_super_stripesize(disk_super);
> 	...
> 	tree_root->stripesize = stripesize;
> 	...
> }
> 
> btrfs_read_roots() {
> 	...
> 	btrfs_read_tree_root()  --> __setup_root(..., tree_root->stripesize, ...)
> }
> 
> Thus, this stripesize has to be sectorsize at least.

Yes, and must be power of two because of ALIGN for now.

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

[Qu Wenruo]

David Sterba wrote on 2016/05/06 16:35 +0200:
> On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote:
>>>> An early check can compare against some reasonable value, but the
>>>> total_bytes value must be equal to the sum of all device sizes
>>>> (disk_total_bytes). I'm not sure if we have enough information to verify
>>>> that at this point though.
>>>
>>> That's what I had in mind, the problem is that only the first device information is recorded in superblock.
>>>
>>> At this moment We have device_num but we don't know the size of other devices.
>>>
>>> Thanks,
>>>
>>> -liubo
>>>
>>>
>> What about error out if we found sb->total_bytes <
>> sb->dev_item->total_bytes?
>>
>> As we are just doing early check, no need to be comprehensive, but spot
>> obvious problem.
>
> Ok.
>
>> For exact device_num and sb->total_bytes, we may do post check when
>> device tree are loaded?
>> Splitting early_check() and post_check() seems valid for me.
>> (Also I prefer post_check() just warning, not forced exit)
>
> Why just warning? Superblock total_bytes and device sizes must be
> correct, otherwise all sorts of operations can fail randomly.
>
>
Because if we exit, we can't even do fsck.

Maybe we need a new flag to control whether exit or warn at post_check().

Thanks,
Qu

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

[Liu Bo]

On Mon, May 09, 2016 at 09:31:37AM +0800, Qu Wenruo wrote:
> 
> 
> David Sterba wrote on 2016/05/06 16:35 +0200:
> > On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote:
> > > > > An early check can compare against some reasonable value, but the
> > > > > total_bytes value must be equal to the sum of all device sizes
> > > > > (disk_total_bytes). I'm not sure if we have enough information to verify
> > > > > that at this point though.
> > > > 
> > > > That's what I had in mind, the problem is that only the first device information is recorded in superblock.
> > > > 
> > > > At this moment We have device_num but we don't know the size of other devices.
> > > > 
> > > > Thanks,
> > > > 
> > > > -liubo
> > > > 
> > > > 
> > > What about error out if we found sb->total_bytes <
> > > sb->dev_item->total_bytes?
> > > 
> > > As we are just doing early check, no need to be comprehensive, but spot
> > > obvious problem.
> > 
> > Ok.

I'm gonna check for total_bytes and num_devices after loading chunk
tree.

> > 
> > > For exact device_num and sb->total_bytes, we may do post check when
> > > device tree are loaded?
> > > Splitting early_check() and post_check() seems valid for me.
> > > (Also I prefer post_check() just warning, not forced exit)
> > 
> > Why just warning? Superblock total_bytes and device sizes must be
> > correct, otherwise all sorts of operations can fail randomly.
> > 
> > 
> Because if we exit, we can't even do fsck.
> 
> Maybe we need a new flag to control whether exit or warn at post_check().
> 

> Thanks,
> Qu
>

IMHO for kernel part, we have to exit in order to avoid any panic due to those invalid value.

For fsck code, we can go forth and back to fix them.  In fact I don't
think fsck could work out anything, as superblock checksum has _matched_
but the values inside superblock are invalid, in this case we cannot trust
 other parts in this FS image, then how can we expect fsck to fix it by reading
other parts?

Thanks,

-liubo

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

[Qu Wenruo]

On 05/14/2016 02:14 AM, Liu Bo wrote:
> On Mon, May 09, 2016 at 09:31:37AM +0800, Qu Wenruo wrote:
>>
>>
>> David Sterba wrote on 2016/05/06 16:35 +0200:
>>> On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote:
>>>>>> An early check can compare against some reasonable value, but the
>>>>>> total_bytes value must be equal to the sum of all device sizes
>>>>>> (disk_total_bytes). I'm not sure if we have enough information to verify
>>>>>> that at this point though.
>>>>>
>>>>> That's what I had in mind, the problem is that only the first device information is recorded in superblock.
>>>>>
>>>>> At this moment We have device_num but we don't know the size of other devices.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -liubo
>>>>>
>>>>>
>>>> What about error out if we found sb->total_bytes <
>>>> sb->dev_item->total_bytes?
>>>>
>>>> As we are just doing early check, no need to be comprehensive, but spot
>>>> obvious problem.
>>>
>>> Ok.
>
> I'm gonna check for total_bytes and num_devices after loading chunk
> tree.
>
>>>
>>>> For exact device_num and sb->total_bytes, we may do post check when
>>>> device tree are loaded?
>>>> Splitting early_check() and post_check() seems valid for me.
>>>> (Also I prefer post_check() just warning, not forced exit)
>>>
>>> Why just warning? Superblock total_bytes and device sizes must be
>>> correct, otherwise all sorts of operations can fail randomly.
>>>
>>>
>> Because if we exit, we can't even do fsck.
>>
>> Maybe we need a new flag to control whether exit or warn at post_check().
>>
>
>> Thanks,
>> Qu
>>
>
> IMHO for kernel part, we have to exit in order to avoid any panic due to those invalid value.

I'm OK with this.

>
> For fsck code, we can go forth and back to fix them.  In fact I don't
> think fsck could work out anything, as superblock checksum has _matched_
> but the values inside superblock are invalid, in this case we cannot trust
>  other parts in this FS image, then how can we expect fsck to fix it by reading
> other parts?

For rw fsck, that may cause huge problem and I agree with you on error out.
But for ro fsck, it's a little overkilled for me.

Currently, if we found error in extent tree, we still continue checking 
fstree, to shows what is really wrong.

And for case like btrfs-image restored images, its dev extents doesn't 
even match with its chunk (may be it's already fixed?), but that's not a 
big problem for ro btrfsck, and we can go on without problem.

So the same case is here for ro btrfsck.
As long as that's ro btrfsck, we could just continue as we don't really 
need the total_bytes in superblock.

Thanks,
Qu

>
> Thanks,
>
> -liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Liu Bo]

On Wed, May 04, 2016 at 03:56:26PM +0200, David Sterba wrote:
> A few minor comments below
> 
> On Mon, May 02, 2016 at 11:15:51AM -0700, Liu Bo wrote:
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -6206,27 +6206,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);
> > @@ -6237,24 +6233,70 @@ 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 %llu",
> > +			  (unsigned long long)btrfs_chunk_sector_size(leaf,
> 
> type cast not necessry
> 
> > +								      chunk));
> > +		return -EIO;
> > +	}
> >  	if (!length || !IS_ALIGNED(length, root->sectorsize)) {
> >  		btrfs_err(root->fs_info,
> >  			"invalid chunk length %llu", length);
> >  		return -EIO;
> >  	}
> > -	if (!is_power_of_2(stripe_len)) {
> > +	if (stripe_len != BTRFS_STRIPE_LEN) {
> 
> Again too strict. As mentined elsewhere, add a helper to validate
> stripe_len and use it so we don't open-code it.

I'm not sure I understand the comment about open-code, right now
the value must be BTRFS_STRIPE_LEN and we don't set any other value,
are we going to add a helper for just (stripe_len != BTRFS_STRIPE_LEN)?

I fixed other issues.

Thanks,

-liubo

> 
> >  		btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
> >  			  stripe_len);
> >  		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 == 0) ||
> > +	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> > +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> > +	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
> > +	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
> > +	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
> 
> I was looking if we could turn that into some generic checks using the
> btrfs_raid_array but seems that the tests do not make a uniform pattern,
> eg the DUP and SINGLE disguised as "mask == 0". As we don't add new
> profiles too often I'm ok with that version.
> 
> > +	     num_stripes != 1)) {
> > +		btrfs_err(root->fs_info, "Invalid num_stripes:sub_stripes %u:%u for profile %llu",
> 
> "invalid..." (no initial capital letter) and put the string on the next
> line so it does not exceed 80 cols
> 
> > +			  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);
> > +	/* Validation check */
> 
> Redundant comment (from the time when the validation was not in a
> wrapper)
> 
> > +	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);
> > @@ -6502,6 +6544,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);
> > @@ -6568,6 +6611,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
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Sterba]

On Fri, May 13, 2016 at 04:57:17PM -0700, Liu Bo wrote:
> > > +								      chunk));
> > > +		return -EIO;
> > > +	}
> > >  	if (!length || !IS_ALIGNED(length, root->sectorsize)) {
> > >  		btrfs_err(root->fs_info,
> > >  			"invalid chunk length %llu", length);
> > >  		return -EIO;
> > >  	}
> > > -	if (!is_power_of_2(stripe_len)) {
> > > +	if (stripe_len != BTRFS_STRIPE_LEN) {
> > 
> > Again too strict. As mentined elsewhere, add a helper to validate
> > stripe_len and use it so we don't open-code it.
> 
> I'm not sure I understand the comment about open-code, right now
> the value must be BTRFS_STRIPE_LEN and we don't set any other value,
> are we going to add a helper for just (stripe_len != BTRFS_STRIPE_LEN)?

mkfs.btrfs will create stripesize == 4k, forcing this to
BTRFS_STRIPE_LEN will suddenly prevent mounting of lots of filesystems.
IIRC the stripe length check was done in several places so even if the
helper is simple we'll change the condition in one place once we'll
decide what are the acceptable values.

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

[David Sterba]

On Sat, May 14, 2016 at 07:42:26AM +0800, Qu Wenruo wrote:
> > For fsck code, we can go forth and back to fix them.  In fact I don't
> > think fsck could work out anything, as superblock checksum has _matched_
> > but the values inside superblock are invalid, in this case we cannot trust
> >  other parts in this FS image, then how can we expect fsck to fix it by reading
> > other parts?
> 
> For rw fsck, that may cause huge problem and I agree with you on error out.
> But for ro fsck, it's a little overkilled for me.
> 
> Currently, if we found error in extent tree, we still continue checking 
> fstree, to shows what is really wrong.
> 
> And for case like btrfs-image restored images, its dev extents doesn't 
> even match with its chunk (may be it's already fixed?), but that's not a 
> big problem for ro btrfsck, and we can go on without problem.
> 
> So the same case is here for ro btrfsck.
> As long as that's ro btrfsck, we could just continue as we don't really 
> need the total_bytes in superblock.

AFAICS super_block->total_bytes is used in btrfs_alloc_chunk and in
several places in mkfs, but 'check' does not look at the value at all,
so warning for the read-only should be ok.