Re: [PATCH v3 2/2] btrfs: Enhance chunk validation check

[Qu Wenruo]

On 12/29/2015 06:11 PM, Chandan Rajendra wrote:

On Tuesday 08 Dec 2015 16:40:42 Qu Wenruo wrote:

Enhance chunk validation:
1) Num_stripes
We already have such check but it's only in super block sys chunk
array.
Now check all on-disk chunks.

2) Chunk logical
It should be aligned to sector size.
This behavior should be *DOUBLE CHECKED* for 64K sector size like
PPC64 or AArch64.
Maybe we can found some hidden bugs.



Sorry about the delayed response. I executed fstests on ppc64 with 64k block
size and all the tests that used to pass earlier (i.e. without patch applied)
continue to pass. Hence,

Tested-by: Chandan Rajendra 


Very glad to hear that.

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

Re: [PATCH v3 2/2] btrfs: Enhance chunk validation check

[Chandan Rajendra]

On Tuesday 08 Dec 2015 16:40:42 Qu Wenruo wrote:
> Enhance chunk validation:
> 1) Num_stripes
>We already have such check but it's only in super block sys chunk
>array.
>Now check all on-disk chunks.
> 
> 2) Chunk logical
>It should be aligned to sector size.
>This behavior should be *DOUBLE CHECKED* for 64K sector size like
>PPC64 or AArch64.
>Maybe we can found some hidden bugs.
> 

Sorry about the delayed response. I executed fstests on ppc64 with 64k block
size and all the tests that used to pass earlier (i.e. without patch applied)
continue to pass. Hence,

Tested-by: Chandan Rajendra 

-- 
chandan

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

[PATCH v3 2/2] btrfs: Enhance chunk validation check

[Qu Wenruo]

Enhance chunk validation:
1) Num_stripes
   We already have such check but it's only in super block sys chunk
   array.
   Now check all on-disk chunks.

2) Chunk logical
   It should be aligned to sector size.
   This behavior should be *DOUBLE CHECKED* for 64K sector size like
   PPC64 or AArch64.
   Maybe we can found some hidden bugs.

3) Chunk length
   Same as chunk logical, should be aligned to sector size.

4) Stripe length
   It should be power of 2.

5) Chunk type
   Any bit out of TYPE_MAS | PROFILE_MASK is invalid.

With all these much restrict rules, several fuzzed image reported in
mail list should no longer cause kernel panic.

Reported-by: Vegard Nossum 
Signed-off-by: Qu Wenruo 

---
v2:
  Fix a typo which forgot to return -EIO after num_stripes check.
---
 fs/btrfs/volumes.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9ea345f..bda84be 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6199,6 +6199,7 @@ static int read_one_chunk(struct btrfs_root *root, struct 
btrfs_key *key,
struct extent_map *em;
u64 logical;
u64 length;
+   u64 stripe_len;
u64 devid;
u8 uuid[BTRFS_UUID_SIZE];
int num_stripes;
@@ -6207,6 +6208,37 @@ static int read_one_chunk(struct btrfs_root *root, 
struct btrfs_key *key,
 
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 */
+   if (!num_stripes) {
+   btrfs_err(root->fs_info, "invalid chunk num_stripes: %u",
+ num_stripes);
+   return -EIO;
+   }
+   if (!IS_ALIGNED(logical, root->sectorsize)) {
+   btrfs_err(root->fs_info,
+ "invalid chunk logical %llu", logical);
+   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)) {
+   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)) {
+   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;
+   }
 
read_lock(_tree->map_tree.lock);
em = lookup_extent_mapping(_tree->map_tree, logical, 1);
@@ -6223,7 +6255,6 @@ static int read_one_chunk(struct btrfs_root *root, struct 
btrfs_key *key,
em = alloc_extent_map();
if (!em)
return -ENOMEM;
-   num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
map = kmalloc(map_lookup_size(num_stripes), GFP_NOFS);
if (!map) {
free_extent_map(em);
-- 
2.6.3



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

[PATCH v3 2/2] btrfs: Enhance chunk validation check

[Qu Wenruo]

Enhance chunk validation:
1) Num_stripes
   We already have such check but it's only in super block sys chunk
   array.
   Now check all on-disk chunks.

2) Chunk logical
   It should be aligned to sector size.
   This behavior should be *DOUBLE CHECKED* for 64K sector size like
   PPC64 or AArch64.
   Maybe we can found some hidden bugs.

3) Chunk length
   Same as chunk logical, should be aligned to sector size.

4) Stripe length
   It should be power of 2.

5) Chunk type
   Any bit out of TYPE_MAS | PROFILE_MASK is invalid.

With all these much restrict rules, several fuzzed image reported in
mail list should no longer cause kernel panic.

Reported-by: Vegard Nossum 
Signed-off-by: Qu Wenruo 

---
v3:
  Fix a typo which forgot to return -EIO after num_stripes check.
---
 fs/btrfs/volumes.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9ea345f..bda84be 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6199,6 +6199,7 @@ static int read_one_chunk(struct btrfs_root *root, struct 
btrfs_key *key,
struct extent_map *em;
u64 logical;
u64 length;
+   u64 stripe_len;
u64 devid;
u8 uuid[BTRFS_UUID_SIZE];
int num_stripes;
@@ -6207,6 +6208,37 @@ static int read_one_chunk(struct btrfs_root *root, 
struct btrfs_key *key,
 
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 */
+   if (!num_stripes) {
+   btrfs_err(root->fs_info, "invalid chunk num_stripes: %u",
+ num_stripes);
+   return -EIO;
+   }
+   if (!IS_ALIGNED(logical, root->sectorsize)) {
+   btrfs_err(root->fs_info,
+ "invalid chunk logical %llu", logical);
+   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)) {
+   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)) {
+   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;
+   }
 
read_lock(_tree->map_tree.lock);
em = lookup_extent_mapping(_tree->map_tree, logical, 1);
@@ -6223,7 +6255,6 @@ static int read_one_chunk(struct btrfs_root *root, struct 
btrfs_key *key,
em = alloc_extent_map();
if (!em)
return -ENOMEM;
-   num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
map = kmalloc(map_lookup_size(num_stripes), GFP_NOFS);
if (!map) {
free_extent_map(em);
-- 
2.6.3



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