Re: [PATCH 2/6] btrfs-progs: check/original: Do extra verification on file extent item

Qu Wenruo
2019-12-18T02:17:36Z

[-- Attachment #1.1: Type: text/plain, Size: 7344 bytes --]

On 2019/12/18 上午10:09, Su Yue wrote:
> On 2019/12/18 9:19 AM, Qu Wenruo wrote:
>> [BUG]
>> For certain fuzzed image, `btrfs check` will fail with the following
>> call trace:
>>    Checking filesystem on issue_213.raw
>>    UUID: 99e50868-0bda-4d89-b0e4-7e8560312ef9
>>    [1/7] checking root items
>>    [2/7] checking extents
>>    Program received signal SIGABRT, Aborted.
>>    0x00007ffff7c88f25 in raise () from /usr/lib/libc.so.6
>>    (gdb) bt
>>    #0  0x00007ffff7c88f25 in raise () from /usr/lib/libc.so.6
>>    #1  0x00007ffff7c72897 in abort () from /usr/lib/libc.so.6
>>    #2  0x00005555555abc3e in run_next_block (...) at check/main.c:6398
>>    #3  0x00005555555b0f36 in deal_root_from_list (...) at
>> check/main.c:8408
>>    #4  0x00005555555b1a3d in check_chunks_and_extents
>> (fs_info=0x5555556a1e30) at check/main.c:8690
>>    #5  0x00005555555b1e3e in do_check_chunks_and_extents
>> (fs_info=0x5555556a1e30) a
>>    #6  0x00005555555b5710 in cmd_check (cmd=0x555555696920
>> <cmd_struct_check>, argc
>>    #7  0x0000555555568dc7 in cmd_execute (cmd=0x555555696920
>> <cmd_struct_check>, ar
>>    #8  0x0000555555569713 in main (argc=2, argv=0x7fffffffde70) at
>> btrfs.c:386
>>
>> [CAUSE]
>> This fuzzed images has a corrupted EXTENT_DATA item in data reloc tree:
>>          item 1 key (256 EXTENT_DATA 256) itemoff 16111 itemsize 12
>>                  generation 0 type 2 (prealloc)
>>                  prealloc data disk byte 16777216 nr 0
>>                  prealloc data offset 0 nr 0
>>
>> There are several problems with the item:
>> - Bad item size
>>    12 is too small.
>> - Bad key offset
>>    offset of EXTENT_DATA type key represents file offset, which should
>>    always be aligned to sector size (4K in this particular case).
>>
>> [FIX]
>> Do extra item size and key offset check for original mode, and remove
>> the abort() call in run_next_block().
>>
>> And to show off how robust lowmem mode is, lowmem can handle it without
>> any hiccup.
>>
>> With this fix, original mode can detect the problem properly:
>>    Checking filesystem on issue_213.raw
>>    UUID: 99e50868-0bda-4d89-b0e4-7e8560312ef9
>>    [1/7] checking root items
>>    [2/7] checking extents
>>    ERROR: invalid file extent item size, have 12 expect (21, 16283]
>>    ERROR: errors found in extent allocation tree or chunk allocation
>>    [3/7] checking free space cache
>>    [4/7] checking fs roots
>>    root 18446744073709551607 root dir 256 error
>>    root 18446744073709551607 inode 256 errors 62, no orphan item, odd
>> file extent, bad file extent
>>    ERROR: errors found in fs roots
>>    found 131072 bytes used, error(s) found
>>    total csum bytes: 0
>>    total tree bytes: 131072
>>    total fs tree bytes: 32768
>>    total extent tree bytes: 16384
>>    btree space waste bytes: 124774
>>    file data blocks allocated: 0
>>     referenced 0
>>
>> Issue: #213
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Almost fine. Two nitpicks below.
> I guess that they could be fixed when merging.
>
>> ---
>>   check/main.c | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/check/main.c b/check/main.c
>> index 08dc9e66..91752dce 100644
>> --- a/check/main.c
>> +++ b/check/main.c
>> @@ -6268,7 +6268,10 @@ static int run_next_block(struct btrfs_root *root,
>>           btree_space_waste += btrfs_leaf_free_space(buf);
>>           for (i = 0; i < nritems; i++) {
>>               struct btrfs_file_extent_item *fi;
>> +            unsigned long inline_offset;
>>
>> +            inline_offset = offsetof(struct btrfs_file_extent_item,
>> +                         disk_bytenr);
>>               btrfs_item_key_to_cpu(buf, &key, i);
>>               /*
>>                * Check key type against the leaf owner.
>> @@ -6384,18 +6387,45 @@ static int run_next_block(struct btrfs_root
>> *root,
>>               }
>>               if (key.type != BTRFS_EXTENT_DATA_KEY)
>>                   continue;
>> +            /* Check itemsize before we continue*/
>
> One more space at the tail.
>> +            if (btrfs_item_size_nr(buf, i) < inline_offset) {
>> +                ret = -EUCLEAN;
>> +                error(
>> +        "invalid file extent item size, have %u expect (%lu, %lu]",
>
> should it be "[%llu, %lu)"?

If the file extent size matches inline_offset, then it's an empty inline
file extent, which is not valid.
So left side must be '('.

For the right side, it can take the whole leaf, e.g. for 4K nodesize.

So it's (%llu, %lu].

Thanks,
Qu
>
> Thanks.
>> +                    btrfs_item_size_nr(buf, i),
>> +                    inline_offset,
>> +                    BTRFS_LEAF_DATA_SIZE(fs_info));
>> +                continue;
>> +            }
>>               fi = btrfs_item_ptr(buf, i,
>>                           struct btrfs_file_extent_item);
>>               if (btrfs_file_extent_type(buf, fi) ==
>>                   BTRFS_FILE_EXTENT_INLINE)
>>                   continue;
>> +
>> +            /* Prealloc/regular extent must have fixed item size */
>> +            if (btrfs_item_size_nr(buf, i) !=
>> +                sizeof(struct btrfs_file_extent_item)) {
>> +                ret = -EUCLEAN;
>> +                error(
>> +            "invalid file extent item size, have %u expect %zu",
>> +                    btrfs_item_size_nr(buf, i),
>> +                    sizeof(struct btrfs_file_extent_item));
>> +                continue;
>> +            }
>> +            /* key.offset (file offset) must be aligned */
>> +            if (!IS_ALIGNED(key.offset, fs_info->sectorsize)) {
>> +                ret = -EUCLEAN;
>> +                error(
>> +            "invalid file offset, have %llu expect aligned to %u",
>> +                    key.offset, fs_info->sectorsize);
>> +                continue;
>> +            }
>>               if (btrfs_file_extent_disk_bytenr(buf, fi) == 0)
>>                   continue;
>>
>>               data_bytes_allocated +=
>>                   btrfs_file_extent_disk_num_bytes(buf, fi);
>> -            if (data_bytes_allocated < root->fs_info->sectorsize)
>> -                abort();
>>
>>               data_bytes_referenced +=
>>                   btrfs_file_extent_num_bytes(buf, fi);
>>
>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]