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@xxxxxxxxxx> >> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
