On Mon, Jun 03, 2024 at 04:01:46PM +0800, lei lu wrote:
> This adds sanity checks for xfs_dir2_data_unused and xfs_dir2_data_entry
> to make sure don'y stray beyond valid memory region. Before patching, the
> loop simply checks that the start offset of the dup and dep is within the
> range. So in a crafted image, if last entry is xfs_dir2_data_unused, we
> can change dup->length to dup->length-1 and leave 1 byte of space. 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.
>
> In the patch, we check dup->length % XFS_DIR2_DATA_ALIGN != 0 to make
> sure that dup is 8 byte aligned. And we also check the size of each entry
> is greater than xfs_dir2_data_entsize(mp, 1) which ensures that there is
> sufficient space to access fixed members. It should be noted that if the
> last object in the buffer is less than xfs_dir2_data_entsize(mp, 1) bytes
> in size it must be a dup entry of exactly XFS_DIR2_DATA_ALIGN bytes in
> length.
>
> Signed-off-by: lei lu <llfamsec@xxxxxxxxx>
> ---
> 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..dd6d43cdf0c5 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -178,6 +178,11 @@ __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 > end - xfs_dir2_data_entsize(mp, 1))
> + if (end - offset != XFS_DIR2_DATA_ALIGN ||
> + be16_to_cpu(dup->freetag) != XFS_DIR2_DATA_FREE_TAG)
> + return __this_address;
> +
Needs {} around the if. With that fixed:
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
