Re: [PATCH v2] xfs: fix uaf when leaf dir bestcount not match with dir data blocks

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 31, 2022 at 08:16:39PM +0800, Guo Xuenan wrote:
> For leaf dir, In most cases, there should be as many bestfree slots
> as the dir data blocks that can fit under i_size (except for [1]).
> 
> Root cause is we 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. This issue
> can also triggered with test cases xfs/473 from fstests.
> 
> Considering the special case [1] , only add check bestfree array boundary,
> to avoid UAF or slab-out-of bound.
> 
> [1] https://lore.kernel.org/all/163961697197.3129691.1911552605195534271.stgit@magnolia/
> 
> Simplify the testcase xfs/473 with commands below:
> DEV=/dev/sdb
> MP=/mnt/sdb
> WORKDIR=/mnt/sdb/341 #1. mkfs create new xfs image
> mkfs.xfs -f ${DEV}
> mount ${DEV} ${MP}
> mkdir -p ${WORKDIR}
> for i in `seq 1 341` #2. create leaf dir with 341 entries file name len 8
> do
>     touch ${WORKDIR}/$(printf "%08d" ${i})
> done
> inode=$(ls -i ${MP} | cut -d' ' -f1)
> umount ${MP}         #3. xfs_db set bestcount to 0
> xfs_db -x ${DEV} -c "inode ${inode}" -c "dblock 8388608" \
> -c "write ltail.bestcount 0"
> mount ${DEV} ${MP}
> touch ${WORKDIR}/{1..100}.txt #4. touch new file, reproduce
.....
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index d9b66306a9a7..4b2a72b3a6f3 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -819,6 +819,18 @@ xfs_dir2_leaf_addname(
>  		 */
>  		else
>  			xfs_dir3_leaf_log_bests(args, lbp, use_block, use_block);
> +		/*
> +		 * An abnormal corner case, bestfree count less than data
> +		 * blocks, add a condition to avoid UAF or slab-out-of bound.
> +		 */
> +		if ((char *)(&bestsp[use_block]) > (char *)ltp) {
> +			xfs_trans_brelse(tp, lbp);
> +			if (tp->t_flags & XFS_TRANS_DIRTY)
> +				xfs_force_shutdown(tp->t_mountp,
> +						SHUTDOWN_CORRUPT_ONDISK);
> +			return -EFSCORRUPTED;
> +		}
> +

As I explained here:

https://lore.kernel.org/linux-xfs/20220829081244.GT3600936@dread.disaster.area/

We don't check for overruns caused by on-disk format corruptions in
every operation - we catch the corruption when the metadata is first
read into memory via the verifier.

Please add a check for a corrupt/mismatched best sizes array to
xfs_dir3_leaf_check_int() so that the corruption is detected on
first read and the internal directory code is never exposed to such
issues.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com