* [PATCH v3] xfs: don't walk off the end of a directory data block
@ 2024-06-06 3:14 lei lu
2024-06-06 16:31 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: lei lu @ 2024-06-06 3:14 UTC (permalink / raw)
To: linux-xfs; +Cc: david, lei lu, Dave Chinner
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@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
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;
+ }
+
/*
* 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] xfs: don't walk off the end of a directory data block
2024-06-06 3:14 [PATCH v3] xfs: don't walk off the end of a directory data block lei lu
@ 2024-06-06 16:31 ` Darrick J. Wong
2024-06-07 3:25 ` lei lu
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2024-06-06 16:31 UTC (permalink / raw)
To: lei lu; +Cc: linux-xfs, david, Dave Chinner
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@gmail.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] xfs: don't walk off the end of a directory data block
2024-06-06 16:31 ` Darrick J. Wong
@ 2024-06-07 3:25 ` lei lu
0 siblings, 0 replies; 3+ messages in thread
From: lei lu @ 2024-06-07 3:25 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david, Dave Chinner
On Fri, Jun 7, 2024 at 12:31 AM Darrick J. Wong <djwong@kernel.org> 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@gmail.com>
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > 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
> >
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-07 3:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-06 3:14 [PATCH v3] xfs: don't walk off the end of a directory data block lei lu
2024-06-06 16:31 ` Darrick J. Wong
2024-06-07 3:25 ` lei lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox