* [PATCH] xfs: don't walk off the end of a directory data block
@ 2024-05-29 22:57 lei lu
2024-05-30 2:38 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: lei lu @ 2024-05-29 22:57 UTC (permalink / raw)
To: djwong, linux-xfs; +Cc: chandan.babu, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: don't walk off the end of a directory data block
2024-05-29 22:57 [PATCH] xfs: don't walk off the end of a directory data block lei lu
@ 2024-05-30 2:38 ` Dave Chinner
2024-05-30 3:10 ` lei lu
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2024-05-30 2:38 UTC (permalink / raw)
To: lei lu; +Cc: djwong, linux-xfs, chandan.babu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: don't walk off the end of a directory data block
2024-05-30 2:38 ` Dave Chinner
@ 2024-05-30 3:10 ` lei lu
2024-06-03 5:58 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: lei lu @ 2024-05-30 3:10 UTC (permalink / raw)
To: Dave Chinner; +Cc: djwong, linux-xfs, chandan.babu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: don't walk off the end of a directory data block
2024-05-30 3:10 ` lei lu
@ 2024-06-03 5:58 ` Dave Chinner
2024-06-03 7:08 ` lei lu
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2024-06-03 5:58 UTC (permalink / raw)
To: lei lu; +Cc: djwong, linux-xfs, chandan.babu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: don't walk off the end of a directory data block
2024-06-03 5:58 ` Dave Chinner
@ 2024-06-03 7:08 ` lei lu
0 siblings, 0 replies; 6+ messages in thread
From: lei lu @ 2024-06-03 7:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: djwong, linux-xfs, chandan.babu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] xfs: don't walk off the end of a directory data block
2024-09-24 22:29 [PATCH] xfs: add bounds checking to xlog_recover_process_data Kuntal Nayak
@ 2024-09-24 22:29 ` Kuntal Nayak
0 siblings, 0 replies; 6+ messages in thread
From: Kuntal Nayak @ 2024-09-24 22:29 UTC (permalink / raw)
To: leah.rumancik, jwong, linux-xfs, linux-kernel, gregkh
Cc: ajay.kaher, alexey.makhalov, vasavi.sirnapalli, lei lu,
Darrick J . Wong, Chandan Babu R, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-24 22:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-29 22:57 [PATCH] xfs: don't walk off the end of a directory data block lei lu
2024-05-30 2:38 ` Dave Chinner
2024-05-30 3:10 ` lei lu
2024-06-03 5:58 ` Dave Chinner
2024-06-03 7:08 ` lei lu
-- strict thread matches above, loose matches on Subject: below --
2024-09-24 22:29 [PATCH] xfs: add bounds checking to xlog_recover_process_data Kuntal Nayak
2024-09-24 22:29 ` [PATCH] xfs: don't walk off the end of a directory data block Kuntal Nayak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox