Re: [PATCH fstests] xfs/554: xfs add illegal bestfree array size inject for leaf dir

[Guo Xuenan <guoxuenan@huawei.com>]

Hi Zorro:

On 2022/9/3 17:57, Zorro Lang wrote:
> On Sat, Sep 03, 2022 at 11:51:13AM +0800, Guo Xuenan wrote:
>> Hi Zorro：
>>
>> On 2022/9/3 9:39, Zorro Lang wrote:
>>> On Fri, Sep 02, 2022 at 05:40:46PM +0800, Guo Xuenan wrote:
>>>> Test leaf dir allocting new block when bestfree array size
>>>> less than data blocks count, which may lead to UAF.
>>>>
>>>> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
>>>> ---
>>>>    tests/xfs/554     | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/xfs/554.out |  6 ++++++
>>>>    2 files changed, 54 insertions(+)
>>>>    create mode 100755 tests/xfs/554
>>>>    create mode 100644 tests/xfs/554.out
>>>>
>>>> diff --git a/tests/xfs/554 b/tests/xfs/554
>>>> new file mode 100755
>>>> index 00000000..fcf45731
>>>> --- /dev/null
>>>> +++ b/tests/xfs/554
>>>> @@ -0,0 +1,48 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
>>>> +#
>>>> +# FS QA Test No. 554
>>>> +#
>>>> +# Test leaf dir bestfree array size match with dir disk size
>>> Is it for a known bug? known commit id?
>> The bug is being solved and waitting to be reviewed here[v1/v2].
>> [v1]
>> https://lore.kernel.org/all/20220902094046.3891252-1-guoxuenan@huawei.com/
>> [v2]
>> https://lore.kernel.org/all/20220831121639.3060527-1-guoxuenan@huawei.com/
>>>> +#
>>>> +. ./common/preamble
>>>> +_begin_fstest auto quick
>>>> +
>>>> +# Import common functions.
>>>> +. ./common/populate
>>>> +
>>>> +# real QA test starts here
>>>> +_supported_fs xfs
>>>> +_require_scratch
>>> Do you need V5 xfs? Or v4 is fine?
>>> _require_scratch_xfs_crc ??
Both v4 and v5 have this problem,
>>>> +_require_check_dmesg
>>>> +
>>>> +echo "Format and mount"
>>>> +_scratch_mkfs > $seqres.full 2>&1
>>>> +_scratch_mount  >> $seqres.full 2>&1
> If _scratch_mount fails, the testing will exit directly, so generally we just
> run _scratch_mount.
OK, you are right.
>>>> +
>>>> +echo "Create and check leaf dir"
>>>> +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>>>> +dblksz="$($XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
>>> Why do you need these two kinds of block size for xfs? And you sometimes
>>> use the former, sometimes use the later? If you'd like to get the xfs data
>>> block size, you can:
>>>
>>>     _scratch_mkfs | _filter_mkfs >>$seqres.full 2>$tmp.mkfs
>>>     . $tmp.mkfs
>>>
>>> Then "dbsize" is what you want.
>>>
>>>> +leaf_lblk="$((32 * 1073741824 / blksz))"
>>>> +node_lblk="$((64 * 1073741824 / blksz))"
>>> I didn't see the "node_lblk" is used in this case, looks like you don't want to
>>> get directory node blocks in this case.
>> It's really needed here, must define leaf_lblk and node_lblk before calling
>> __populate_check_xfs_dir
>> or an waring will be printed by the function.
> Oh, so these two global parameters are used for later __populate_check_xfs_dir.
> Hmm.. are "blksz" and "dblksz" necessary too, for someone __populate_* helper
> you used? I really don't understand why we need them both. These helpers are
> written by Darrick, I think he learns about that more :)
yes, there are same usage for eg. xfs/113 xfs/101 ...
>>>> +__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))"
>>>> +leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
>>>> +_scratch_unmount
>>>> +__populate_check_xfs_dir "${leaf_dir}" "leaf"
>>>> +
>>>> +echo "Inject bad bestfress array size"
>>>> +_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock 8388608" -c "write ltail.bestcount 0"
>>> As you tried to detect xfs block size above, so it might not 4k block size, so
>>> 8388608 is not fixed.
>>>
>>> According to the kernel definition:
>>>     #define XFS_DIR2_DATA_ALIGN_LOG 3
>>>     #define XFS_DIR2_SPACE_SIZE     (1ULL << (32 + XFS_DIR2_DATA_ALIGN_LOG))
>>>     #define XFS_DIR2_LEAF_SPACE     1
>>>     #define XFS_DIR2_LEAF_OFFSET    (XFS_DIR2_LEAF_SPACE * XFS_DIR2_SPACE_SIZE)
>>>
>>> The XFS_DIR2_LEAF_OFFSET = 1 * (1 << (32 + 3)) = 1<<35 = 34359738368 = 32GB, so
>>> the fixed logical offset of leaf extent is 34359738368 bytes, then the offset
>>> block number should be "34359738368 / dbsize". 8388608 is only for 4k block
>>> size.
>> Sorry, you are totally right! it should be "dblock ${leaf_lblk}"
> That looks better.
>
>>>> +
>>>> +echo "Test add entry to dir"
>>>> +_scratch_mount
>>>> +touch ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/{1..100}.txt > /dev/null 2>&1
>>>> +_scratch_unmount 2>&1
> This "2>&1" looks useless, I think it can be removed
OK, in v2 it will disappear :）
>>>> +_repair_scratch_fs >> $seqres.full 2>&1
>>> Can you explain more about this testing steps? The xfs has been corrupted, then
>>> we expect is can be mounted. And create 100 new files on that corrupted dir,
>>> do you expect the 100 files can be created successfully? Or what ever, even
>>> nothing be created?
>> since we have create an leaf dir,and set bestfree count to 0; then, need to
>> touch some files to
>> trigger the problem, the action will be failed as expected.
> OK, I think you can add more comments to explain this part. Due to you make
> a obvious corruption at first, then try to mount and write the corrupted fs,
> there must be some error happen, so you'd better to explain what do you
> expect, and what's not.
Thanks a lot, and I'm happliy accept your suggestion, It's really a bit 
confusing here,
I will add some specific description
>>> What's the xfs_repair expect? Fix all curruption and left a clean xfs?
>> Adding repair is really not necessary, only toavoid _check_xfs_filesystem
>> warning
>> " _check_xfs_filesystem: filesystem on /dev/sdb is inconsistent (r)"
> Except you'd like to verify if xfs_repair can fix this corruption. Or replace
> _require_scatch with _require_scratch_nocheck, that will help you avoid known
> fs corruption warning. Then you can remove _repair_scratch_fs and above
> _scratch_unmount.
Yep, you got my point, since it's my fisrt contribution code for 
fstests, and
I didn't figure out how to disable the post fsck execution. so great, I 
will
add _require_scratch_nocheck.
>>>> +
>>>> +# check demsg error
>>>> +_check_dmesg
>>> Which above step will trigger a dmesg you want to check? What kind of dmesg do
>> dmesg eg:
>> [   80.543884] XFS (sdb): Internal error xfs_dir2_data_use_free at line 1200
>> of file fs/xfs/libxfs/xfs_dir2_data.c.  Caller
>> xfs_dir2_data_use_free+0xb3/0xeb0
>> [   80.545141] CPU: 2 PID: 2978 Comm: touch Not tainted 6.0.0-rc3+ #115
>> [   80.545715] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> 1.13.0-1ubuntu1.1 04/01/2014
>> [   80.546546] Call Trace:
>> [   80.546785]  <TASK>
>> [   80.546985]  dump_stack_lvl+0x4d/0x66
>> [   80.547335] xfs_corruption_error+0x132/0x150
>> [   80.548391]  ? xfs_dir2_data_use_free+0xb3/0xeb0
>> [   80.548901]  ? xfs_dir2_data_use_free+0xb3/0xeb0
>> [   80.549319]  ? xfs_dir2_data_use_free+0xb3/0xeb0
>> [   80.550190] xfs_dir2_data_use_free+0x198/0xeb0
>> [   80.550718]  ? xfs_dir2_data_use_free+0xb3/0xeb0
>> [   80.551140] xfs_dir2_leaf_addname+0xa59/0x1ac0
>> [   80.551881]  ? _raw_spin_unlock_irqrestore+0x42/0x80
>> [   80.552403]  ? xfs_dir2_leaf_search_hash+0x300/0x300
>> or
>> [  201.405239] BUG: KASAN: slab-out-of-bounds in
>> xfs_dir2_leaf_addname+0x1995/0x1ac0
>> [  201.406179] Write of size 2 at addr ffff888078c33000 by task touch/7433
>> [  201.407010]
>> [  201.407217] CPU: 6 PID: 7433 Comm: touch Not tainted 6.0.0-rc3+ #115
>> [  201.408016] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> 1.13.0-1ubuntu1.1 04/01/2014
>> [  201.409143] Call Trace:
>> [  201.409461]  <TASK>
>> [  201.409740]  dump_stack_lvl+0x4d/0x66
>> [  201.410214]  print_report.cold+0xf6/0x691
>> [  201.410730]  ? xfs_dir3_data_init+0x18e/0x960
>>
>> UAF/slab-out-of bound etc...
> Look at the _check_dmesg, it checks "Internal error" and "\bBUG:" etc, so I
> think it can catch above dmesg error, you can remove this _check_dmesg and
> run again, to make sure if it works as you wish.
OK, I will check it again.
>>> you want to check? I think xfstests checks dmesg at the end of each test case,
>>> except you need to check some special one, or need a special filter?
>> check demsg without filter seems enough, so i did not add special filter.
>>> Thanks,
>>> Zorro
>>>
>>>> +
>>>> +# success, all done
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
>>>> new file mode 100644
>>>> index 00000000..ea1f30cc
>>>> --- /dev/null
>>>> +++ b/tests/xfs/554.out
>>>> @@ -0,0 +1,6 @@
>>>> +QA output created by 554
>>>> +Format and mount
>>>> +Create and check leaf dir
>>>> +Inject bad bestfress array size
>>>> +ltail.bestcount = 0
>>>> +Test add entry to dir
>>>> -- 
>>>> 2.25.1
>>>>
>>> .
> .
Thanks
Xuenan