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

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

[lei lu]

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@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..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;
+
 		/*
 		 * If it's unused, look for the space in the bestfree table.
 		 * If we find it, account for that, else make sure it
@@ -188,6 +193,8 @@ __xfs_dir3_data_check(
 
 			if (lastfree != 0)
 				return __this_address;
+			if (be16_to_cpu(dup->length) % XFS_DIR2_DATA_ALIGN != 0)
+				return __this_address;
 			if (offset + be16_to_cpu(dup->length) > end)
 				return __this_address;
 			if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
-- 
2.34.1

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

[lei lu]

Hi, Dave,

Do you have further comments and/or suggestions?

Thanks,
LL

On Mon, Jun 3, 2024 at 4:02 PM lei lu <llfamsec@gmail.com> 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@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..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;
> +
>                 /*
>                  * If it's unused, look for the space in the bestfree table.
>                  * If we find it, account for that, else make sure it
> @@ -188,6 +193,8 @@ __xfs_dir3_data_check(
>
>                         if (lastfree != 0)
>                                 return __this_address;
> +                       if (be16_to_cpu(dup->length) % XFS_DIR2_DATA_ALIGN != 0)
> +                               return __this_address;
>                         if (offset + be16_to_cpu(dup->length) > end)
>                                 return __this_address;
>                         if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
> --
> 2.34.1
>

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

[Dave Chinner]

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@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..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@redhat.com>
-- 
Dave Chinner
david@fromorbit.com