| Message ID | 20180702083414.13670-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2.07.2018 11:34, Qu Wenruo wrote: > + 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 ? -- 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日 16:37, Nikolay Borisov wrote: > > > On 2.07.2018 11:34, Qu Wenruo wrote: >> + 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 > -- > 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 > -- 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 2.07.2018 11:47, Qu Wenruo wrote: > > > On 2018年07月02日 16:37, Nikolay Borisov wrote: >> >> >> On 2.07.2018 11:34, Qu Wenruo wrote: >>> + 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. Agh, it's implied, doesn't even have a define. Then it's fine I guess, huhz so many "special cases" ... Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > Thanks, > Qu > >> -- >> 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 >> > -- 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 Mon, Jul 02, 2018 at 04:34:14PM +0800, 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) > + 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. -- 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日 22:48, David Sterba wrote: > On Mon, Jul 02, 2018 at 04:34:14PM +0800, 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) > >> + 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 > -- > 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..d4f872682668 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -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; }
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> --- 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(+)