| Message ID | 20180702055348.25623-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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",
~~^
%u
vim +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
On 2.07.2018 08:53, Qu Wenruo wrote: > 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 <wen.xu@gatech.edu> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 8d40e7dd8c30..a42187ba50d7 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -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, 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 . > + 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; > + } Put an empty line after each if to distinguish each part more easily. > + 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)) { 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 :) > + 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; > } > -- 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
On 2018年07月02日 15:28, Nikolay Borisov wrote: > > > On 2.07.2018 08:53, Qu Wenruo wrote: >> 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 <wen.xu@gatech.edu> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 88 insertions(+) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 8d40e7dd8c30..a42187ba50d7 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -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, > 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 . Nope, it's ensured to be a leaf. The caller is only from check_leaf(), whose name explains itself. >> + 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; >> + } > > Put an empty line after each if to distinguish each part more easily. > >> + 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)) { > > 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) In fact, "hweight64() > 1" is good enough. As all zero and only 1 set bit both fits above check. I'll use hweight64() only. > > It's easy to miss the ! right before the two (( and it causes a mild > head scratch :) Yeah, hweight64() > 1 solves all. Thanks, Qu > >> + 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; >> } >> -- 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
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 8d40e7dd8c30..a42187ba50d7 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -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; }
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 <wen.xu@gatech.edu> Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)