btrfs: tree-checker: Verify block_group_item
PATCH 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.
--- fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
fs/btrfs/tree-checker.c +88 -0
@@ -353,6 +353,91 @@ 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 (!((flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) ==0 || +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 +459,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
kbuild test robot
build botThank you for the patch! Perhaps something to improve:
Hi Qu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on btrfs/next]
[also build test WARNING on v4.18-rc3 next-20180629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-tree-checker-Verify-block_group_item/20180702-135502 base: https://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git next config: i386-randconfig-x016-201826 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386
All warnings (new ones prefixed by >>):
fs/btrfs/tree-checker.c: In function 'check_block_group_item':
>> fs/btrfs/tree-checker.c:402:41: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'unsigned int' [-Wformat=]
"invalid item size, have %u expect %lu",
~~^
%uvim +402 fs/btrfs/tree-checker.c
379
380 static int check_block_group_item(struct btrfs_fs_info *fs_info,
381 struct extent_buffer *leaf,
382 struct btrfs_key *key, int slot)
383 {
384 struct btrfs_block_group_item bgi;
385 u32 item_size = btrfs_item_size_nr(leaf, slot);
386 u64 flags;
387
388 /*
389 * Here we don't really care about unalignment since extent allocator
390 * can handle it.
391 * We care more about the size, as if one block group is larger than
392 * maximum size, it's must be some obvious corruption
393 */
394 if (key->offset > 10ULL * SZ_1G) {
395 block_group_err(fs_info, leaf, slot,
396 "invalid block group size, have %llu expect (0, %llu)",
397 key->offset, 10ULL * SZ_1G);
398 return -EUCLEAN;
399 }
400 if (item_size != sizeof(bgi)) {
401 block_group_err(fs_info, leaf, slot,
> 402 "invalid item size, have %u expect %lu",
403 item_size, sizeof(bgi));
404 return -EUCLEAN;
405 }
406 read_extent_buffer(leaf, &bgi, btrfs_item_ptr_offset(leaf, slot),
407 sizeof(bgi));
408 if (btrfs_block_group_chunk_objectid(&bgi) !=
409 BTRFS_FIRST_CHUNK_TREE_OBJECTID) {
410 block_group_err(fs_info, leaf, slot,
411 "invalid block group chunk objectid, have %llu expect %llu",
412 btrfs_block_group_chunk_objectid(&bgi),
413 BTRFS_FIRST_CHUNK_TREE_OBJECTID);
414 return -EUCLEAN;
415 }
416 if (btrfs_block_group_used(&bgi) > key->offset) {
417 block_group_err(fs_info, leaf, slot,
418 "invalid block group used, have %llu expect [0, %llu)",
419 btrfs_block_group_used(&bgi), key->offset);
420 return -EUCLEAN;
421 }
422 flags = btrfs_block_group_flags(&bgi);
423 if (!((flags & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 ||
424 hweight64(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 1)) {
425 block_group_err(fs_info, leaf, slot,
426 "invalid profile flags, have 0x%llx (%lu bits set) expect no more than 1 bit set",
427 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK,
428 hweight64(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK));
429 return -EUCLEAN;
430 }
431 if (hweight64(flags & BTRFS_BLOCK_GROUP_TYPE_MASK) != 1) {
432 block_group_err(fs_info, leaf, slot,
433 "invalid type flags, have 0x%llx (%lu bits set) expect exactly 1 bit set",
434 flags & BTRFS_BLOCK_GROUP_TYPE_MASK,
435 hweight64(flags & BTRFS_BLOCK_GROUP_TYPE_MASK));
436 return -EUCLEAN;
437 }
438 return 0;
439 }
440
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Quoted 45 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)
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.
Reported-by Xu Wen <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)fs/btrfs/tree-checker.c +16
-0@@ -353,6 +353,91 @@ 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; +
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 381
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+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,
Seems it's not mandatory that this extent buffer points to a leaf, it might very well point to an interim node (judging from the btrfs_header_level() check in block_group_err). I'd suggest you use the more neutral - eb .
Quoted 8 lines
fs/btrfs/tree-checker.c +8
-0+ 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
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 399
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+* 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 ; + }
Put an empty line after each if to distinguish each part more easily.
Quoted 15 lines
fs/btrfs/tree-checker.c +15
-0+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 ;
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 424
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+ } +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 (!((flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) ==0 || +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) ==1 )) {
Can you make this condition a bit more stupid like:
if ((flags & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 || hweight64(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK) > 1)
It's easy to miss the ! right before the two (( and it causes a mild head scratch :)
Quoted 30 lines
fs/btrfs/tree-checker.c +19
-0+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 +459,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; }
Quoted 54 lines · Nikolay Borisov
Quoted 51 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)
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.
Reported-by Xu Wen <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)fs/btrfs/tree-checker.c +22
-0@@ -353,6 +353,91 @@ 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);
Quoted 8 lines
fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 381
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+} + +static int check_block_group_item(struct btrfs_fs_info *fs_info, + struct extent_buffer *leaf,Seems it's not mandatory that this extent buffer points to a leaf, it might very well point to an interim node (judging from the
btrfs_header_level()check inblock_group_err). I'd suggest you use the more neutral - eb .
Nope, it's ensured to be a leaf.
The caller is only from check_leaf(), whose name explains itself.
Quoted 49 lines
Quoted 8 lines
fs/btrfs/tree-checker.c +8
-0+ 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 fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 399
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+* 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 ; + }Put an empty line after each if to distinguish each part more easily.
Quoted 15 lines
fs/btrfs/tree-checker.c +15
-0+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 ;fs/btrfs/tree-checker.c static int check_dir_item(struct btrfs_fs_info *fs_info, · line 424
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+ } +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 (!((flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) ==0 || +hweight64 (flags &BTRFS_BLOCK_GROUP_PROFILE_MASK ) ==1 )) {Can you make this condition a bit more stupid like:
Quoted 1 line
if ((flags &
BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 ||
!=
Quoted 1 line
hweight64(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK)> 1)
In fact, "hweight64() > 1" is good enough.
As all zero and only 1 set bit both fits above check.
I'll use hweight64() only.
Quoted 3 lines
It's easy to miss the ! right before the two (( and it causes a mild head scratch :)
Yeah, hweight64() > 1 solves all.
Thanks,
Qu
Quoted 31 lines
Quoted 31 lines
fs/btrfs/tree-checker.c +19
-0+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 +459,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; }