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

[Su Yue <Damenly_Su@gmx.com>]

On 2019/12/18 10:17 AM, Qu Wenruo wrote:
> 
> 
> 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].
> 

Reviewed-by: Su Yue <Damenly_Su@gmx.com>

> 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);
>>>
>>
>