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

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Sep 04, 2022 at 08:55:49AM +0800, Zorro Lang wrote:
> On Sat, Sep 03, 2022 at 07:12:54PM +0800, Guo Xuenan wrote:
> > 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,
> 
> OK, that's fine if you can reproduce this bug on both.
> 
> > > > > > +_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')"

This is the dirblock size, not the filesystem blocksize...

> > > > > 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.

...so dirbsize 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 ...
> 
> OK, cc Darrick to ask why we need both dblksz and blksz at here?

dirbsize (aka naming.bsize) is the size of a dirent block, which you
need to compute the number of directory entries required to bloat up the
directory to be large enough to have the structure you want.

dbsize/blksz (aka fs blocksize) is used to compute the file block offset
(xfs_fileoff_t) where the leaf and node partitions begin.

I... think the geometry calculation code ought to be refactored into a
helper that will do all that for us.

> > > > > > +__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.

TBH you probably /ought/ to ensure that xfs_repair -n will find the
corruption and then run xfs_repair again to ensure that it fixes
that.

--D

> Sure, welcome more patches from you :)
> 
> > > > > > +
> > > > > > +# 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
> > 
>