On Thu, Jun 06, 2024 at 11:14:16AM +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. 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>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_dir2_data.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index dbcf58979a59..71398ce0225f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -178,6 +178,12 @@ __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;
> + }
Let me work through the logic here. If @offset is too close to @end to
contain a dep for a single-byte name, then you check if it's an 8-byte
dup. If it's not a an 8-byte dup, then you bail out. Is that correct?
So if we get to this point in the function, either @offset is far enough
away from @end to contain a possibly valid dep; or it's an 8-byte
FREE_TAG region that's possibly correct.
I think the logic is correct, though I think it would be clearer if
you'd add this to xfs_dir2_priv.h:
static inline unsigned int
xfs_dir2_data_unusedsize(
unsigned int len)
{
return round_up(len, XFS_DIR2_DATA_ALIGN);
}
and modify the loop to read like this:
/*
* Loop over the data/unused entries.
*/
while (offset < end) {
struct xfs_dir2_data_unused *dup = bp->b_addr + offset;
struct xfs_dir2_data_entry *dep = bp->b_addr + offset;
unsigned int reclen;
/*
* Are the remaining bytes large enough to hold an
* unused entry?
*/
if (offset > end - xfs_dir2_data_unusedsize(1))
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
* doesn't need to be there.
*/
if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
xfs_failaddr_t fa;
reclen = xfs_dir2_data_unusedsize(be16_to_cpu(dup->length));
if (lastfree != 0)
return __this_address;
if (be16_to_cpu(dup->length) != reclen)
return __this_address;
if (offset + reclen > end)
return __this_address;
...
offset += reclen;
continue;
}
/*
* This is not an unused entry. Are the remaining bytes
* large enough for a dirent with a single-byte name?
*/
if (offset > end - xfs_dir2_data_entsize(mp, 1))
return __this_address;
/*
* It's a real entry. Validate the fields.
* If this is a block directory then make sure it's
* in the leaf section of the block.
* The linear search is crude but this is DEBUG code.
*/
if (dep->namelen == 0)
return __this_address;
reclen = xfs_dir2_data_entsize(mp, dep->namelen);
if (offset + reclen > end)
return __this_address;
if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
return __this_address;
...
offset += reclen;
}
What do you all think?
--D
> +
> /*
> * 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 +194,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
>
>
