btrfs: tree-checker: Verify block_group_item
PATCH v2 1/1 btrfs: tree-checker: Verify block_group_item
As reported in https://bugzilla.kernel.org/show_bug.cgi?id=199849,
a crafted image with invalid block group items could make free space cache code to cause panic.
We could early detect such invalid block group item by checking:
1) Size (key)
We have a up limit on block group item (10G)
2) Chunk objectid
3) Type
Exactly 1 bit set for type and no more than 1 bit set for profile
4) Used space
No more than block group size.
This should allow btrfs to detect and refuse to mount the crafted image.
---
changelog:
v2:
Format change for if() code block, added one new line for each block. Use "hweight64() > 1" to detect profile error, replacing the bloated !(flags & MASK == 0 || hweight64() == 1) condition.
--- fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)
fs/btrfs/tree-checker.c +92 -0
@@ -353,6 +353,95 @@ static int check_dir_item(struct btrfs_fs_info *fs_info,return 0 ; } +__printf(4, 5) +__cold +static void block_group_err (const struct btrfs_fs_info *fs_info, +const struct extent_buffer *eb,int slot, +const char *fmt, ...) +{ +struct btrfs_key key; +struct va_format vaf; +va_list args; + +btrfs_item_key_to_cpu (eb, &key, slot); +va_start (args, fmt); + + vaf.fmt = fmt; + vaf.va = &args; + +btrfs_crit (fs_info, +"corrupt %s: root=%llu block=%llu slot=%d bg_start=%llu bg_len=%llu, %pV" , +btrfs_header_level (eb) ==0 ?"leaf" :"node" , +btrfs_header_owner (eb),btrfs_header_bytenr (eb), slot, + key.objectid, key.offset, &vaf); +va_end (args); +} + +static int check_block_group_item (struct btrfs_fs_info *fs_info, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{ +struct btrfs_block_group_item bgi; +u32 item_size =btrfs_item_size_nr (leaf, slot); +u64 flags; + +/* +* Here we don't really care about unalignment since extent allocator +* can handle it. +* We care more about the size, as if one block group is larger than +* maximum size, it's must be some obvious corruption +*/ +if (key->offset >10ULL *SZ_1G ) { +block_group_err (fs_info, leaf, slot, +"invalid block group size, have %llu expect (0, %llu)" , + key->offset,10ULL *SZ_1G ); +return -EUCLEAN ; + } + +if (item_size !=sizeof (bgi)) { +block_group_err (fs_info, leaf, slot, +"invalid item size, have %u expect %lu" , + item_size,sizeof (bgi)); +return -EUCLEAN ; + } + +read_extent_buffer (leaf, &bgi,btrfs_item_ptr_offset (leaf, slot), +sizeof (bgi)); +if (btrfs_block_group_chunk_objectid (&bgi) != +BTRFS_FIRST_CHUNK_TREE_OBJECTID ) { +block_group_err (fs_info, leaf, slot, +"invalid block group chunk objectid, have %llu expect %llu" , +btrfs_block_group_chunk_objectid (&bgi), +BTRFS_FIRST_CHUNK_TREE_OBJECTID ); +return -EUCLEAN ; + } + +if (btrfs_block_group_used (&bgi) > key->offset) { +block_group_err (fs_info, leaf, slot, +"invalid block group used, have %llu expect [0, %llu)" , +btrfs_block_group_used (&bgi), key->offset); +return -EUCLEAN ; + } + + flags =btrfs_block_group_flags (&bgi); +if (hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) >1 ) { +block_group_err (fs_info, leaf, slot, +"invalid profile flags, have 0x%llx (%lu bits set) expect no more than 1 bit set" , + flags &BTRFS_BLOCK_GROUP_PROFILE_MASK , +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK )); +return -EUCLEAN ; + } + +if (hweight64 (flags &BTRFS_BLOCK_GROUP_TYPE_MASK ) !=1 ) { +block_group_err (fs_info, leaf, slot, +"invalid type flags, have 0x%llx (%lu bits set) expect exactly 1 bit set" , + flags &BTRFS_BLOCK_GROUP_TYPE_MASK , +hweight64 (flags &BTRFS_BLOCK_GROUP_TYPE_MASK )); +return -EUCLEAN ; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */
@@ -374,6 +463,9 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,case BTRFS_XATTR_ITEM_KEY : ret =check_dir_item (fs_info, leaf, key, slot);break ; +case BTRFS_BLOCK_GROUP_ITEM_KEY : + ret =check_block_group_item (fs_info, leaf, key, slot); +break ; }return ret; }
Signature · 1 line
2.18.0
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 431
Quoted 5 lines · Qu Wenruo
fs/btrfs/tree-checker.c +5
-0+if (hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) >1 ) { +block_group_err (fs_info, leaf, slot, +"invalid profile flags, have 0x%llx (%lu bits set) expect no more than 1 bit set" , + flags &BTRFS_BLOCK_GROUP_PROFILE_MASK , +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ));
Shouldn't this also be a hweight64() != 1 check ?
Quoted 10 lines · Nikolay Borisov
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 431
Quoted 5 lines · Qu Wenruo
fs/btrfs/tree-checker.c +5
-0+if (hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) >1 ) { +block_group_err (fs_info, leaf, slot, +"invalid profile flags, have 0x%llx (%lu bits set) expect no more than 1 bit set" , + flags &BTRFS_BLOCK_GROUP_PROFILE_MASK , +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ));Shouldn't this also be a
hweight64()!= 1 check ?
What about SINGLE profile on disk?
It doesn't has any profile bit set.
Thanks,
Qu
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Nikolay Borisov
Reviewed-byQuoted 14 lines · Qu Wenruo
Quoted 10 lines · Nikolay Borisov
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 431
Quoted 5 lines · Qu Wenruo
fs/btrfs/tree-checker.c +5
-0+if (hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) >1 ) { +block_group_err (fs_info, leaf, slot, +"invalid profile flags, have 0x%llx (%lu bits set) expect no more than 1 bit set" , + flags &BTRFS_BLOCK_GROUP_PROFILE_MASK , +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ));Shouldn't this also be a
hweight64()!= 1 check ?
Quoted 2 lines
What about SINGLE profile on disk?
It doesn't has any profile bit set.
Agh, it's implied, doesn't even have a define. Then it's fine I guess, huhz so many "special cases" ...
Quoted 10 lines
Thanks,
QuQuoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 397
Quoted 12 lines · Qu Wenruo
As reported in https://bugzilla.kernel.org/show_bug.cgi?id=199849,
a crafted image with invalid block group items could make free space cache code to cause panic.We could early detect such invalid block group item by checking:
1) Size (key)
We have a up limit on block group item (10G)fs/btrfs/tree-checker.c +4
-0+if (key->offset >10ULL *SZ_1G ) { +block_group_err (fs_info, leaf, slot, +"invalid block group size, have %llu expect (0, %llu)" , + key->offset,10ULL *SZ_1G );
Can you please make this magic constant a define and explain in a comment how it's calculated? Thanks.
Quoted 9 lines · David Sterba
Quoted 7 lines · Qu Wenruo
As reported in https://bugzilla.kernel.org/show_bug.cgi?id=199849,
a crafted image with invalid block group items could make free space cache code to cause panic.We could early detect such invalid block group item by checking:
1) Size (key)
We have a up limit on block group item (10G)
Quoted 7 lines
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 397
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+if (key->offset >10ULL *SZ_1G ) { +block_group_err (fs_info, leaf, slot, +"invalid block group size, have %llu expect (0, %llu)" , + key->offset,10ULL *SZ_1G );Can you please make this magic constant a define and explain in a comment how it's calculated? Thanks.
Of course, it's used in btrfs_alloc_chunk(), and I'll make it a constant define.
Thanks,
Qu
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html