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

[PATCH] 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't stray beyond valid memory region. It just checks start
offset < end without checking end offset < end. So if last entry is
xfs_dir2_data_unused, and is located at the end of ag. 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.

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;
+
 		/*
 		 * It's a real entry.  Validate the fields.
 		 * If this is a block directory then make sure it's
-- 
2.34.1

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

[Dave Chinner]

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

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

[lei lu]

Thanks for your time.

I just add check for the fixed members because I see after the patch
code there is some checks for dup and dep. "offset +
be16_to_cpu(dup->length) > end" for dup and "offset +
xfs_dir2_data_entsize(mp, dep->namelen) > end" for dep.
“xfs_dir2_data_entsize(mp, dep->namelen)” ensures the alignment of the
dep.

Dave Chinner <david@fromorbit.com> 于2024年5月30日周四 10:38写道：
>
> 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

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

[Dave Chinner]

On Thu, May 30, 2024 at 11:10:57AM +0800, lei lu wrote:
> Thanks for your time.
> 
> I just add check for the fixed members because I see after the patch
> code there is some checks for dup and dep. "offset +
> be16_to_cpu(dup->length) > end" for dup and "offset +
> xfs_dir2_data_entsize(mp, dep->namelen) > end" for dep.
> “xfs_dir2_data_entsize(mp, dep->namelen)” ensures the alignment of the
> dep.

Sure, but go back and read what I said.

Detect the actual object corruption, not the downstream symptom.

IOWs, the verifier should be detecting the exact corruption you
induced.

Catching all the object corruptions prevents a buffer overrun.
We abort processing before we move beyond the end of the buffer.

IOWs, we need to:

1. verify dup->length is a multiple of XFS_DIR2_DATA_ALIGN; and
2. verify 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.

If either of these checks fail, then the block is corrupt.
#1 will catch your induced corruption and fail immediately.
#2 will catch the runt entry in the structure without derefencing
past the end of the structure.

Can you now see how properly validating that the objects within the
structure will prevent buffer overruns from occurring without
needing generic buffer overrun checks?

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

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

[lei lu]

Got it. I will send a V2 patch to do a proper validation.

On Mon, Jun 3, 2024 at 1:58 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Thu, May 30, 2024 at 11:10:57AM +0800, lei lu wrote:
> > Thanks for your time.
> >
> > I just add check for the fixed members because I see after the patch
> > code there is some checks for dup and dep. "offset +
> > be16_to_cpu(dup->length) > end" for dup and "offset +
> > xfs_dir2_data_entsize(mp, dep->namelen) > end" for dep.
> > “xfs_dir2_data_entsize(mp, dep->namelen)” ensures the alignment of the
> > dep.
>
> Sure, but go back and read what I said.
>
> Detect the actual object corruption, not the downstream symptom.
>
> IOWs, the verifier should be detecting the exact corruption you
> induced.
>
> Catching all the object corruptions prevents a buffer overrun.
> We abort processing before we move beyond the end of the buffer.
>
> IOWs, we need to:
>
> 1. verify dup->length is a multiple of XFS_DIR2_DATA_ALIGN; and
> 2. verify 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.
>
> If either of these checks fail, then the block is corrupt.
> #1 will catch your induced corruption and fail immediately.
> #2 will catch the runt entry in the structure without derefencing
> past the end of the structure.
>
> Can you now see how properly validating that the objects within the
> structure will prevent buffer overruns from occurring without
> needing generic buffer overrun checks?
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com

[PATCH] xfs: add bounds checking to xlog_recover_process_data

[Kuntal Nayak]

From: lei lu <llfamsec@gmail.com>

[ Upstream commit fb63435b7c7dc112b1ae1baea5486e0a6e27b196 ]

There is a lack of verification of the space occupied by fixed members
of xlog_op_header in the xlog_recover_process_data.

We can create a crafted image to trigger an out of bounds read by
following these steps:
    1) Mount an image of xfs, and do some file operations to leave records
    2) Before umounting, copy the image for subsequent steps to simulate
       abnormal exit. Because umount will ensure that tail_blk and
       head_blk are the same, which will result in the inability to enter
       xlog_recover_process_data
    3) Write a tool to parse and modify the copied image in step 2
    4) Make the end of the xlog_op_header entries only 1 byte away from
       xlog_rec_header->h_size
    5) xlog_rec_header->h_num_logops++
    6) Modify xlog_rec_header->h_crc

Fix:
Add a check to make sure there is sufficient space to access fixed members
of xlog_op_header.

Signed-off-by: lei lu <llfamsec@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
Signed-off-by: Kuntal Nayak <kuntal.nayak@broadcom.com>
---
 fs/xfs/xfs_log_recover.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e61f28ce3..eafe76f30 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2419,7 +2419,10 @@ xlog_recover_process_data(
 
 		ohead = (struct xlog_op_header *)dp;
 		dp += sizeof(*ohead);
-		ASSERT(dp <= end);
+		if (dp > end) {
+			xfs_warn(log->l_mp, "%s: op header overrun", __func__);
+			return -EFSCORRUPTED;
+		}
 
 		/* errors will abort recovery */
 		error = xlog_recover_process_ophdr(log, rhash, rhead, ohead,
-- 
2.39.3

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

[Kuntal Nayak]

From: lei lu <llfamsec@gmail.com>

[ Upstream commit 0c7fcdb6d06cdf8b19b57c17605215b06afa864a ]

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 make sure that the remaining bytes large enough to hold
an unused entry before accessing xfs_dir2_data_unused and
xfs_dir2_data_unused is XFS_DIR2_DATA_ALIGN byte aligned. We also make
sure that the remaining bytes large enough to hold a dirent with a
single-byte name before accessing xfs_dir2_data_entry.

Signed-off-by: lei lu <llfamsec@gmail.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
Signed-off-by: Kuntal Nayak <kuntal.nayak@broadcom.com>
---
 fs/xfs/libxfs/xfs_dir2_data.c | 31 ++++++++++++++++++++++++++-----
 fs/xfs/libxfs/xfs_dir2_priv.h |  7 +++++++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index e67fa086f..72e4c3678 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -177,6 +177,14 @@ __xfs_dir3_data_check(
 	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.
@@ -186,9 +194,13 @@ __xfs_dir3_data_check(
 		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 (offset + be16_to_cpu(dup->length) > end)
+			if (be16_to_cpu(dup->length) != reclen)
+				return __this_address;
+			if (offset + reclen > end)
 				return __this_address;
 			if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
 			    offset)
@@ -206,10 +218,18 @@ __xfs_dir3_data_check(
 				    be16_to_cpu(bf[2].length))
 					return __this_address;
 			}
-			offset += be16_to_cpu(dup->length);
+			offset += reclen;
 			lastfree = 1;
 			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
@@ -218,9 +238,10 @@ __xfs_dir3_data_check(
 		 */
 		if (dep->namelen == 0)
 			return __this_address;
-		if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
+		reclen = xfs_dir2_data_entsize(mp, dep->namelen);
+		if (offset + reclen > end)
 			return __this_address;
-		if (offset + xfs_dir2_data_entsize(mp, dep->namelen) > end)
+		if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
 			return __this_address;
 		if (be16_to_cpu(*xfs_dir2_data_entry_tag_p(mp, dep)) != offset)
 			return __this_address;
@@ -244,7 +265,7 @@ __xfs_dir3_data_check(
 			if (i >= be32_to_cpu(btp->count))
 				return __this_address;
 		}
-		offset += xfs_dir2_data_entsize(mp, dep->namelen);
+		offset += reclen;
 	}
 	/*
 	 * Need to have seen all the entries and all the bestfree slots.
diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
index 44c6a77cb..e46063cde 100644
--- a/fs/xfs/libxfs/xfs_dir2_priv.h
+++ b/fs/xfs/libxfs/xfs_dir2_priv.h
@@ -186,6 +186,13 @@ void xfs_dir2_sf_put_ftype(struct xfs_mount *mp,
 extern int xfs_readdir(struct xfs_trans *tp, struct xfs_inode *dp,
 		       struct dir_context *ctx, size_t bufsize);
 
+static inline unsigned int
+xfs_dir2_data_unusedsize(
+	unsigned int	len)
+{
+	return round_up(len, XFS_DIR2_DATA_ALIGN);
+}
+
 static inline unsigned int
 xfs_dir2_data_entsize(
 	struct xfs_mount	*mp,
-- 
2.39.3