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

[Zorro Lang <zlang@redhat.com>]

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
>