* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-02 18:15 ` [PATCH 2/2] Btrfs: add valid checks for chunk loading Liu Bo
@ 2016-05-03 1:12 ` Qu Wenruo
2016-05-03 23:36 ` Liu Bo
2016-05-03 5:53 ` Anand Jain
2016-05-04 13:56 ` David Sterba
2 siblings, 1 reply; 24+ messages in thread
From: Qu Wenruo @ 2016-05-03 1:12 UTC (permalink / raw)
To: Liu Bo, linux-btrfs; +Cc: vegard.nossum, sterba
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;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-03 1:12 ` Qu Wenruo
@ 2016-05-03 23:36 ` Liu Bo
2016-05-05 1:03 ` Qu Wenruo
0 siblings, 1 reply; 24+ messages in thread
From: Liu Bo @ 2016-05-03 23:36 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, vegard.nossum, sterba
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-03 23:36 ` Liu Bo
@ 2016-05-05 1:03 ` Qu Wenruo
0 siblings, 0 replies; 24+ messages in thread
From: Qu Wenruo @ 2016-05-05 1:03 UTC (permalink / raw)
To: bo.li.liu; +Cc: linux-btrfs, vegard.nossum, sterba
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
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-02 18:15 ` [PATCH 2/2] Btrfs: add valid checks for chunk loading Liu Bo
2016-05-03 1:12 ` Qu Wenruo
@ 2016-05-03 5:53 ` Anand Jain
2016-05-03 23:33 ` Liu Bo
2016-05-04 13:56 ` David Sterba
2 siblings, 1 reply; 24+ messages in thread
From: Anand Jain @ 2016-05-03 5:53 UTC (permalink / raw)
To: Liu Bo, linux-btrfs; +Cc: vegard.nossum, sterba
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;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-03 5:53 ` Anand Jain
@ 2016-05-03 23:33 ` Liu Bo
0 siblings, 0 replies; 24+ messages in thread
From: Liu Bo @ 2016-05-03 23:33 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, vegard.nossum, sterba
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;
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-02 18:15 ` [PATCH 2/2] Btrfs: add valid checks for chunk loading Liu Bo
2016-05-03 1:12 ` Qu Wenruo
2016-05-03 5:53 ` Anand Jain
@ 2016-05-04 13:56 ` David Sterba
2016-05-13 23:57 ` Liu Bo
2 siblings, 1 reply; 24+ messages in thread
From: David Sterba @ 2016-05-04 13:56 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs, vegard.nossum, dsterba
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-04 13:56 ` David Sterba
@ 2016-05-13 23:57 ` Liu Bo
2016-05-17 13:37 ` David Sterba
0 siblings, 1 reply; 24+ messages in thread
From: Liu Bo @ 2016-05-13 23:57 UTC (permalink / raw)
To: dsterba, linux-btrfs, vegard.nossum
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] Btrfs: add valid checks for chunk loading
2016-05-13 23:57 ` Liu Bo
@ 2016-05-17 13:37 ` David Sterba
0 siblings, 0 replies; 24+ messages in thread
From: David Sterba @ 2016-05-17 13:37 UTC (permalink / raw)
To: Liu Bo; +Cc: dsterba, linux-btrfs, vegard.nossum
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.
^ permalink raw reply [flat|nested] 24+ messages in thread