On Fri, Jun 7, 2024 at 12:31 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> 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?
it looks clearer.
>
> --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
> >
> >
