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

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

[Guo Xuenan]

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
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+# Import common functions.
+. ./common/populate
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+_require_check_dmesg
+
+echo "Format and mount"
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount  >> $seqres.full 2>&1
+
+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')"
+leaf_lblk="$((32 * 1073741824 / blksz))"
+node_lblk="$((64 * 1073741824 / blksz))"
+__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"
+
+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
+_repair_scratch_fs >> $seqres.full 2>&1
+
+# check demsg error
+_check_dmesg
+
+# 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

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

[Zorro Lang]

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?

> +#
> +. ./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 ??

> +_require_check_dmesg
> +
> +echo "Format and mount"
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount  >> $seqres.full 2>&1
> +
> +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.

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

> +
> +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
> +_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?

What's the xfs_repair expect? Fix all curruption and left a clean xfs?

> +
> +# check demsg error
> +_check_dmesg

Which above step will trigger a dmesg you want to check? What kind of dmesg do
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?

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
> 

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

[Guo Xuenan]

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

>> +_require_check_dmesg
>> +
>> +echo "Format and mount"
>> +_scratch_mkfs > $seqres.full 2>&1
>> +_scratch_mount  >> $seqres.full 2>&1
>> +
>> +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.
>> +__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}"
>> +
>> +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
>> +_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.
> 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)"
>> +
>> +# 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...

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

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

[Zorro Lang]

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 ??
> 
> > > +_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.

> > > +
> > > +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 :)

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

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

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

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

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

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

[Guo Xuenan]

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

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

[Zorro Lang]

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')"
> > > > 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 ...

OK, cc Darrick to ask why we need both dblksz and blksz at here?

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

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
> 

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

[Darrick J. Wong]

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

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

[Guo Xuenan]

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     | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554.out |  7 ++++
 2 files changed, 103 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..dba6aefa
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,96 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Check the running state of the XFS under illegal bestfree count
+# for leaf directory format.
+
+. ./common/preamble
+_begin_fstest auto quick
+
+# Import common functions.
+. ./common/populate
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+
+# Get last dirblock entries
+__lastdb_entry_list()
+{
+	local dir_ino=$1
+	local entry_list=$2
+	local nblocks=`_scratch_xfs_db -c "inode $dir_ino" -c "p core.nblocks" |
+			sed -e 's/^.*core.nblocks = //g' -e 's/\([0-9]*\).*$/\1/g'`
+	local last_db=$((nblocks / 2))
+	_scratch_xfs_db -c "inode $dir_ino" -c "dblock ${last_db}" -c 'p du' |\
+		grep ".name =" | sed -e 's/^.*.name = //g' \
+		-e 's/\"//g' > ${entry_list} ||\
+		_fail "get last dir block entries failed"
+}
+
+echo "Format and mount"
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount
+
+echo "Create and check leaf dir"
+blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
+dirblksz=`$XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize |
+	sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'`
+# Usually, following routine will create a directory with one leaf block
+# and three data block, meanwhile, the last data block is not full.
+__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dirblksz / 12))"
+leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
+_scratch_unmount
+
+# Delete directory entries in the last directory block,
+last_db_entries=$tmp.ldb_ents
+__lastdb_entry_list ${leaf_dir} ${last_db_entries}
+_scratch_mount
+cat ${last_db_entries} >> $seqres.full
+cat ${last_db_entries} | while read f
+do
+	rm -rf ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/$f
+done
+_scratch_unmount
+
+# Check leaf directory
+leaf_lblk="$((32 * 1073741824 / blksz))"
+node_lblk="$((64 * 1073741824 / blksz))"
+__populate_check_xfs_dir "${leaf_dir}" "leaf"
+
+# Inject abnormal bestfree count
+echo "Inject bad bestfree count."
+_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock ${leaf_lblk}" \
+	-c "write ltail.bestcount 0"
+# Adding new entries to S_IFDIR.FMT_LEAF. Since we delete the files
+# in last direcotry block, current dir block have no spare space for new
+# entry. With ltail.bestcount setting illegally (eg. bestcount=0), then
+# creating new directories, which will trigger xfs to allocate new dir
+# block, meanwhile, exception will be triggered.
+# Root cause is that xfs don't examin the number bestfree slots, when the
+# slots number less than dir data blocks, if we need to allocate new dir
+# data block and update the bestfree array, we will use the dir block number
+# as index to assign bestfree array, while we did not check the leaf buf
+# boundary which may cause UAF or other memory access problem.
+echo "Add directory entries to trigger exception."
+_scratch_mount
+seq 1 $((dirblksz / 24)) | while read d
+do
+mkdir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/TEST$(printf "%.04d" "$d")" >> $seqres.full 2>&1
+done
+_scratch_unmount
+
+# Bad bestfree count should be found and fixed by xfs_repair
+_scratch_xfs_repair -n >> $seqres.full 2>&1
+egrep -q 'leaf block.*bad tail' $seqres.full && echo "Repair found problems."
+_repair_scratch_fs >> $seqres.full 2>&1 || _fail "Repair failed!"
+
+# Check demsg error
+_check_dmesg
+
+# Success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..d966ab0a
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,7 @@
+QA output created by 554
+Format and mount
+Create and check leaf dir
+Inject bad bestfree count.
+ltail.bestcount = 0
+Add a directory entry to trigger exception.
+Repair found problems.
-- 
2.25.1

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

[Zorro Lang]

On Wed, Sep 28, 2022 at 05:53:55PM +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     | 96 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/554.out |  7 ++++
>  2 files changed, 103 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..dba6aefa
> --- /dev/null
> +++ b/tests/xfs/554
> @@ -0,0 +1,96 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
> +#
> +# FS QA Test No. 554
> +#
> +# Check the running state of the XFS under illegal bestfree count
> +# for leaf directory format.
> +
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +# Import common functions.
> +. ./common/populate
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_require_scratch
> +
> +# Get last dirblock entries
> +__lastdb_entry_list()

I think you don't need to use "__" for a case internal function.

> +{
> +	local dir_ino=$1
> +	local entry_list=$2
> +	local nblocks=`_scratch_xfs_db -c "inode $dir_ino" -c "p core.nblocks" |
> +			sed -e 's/^.*core.nblocks = //g' -e 's/\([0-9]*\).*$/\1/g'`

_scratch_xfs_get_metadata_field ...

> +	local last_db=$((nblocks / 2))

I'm not a xfs expert, what's this mean? Why nblocks/2 is the last data block?
For example, if we get a directory inode as this:
  u3.bmx[0-3] = [startoff,startblock,blockcount,extentflag]
  0:[0,14,2,0]
  1:[2,10,2,0]
  2:[4,72,6,0]
  3:[8388608,12,2,0]

do you want to get the "2:[4,72,6,0]" ?

> +	_scratch_xfs_db -c "inode $dir_ino" -c "dblock ${last_db}" -c 'p du' |\
> +		grep ".name =" | sed -e 's/^.*.name = //g' \
> +		-e 's/\"//g' > ${entry_list} ||\
> +		_fail "get last dir block entries failed"
> +}
> +
> +echo "Format and mount"
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount
> +
> +echo "Create and check leaf dir"
> +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> +dirblksz=`$XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize |
> +	sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'`
> +# Usually, following routine will create a directory with one leaf block
> +# and three data block, meanwhile, the last data block is not full.
> +__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dirblksz / 12))"
> +leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
> +_scratch_unmount
> +
> +# Delete directory entries in the last directory block,
> +last_db_entries=$tmp.ldb_ents
> +__lastdb_entry_list ${leaf_dir} ${last_db_entries}

Hmm... I don't like to give a tmp file to a function to get a name list,
how about:

  lastdb_entry_list ${leaf_dir} > $tmp.ldb_ents

Or ...(below)

> +_scratch_mount
> +cat ${last_db_entries} >> $seqres.full
> +cat ${last_db_entries} | while read f
> +do
> +	rm -rf ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/$f
> +done
> +_scratch_unmount

... you even can make all above code into a function named
remove_lastdb_entries() or other name you like.

This new part (not in v1) makes this case more complicated than v1, is it
necessary to reproduce the bug? Do we have a simple way?

> +
> +# Check leaf directory
> +leaf_lblk="$((32 * 1073741824 / blksz))"
> +node_lblk="$((64 * 1073741824 / blksz))"
> +__populate_check_xfs_dir "${leaf_dir}" "leaf"
> +
> +# Inject abnormal bestfree count
> +echo "Inject bad bestfree count."
> +_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock ${leaf_lblk}" \
> +	-c "write ltail.bestcount 0"
> +# Adding new entries to S_IFDIR.FMT_LEAF. Since we delete the files
> +# in last direcotry block, current dir block have no spare space for new
> +# entry. With ltail.bestcount setting illegally (eg. bestcount=0), then
> +# creating new directories, which will trigger xfs to allocate new dir
> +# block, meanwhile, exception will be triggered.
> +# Root cause is that xfs don't examin the number bestfree slots, when the
> +# slots number less than dir data blocks, if we need to allocate new dir
> +# data block and update the bestfree array, we will use the dir block number
> +# as index to assign bestfree array, while we did not check the leaf buf
> +# boundary which may cause UAF or other memory access problem.
> +echo "Add directory entries to trigger exception."
> +_scratch_mount
> +seq 1 $((dirblksz / 24)) | while read d
> +do
> +mkdir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/TEST$(printf "%.04d" "$d")" >> $seqres.full 2>&1

Indentation

> +done
> +_scratch_unmount
> +
> +# Bad bestfree count should be found and fixed by xfs_repair
> +_scratch_xfs_repair -n >> $seqres.full 2>&1
> +egrep -q 'leaf block.*bad tail' $seqres.full && echo "Repair found problems."

If you don't care about the xfs_repair output, you can do it simply as:
  _scratch_xfs_repair -n >> $seqres.full 2>&1 && \
          echo "repair didn't find corruption?"

Or you can filter the output of `_scratch_xfs_repair -n` to be golden image.

> +_repair_scratch_fs >> $seqres.full 2>&1 || _fail "Repair failed!"

How about:

  _repair_scratch_fs >> $seqres.full 2>&1
  _scratch_xfs_repair -n >> $seqres.full 2>&1 || _fail "xfs_repair should not fail"

(not sure, welcome more suggestions)

> +
> +# Check demsg error
> +_check_dmesg

You don't need to do that manually, except your dmesg error isn't in the
default checking list of _check_dmesg.

> +
> +# Success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> new file mode 100644
> index 00000000..d966ab0a
> --- /dev/null
> +++ b/tests/xfs/554.out
> @@ -0,0 +1,7 @@
> +QA output created by 554
> +Format and mount
> +Create and check leaf dir
> +Inject bad bestfree count.
> +ltail.bestcount = 0
> +Add a directory entry to trigger exception.
> +Repair found problems.
> -- 
> 2.25.1
> 

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

[Darrick J. Wong]

On Thu, Sep 29, 2022 at 07:32:13PM +0800, Zorro Lang wrote:
> On Wed, Sep 28, 2022 at 05:53:55PM +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     | 96 +++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/554.out |  7 ++++
> >  2 files changed, 103 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..dba6aefa
> > --- /dev/null
> > +++ b/tests/xfs/554
> > @@ -0,0 +1,96 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 554
> > +#
> > +# Check the running state of the XFS under illegal bestfree count
> > +# for leaf directory format.
> > +
> > +. ./common/preamble
> > +_begin_fstest auto quick
> > +
> > +# Import common functions.
> > +. ./common/populate
> > +
> > +# real QA test starts here
> > +_supported_fs xfs
> > +_require_scratch
> > +
> > +# Get last dirblock entries
> > +__lastdb_entry_list()
> 
> I think you don't need to use "__" for a case internal function.
> 
> > +{
> > +	local dir_ino=$1
> > +	local entry_list=$2
> > +	local nblocks=`_scratch_xfs_db -c "inode $dir_ino" -c "p core.nblocks" |
> > +			sed -e 's/^.*core.nblocks = //g' -e 's/\([0-9]*\).*$/\1/g'`
> 
> _scratch_xfs_get_metadata_field ...
> 
> > +	local last_db=$((nblocks / 2))
> 
> I'm not a xfs expert, what's this mean? Why nblocks/2 is the last data block?
> For example, if we get a directory inode as this:
>   u3.bmx[0-3] = [startoff,startblock,blockcount,extentflag]
>   0:[0,14,2,0]
>   1:[2,10,2,0]
>   2:[4,72,6,0]
>   3:[8388608,12,2,0]
> 
> do you want to get the "2:[4,72,6,0]" ?

I think they want 9 (offset (4) + blockcount (6) - 1)?

I'm also not sure what this computation does...

> > +	_scratch_xfs_db -c "inode $dir_ino" -c "dblock ${last_db}" -c 'p du' |\
> > +		grep ".name =" | sed -e 's/^.*.name = //g' \
> > +		-e 's/\"//g' > ${entry_list} ||\
> > +		_fail "get last dir block entries failed"

...though from the 'print du' command, I'm fairly sure they want to
extract the dirents from the last directory data block.  So yes, 9.

This is sorta ugly, I think this means that you'd have to 'print u3.bmx'
and then walk the bmbt entries to return the fileoff of the last block
before $leaf_lblk.

> > +}
> > +
> > +echo "Format and mount"
> > +_scratch_mkfs > $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +echo "Create and check leaf dir"
> > +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> > +dirblksz=`$XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize |
> > +	sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'`
> > +# Usually, following routine will create a directory with one leaf block
> > +# and three data block, meanwhile, the last data block is not full.



> > +__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dirblksz / 12))"
> > +leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
> > +_scratch_unmount
> > +
> > +# Delete directory entries in the last directory block,
> > +last_db_entries=$tmp.ldb_ents
> > +__lastdb_entry_list ${leaf_dir} ${last_db_entries}
> 
> Hmm... I don't like to give a tmp file to a function to get a name list,
> how about:
> 
>   lastdb_entry_list ${leaf_dir} > $tmp.ldb_ents
> 
> Or ...(below)
> 
> > +_scratch_mount
> > +cat ${last_db_entries} >> $seqres.full
> > +cat ${last_db_entries} | while read f
> > +do
> > +	rm -rf ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/$f
> > +done
> > +_scratch_unmount
> 
> ... you even can make all above code into a function named
> remove_lastdb_entries() or other name you like.
> 
> This new part (not in v1) makes this case more complicated than v1, is it
> necessary to reproduce the bug? Do we have a simple way?
> 
> > +
> > +# Check leaf directory
> > +leaf_lblk="$((32 * 1073741824 / blksz))"
> > +node_lblk="$((64 * 1073741824 / blksz))"
> > +__populate_check_xfs_dir "${leaf_dir}" "leaf"
> > +
> > +# Inject abnormal bestfree count
> > +echo "Inject bad bestfree count."
> > +_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock ${leaf_lblk}" \
> > +	-c "write ltail.bestcount 0"
> > +# Adding new entries to S_IFDIR.FMT_LEAF. Since we delete the files

It would be nice to have a blank line before the comments so that the
test doesn't look quite so much like a wall of text.

> > +# in last direcotry block, current dir block have no spare space for new

s/direcotry/directory/

> > +# entry. With ltail.bestcount setting illegally (eg. bestcount=0), then
> > +# creating new directories, which will trigger xfs to allocate new dir
> > +# block, meanwhile, exception will be triggered.
> > +# Root cause is that xfs don't examin the number bestfree slots, when the
> > +# slots number less than dir data blocks, if we need to allocate new dir
> > +# data block and update the bestfree array, we will use the dir block number
> > +# as index to assign bestfree array, while we did not check the leaf buf
> > +# boundary which may cause UAF or other memory access problem.

I suppose this implies that the test should be tagged dangerous for now?

> > +echo "Add directory entries to trigger exception."
> > +_scratch_mount
> > +seq 1 $((dirblksz / 24)) | while read d
> > +do
> > +mkdir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/TEST$(printf "%.04d" "$d")" >> $seqres.full 2>&1
> 
> Indentation
> 
> > +done
> > +_scratch_unmount
> > +
> > +# Bad bestfree count should be found and fixed by xfs_repair
> > +_scratch_xfs_repair -n >> $seqres.full 2>&1
> > +egrep -q 'leaf block.*bad tail' $seqres.full && echo "Repair found problems."
> 
> If you don't care about the xfs_repair output, you can do it simply as:
>   _scratch_xfs_repair -n >> $seqres.full 2>&1 && \
>           echo "repair didn't find corruption?"
> 
> Or you can filter the output of `_scratch_xfs_repair -n` to be golden image.

I think they're trying to look specifically for xfs_repair complaining
about the bad leaf1 block tail, not just any error.

--D

> > +_repair_scratch_fs >> $seqres.full 2>&1 || _fail "Repair failed!"
> 
> How about:
> 
>   _repair_scratch_fs >> $seqres.full 2>&1
>   _scratch_xfs_repair -n >> $seqres.full 2>&1 || _fail "xfs_repair should not fail"
> 
> (not sure, welcome more suggestions)
> 
> > +
> > +# Check demsg error
> > +_check_dmesg
> 
> You don't need to do that manually, except your dmesg error isn't in the
> default checking list of _check_dmesg.
> 
> > +
> > +# Success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> > new file mode 100644
> > index 00000000..d966ab0a
> > --- /dev/null
> > +++ b/tests/xfs/554.out
> > @@ -0,0 +1,7 @@
> > +QA output created by 554
> > +Format and mount
> > +Create and check leaf dir
> > +Inject bad bestfree count.
> > +ltail.bestcount = 0
> > +Add a directory entry to trigger exception.
> > +Repair found problems.
> > -- 
> > 2.25.1
> > 
> 

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

[Zorro Lang]

On Thu, Sep 29, 2022 at 02:04:19PM -0700, Darrick J. Wong wrote:
> On Thu, Sep 29, 2022 at 07:32:13PM +0800, Zorro Lang wrote:
> > On Wed, Sep 28, 2022 at 05:53:55PM +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     | 96 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/xfs/554.out |  7 ++++
> > >  2 files changed, 103 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..dba6aefa
> > > --- /dev/null
> > > +++ b/tests/xfs/554
> > > @@ -0,0 +1,96 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 554
> > > +#
> > > +# Check the running state of the XFS under illegal bestfree count
> > > +# for leaf directory format.
> > > +
> > > +. ./common/preamble
> > > +_begin_fstest auto quick
> > > +
> > > +# Import common functions.
> > > +. ./common/populate
> > > +
> > > +# real QA test starts here
> > > +_supported_fs xfs
> > > +_require_scratch
> > > +
> > > +# Get last dirblock entries
> > > +__lastdb_entry_list()
> > 
> > I think you don't need to use "__" for a case internal function.
> > 
> > > +{
> > > +	local dir_ino=$1
> > > +	local entry_list=$2
> > > +	local nblocks=`_scratch_xfs_db -c "inode $dir_ino" -c "p core.nblocks" |
> > > +			sed -e 's/^.*core.nblocks = //g' -e 's/\([0-9]*\).*$/\1/g'`
> > 
> > _scratch_xfs_get_metadata_field ...
> > 
> > > +	local last_db=$((nblocks / 2))
> > 
> > I'm not a xfs expert, what's this mean? Why nblocks/2 is the last data block?
> > For example, if we get a directory inode as this:
> >   u3.bmx[0-3] = [startoff,startblock,blockcount,extentflag]
> >   0:[0,14,2,0]
> >   1:[2,10,2,0]
> >   2:[4,72,6,0]
> >   3:[8388608,12,2,0]
> > 
> > do you want to get the "2:[4,72,6,0]" ?
> 
> I think they want 9 (offset (4) + blockcount (6) - 1)?
> 
> I'm also not sure what this computation does...
> 
> > > +	_scratch_xfs_db -c "inode $dir_ino" -c "dblock ${last_db}" -c 'p du' |\
> > > +		grep ".name =" | sed -e 's/^.*.name = //g' \
> > > +		-e 's/\"//g' > ${entry_list} ||\
> > > +		_fail "get last dir block entries failed"
> 
> ...though from the 'print du' command, I'm fairly sure they want to
> extract the dirents from the last directory data block.  So yes, 9.

OK, so he wants the last logic block of the last extent which store dir entries.

> 
> This is sorta ugly, I think this means that you'd have to 'print u3.bmx'
> and then walk the bmbt entries to return the fileoff of the last block
> before $leaf_lblk.

Yeah, it will be ugly, might be something like this:

remove_lastdb_entries()
{
	local dino=$1

	local leaf_bmx=$(_scratch_xfs_db -c "inode $dino" -c "p u3.bmx" | \
				sed -ne "/\[$leaf_lblk,/s/\(.*\)\:\[.*\]/\1/p")
	local lastdb_bmx=$((leaf_bmx - 1))
	local startoff=$(_scratch_xfs_db -c "inode $dino" -c "p u3.bmx" | \
				sed -ne "s/$lastdb_bmx:\[\(.*\),.*,.*,.*\]/\1/p")
	local blockcount=$(_scratch_xfs_db -c "inode $dino" -c "p u3.bmx" | \
				sed -ne "s/$lastdb_bmx:\[.*,.*,\(.*\),.*\]/\1/p")
	local lastdb=$((startoff + blockcount - 1))

	# remove all entries in the last logic data block
	_scratch_xfs_db -c "inode $dino" -c "dblock $lastdb" -c "p du" | \
		sed -ne "s/du.*\.name = \"\(.*\)\"/\1/p" | xargs rm -f
}

Or some better implements. Anyway these code looks like easy to broken :)

So I tried to ask if this step is necessary to reproduce the bug? The v1 patch
didn't have this step, is it can reproduce this bug ?
I think we don't need to make sure it's reproducible 100%, if we can simplify
a test case very much and keep high probability to reproduce the bug, that's
fine for me.

Thanks,
Zorro

> 
> > > +}
> > > +
> > > +echo "Format and mount"
> > > +_scratch_mkfs > $seqres.full 2>&1
> > > +_scratch_mount
> > > +
> > > +echo "Create and check leaf dir"
> > > +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> > > +dirblksz=`$XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize |
> > > +	sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'`
> > > +# Usually, following routine will create a directory with one leaf block
> > > +# and three data block, meanwhile, the last data block is not full.
> 
> 
> 
> > > +__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dirblksz / 12))"
> > > +leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
> > > +_scratch_unmount
> > > +
> > > +# Delete directory entries in the last directory block,
> > > +last_db_entries=$tmp.ldb_ents
> > > +__lastdb_entry_list ${leaf_dir} ${last_db_entries}
> > 
> > Hmm... I don't like to give a tmp file to a function to get a name list,
> > how about:
> > 
> >   lastdb_entry_list ${leaf_dir} > $tmp.ldb_ents
> > 
> > Or ...(below)
> > 
> > > +_scratch_mount
> > > +cat ${last_db_entries} >> $seqres.full
> > > +cat ${last_db_entries} | while read f
> > > +do
> > > +	rm -rf ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/$f
> > > +done
> > > +_scratch_unmount
> > 
> > ... you even can make all above code into a function named
> > remove_lastdb_entries() or other name you like.
> > 
> > This new part (not in v1) makes this case more complicated than v1, is it
> > necessary to reproduce the bug? Do we have a simple way?
> > 
> > > +
> > > +# Check leaf directory
> > > +leaf_lblk="$((32 * 1073741824 / blksz))"
> > > +node_lblk="$((64 * 1073741824 / blksz))"
> > > +__populate_check_xfs_dir "${leaf_dir}" "leaf"
> > > +
> > > +# Inject abnormal bestfree count
> > > +echo "Inject bad bestfree count."
> > > +_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock ${leaf_lblk}" \
> > > +	-c "write ltail.bestcount 0"
> > > +# Adding new entries to S_IFDIR.FMT_LEAF. Since we delete the files
> 
> It would be nice to have a blank line before the comments so that the
> test doesn't look quite so much like a wall of text.
> 
> > > +# in last direcotry block, current dir block have no spare space for new
> 
> s/direcotry/directory/
> 
> > > +# entry. With ltail.bestcount setting illegally (eg. bestcount=0), then
> > > +# creating new directories, which will trigger xfs to allocate new dir
> > > +# block, meanwhile, exception will be triggered.
> > > +# Root cause is that xfs don't examin the number bestfree slots, when the
> > > +# slots number less than dir data blocks, if we need to allocate new dir
> > > +# data block and update the bestfree array, we will use the dir block number
> > > +# as index to assign bestfree array, while we did not check the leaf buf
> > > +# boundary which may cause UAF or other memory access problem.
> 
> I suppose this implies that the test should be tagged dangerous for now?
> 
> > > +echo "Add directory entries to trigger exception."
> > > +_scratch_mount
> > > +seq 1 $((dirblksz / 24)) | while read d
> > > +do
> > > +mkdir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/TEST$(printf "%.04d" "$d")" >> $seqres.full 2>&1
> > 
> > Indentation
> > 
> > > +done
> > > +_scratch_unmount
> > > +
> > > +# Bad bestfree count should be found and fixed by xfs_repair
> > > +_scratch_xfs_repair -n >> $seqres.full 2>&1
> > > +egrep -q 'leaf block.*bad tail' $seqres.full && echo "Repair found problems."
> > 
> > If you don't care about the xfs_repair output, you can do it simply as:
> >   _scratch_xfs_repair -n >> $seqres.full 2>&1 && \
> >           echo "repair didn't find corruption?"
> > 
> > Or you can filter the output of `_scratch_xfs_repair -n` to be golden image.
> 
> I think they're trying to look specifically for xfs_repair complaining
> about the bad leaf1 block tail, not just any error.
> 
> --D
> 
> > > +_repair_scratch_fs >> $seqres.full 2>&1 || _fail "Repair failed!"
> > 
> > How about:
> > 
> >   _repair_scratch_fs >> $seqres.full 2>&1
> >   _scratch_xfs_repair -n >> $seqres.full 2>&1 || _fail "xfs_repair should not fail"
> > 
> > (not sure, welcome more suggestions)
> > 
> > > +
> > > +# Check demsg error
> > > +_check_dmesg
> > 
> > You don't need to do that manually, except your dmesg error isn't in the
> > default checking list of _check_dmesg.
> > 
> > > +
> > > +# Success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/xfs/554.out b/tests/xfs/554.out
> > > new file mode 100644
> > > index 00000000..d966ab0a
> > > --- /dev/null
> > > +++ b/tests/xfs/554.out
> > > @@ -0,0 +1,7 @@
> > > +QA output created by 554
> > > +Format and mount
> > > +Create and check leaf dir
> > > +Inject bad bestfree count.
> > > +ltail.bestcount = 0
> > > +Add a directory entry to trigger exception.
> > > +Repair found problems.
> > > -- 
> > > 2.25.1
> > > 
> > 
> 

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

[Guo Xuenan]

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     | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/554.out |  7 ++++
 2 files changed, 103 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..ee088fdc
--- /dev/null
+++ b/tests/xfs/554
@@ -0,0 +1,96 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Huawei Limited.  All Rights Reserved.
+#
+# FS QA Test No. 554
+#
+# Check the running state of the XFS under illegal bestfree count
+# for leaf directory format.
+
+. ./common/preamble
+_begin_fstest auto quick dangerous
+
+# Import common functions.
+. ./common/populate
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+
+# Remove all entries in the last logic dir data block
+leaf_dir_remove_lastdb_entries()
+{
+	local dir_ino=$1
+	local blks_pdirb=$2
+	local nblocks=$(_scratch_xfs_get_metadata_field 'core.nblocks' "inode ${dir_ino}")
+	#last dir data block index = total blocks - one leaf dir block - one dir block
+	local last_db=$((nblocks - blks_pdirb * 2))
+
+	echo "last dir block $last_db" >> $seqres.full
+	_scratch_xfs_db -c "inode $dir_ino" -c "dblock ${last_db}" -c 'p du' |\
+		grep ".name =" | sed -e 's/^.*.name = //g' \
+		-e 's/\"//g' > $tmp.ldb_ents ||\
+		_fail "get last dir block entries failed"
+	cat $tmp.ldb_ents >> $seqres.full
+	_scratch_mount
+	cat $tmp.ldb_ents | while read f
+	do
+		rm -rf ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/$f
+	done
+	_scratch_unmount
+}
+
+echo "Format and mount"
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount
+
+echo "Create and check leaf dir"
+blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
+dirblksz=`$XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize |
+	sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'`
+
+# Usually, following routine will create a directory with one leaf block
+# and three data block, meanwhile, the last data block is not full.
+__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dirblksz / 12))"
+leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")"
+_scratch_unmount
+
+# Delete directory entries in the last directory block,
+leaf_dir_remove_lastdb_entries ${leaf_dir} $((dirblksz / blksz))
+
+# Check leaf directory
+leaf_lblk="$((32 * 1073741824 / blksz))"
+node_lblk="$((64 * 1073741824 / blksz))"
+__populate_check_xfs_dir "${leaf_dir}" "leaf"
+
+# Inject abnormal bestfree count
+echo "Inject bad bestfree count."
+_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock ${leaf_lblk}" \
+	-c "write ltail.bestcount 0"
+
+# Adding new entries to S_IFDIR.FMT_LEAF. Since we delete the files
+# in last directory block, current dir block have no spare space for new
+# entry. With ltail.bestcount setting illegally (eg. bestcount=0), then
+# creating new directories, which will trigger xfs to allocate new dir
+# block, meanwhile, exception will be triggered.
+# Root cause is that xfs don't examin the number bestfree slots, when the
+# slots number less than dir data blocks, if we need to allocate new dir
+# data block and update the bestfree array, we will use the dir block number
+# as index to assign bestfree array, while we did not check the leaf buf
+# boundary which may cause UAF or other memory access problem.
+echo "Add directory entries to trigger exception."
+_scratch_mount
+seq 1 $((dirblksz / 24)) | while read d
+do
+	mkdir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/TEST$(printf "%.04d" "$d")" >> $seqres.full 2>&1
+done
+_scratch_unmount
+
+# Bad bestfree count should be found and fixed by xfs_repair
+_scratch_xfs_repair -n >> $seqres.full 2>&1
+egrep -q 'leaf block.*bad tail' $seqres.full && echo "Repair found problems."
+_repair_scratch_fs >> $seqres.full 2>&1 || _fail "Repair failed!"
+
+# Success, all done
+status=0
+exit
diff --git a/tests/xfs/554.out b/tests/xfs/554.out
new file mode 100644
index 00000000..398ee7d5
--- /dev/null
+++ b/tests/xfs/554.out
@@ -0,0 +1,7 @@
+QA output created by 554
+Format and mount
+Create and check leaf dir
+Inject bad bestfree count.
+ltail.bestcount = 0
+Add directory entries to trigger exception.
+Repair found problems.
-- 
2.25.1