Re: [PATCH] xfs: don't walk off the end of a directory data block

[Dave Chinner <david@fromorbit.com>]

On Thu, May 30, 2024 at 06:57:36AM +0800, lei lu wrote:
> This adds sanity checks for xfs_dir2_data_unused and xfs_dir2_data_entry
> to make sure don't stray beyond valid memory region. It just checks start
> offset < end without checking end offset < end.

Well, it does do this checking, but it assumes that the dup/dep
headers fit in the buffer because of entry size and alignment
constraints.

> So if last entry is
> xfs_dir2_data_unused, and is located at the end of ag.

Not sure what this means.

> We can change
> dup->length to dup->length-1 and leave 1 byte of space.

Ah, so not a real-world issue in any way.

Regardless, this is the corruption we are failing to catch.  All the
structures in the directory name area should be 8 byte aligned, and
we should be catching dup->length % XFS_DIR2_DATA_ALIGN != 0 and
reporting that as corruption.

This also means that the smallest valid length for dup->length is
xfs_dir2_data_entsize(mp, 1), except if it is the last entry in the
block (i.e. at end - offset == XFS_DIR2_DATA_ALIGN), in which case
it may be XFS_DIR2_DATA_ALIGN bytes in length.

IOWs, we're failing to check for both the alignment and the size
constraints on the dup->length field, and that's the problem we need
to fix to address the out of bounds read error being reported.

Can you please rework the patch to catch the corruption you induced
at the exact point we are processing the corrupt object, rather than
try to catch an overrun that might happen several iterations after
the corrupt object itself was processed?

> In the next
> traversal, this space will be considered as dup or dep. We may encounter
> an out-of-bound read when accessing the fixed members.

Verifiers are supposed to validate each object in the structure is
within specification, not be coded simply to prevent out of bounds
accesses. i.e. if the next traversal trips over an out of bounds
access, then one of the previous iobject verifications failed to
detect an out of bounds value that it should not have missed.

> Signed-off-by: lei lu <llfamsec@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_dir2_data.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index dbcf58979a59..08c18e0c1baa 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -178,6 +178,9 @@ __xfs_dir3_data_check(
>  		struct xfs_dir2_data_unused	*dup = bp->b_addr + offset;
>  		struct xfs_dir2_data_entry	*dep = bp->b_addr + offset;
>  
> +		if (offset + sizeof(*dup) > end)
> +			return __this_address;
> +
>  		/*
>  		 * If it's unused, look for the space in the bestfree table.
>  		 * If we find it, account for that, else make sure it
> @@ -210,6 +213,10 @@ __xfs_dir3_data_check(
>  			lastfree = 1;
>  			continue;
>  		}
> +
> +		if (offset + sizeof(*dep) > end)
> +			return __this_address;

That doesn't look correct - dep has a variable sized array and tail
packed information in it that sizeof(*dep) doesn't take into
account. The actual size of the dep structure we need to consider
here is going to be a minimum sized entry -
xfs_dir2_data_entsize(mp, 1) - as anything smaller than this size is
definitely invalid and we shouldn't attempt to decode any of it.

-Dave.
-- 
Dave Chinner
david@fromorbit.com