* [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries
@ 2023-05-09 15:49 Pedro Falcato
2023-05-09 15:49 ` [PATCH v3 2/2] Ext4Pkg: Advertise CSUM_SEED as supported Pedro Falcato
2023-05-09 16:07 ` [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Marvin Häuser
0 siblings, 2 replies; 4+ messages in thread
From: Pedro Falcato @ 2023-05-09 15:49 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Marvin Häuser, Savva Mitrofanov
Improve the extent tree node validation by validating the number of
entries the node advertises against the theoretical max (derived from
the size of on-disk structs and the block size (or i_data, if inline
extents).
Previously, we did not validate the number of entries. This could be
exploited for out-of-bounds reads and crashes.
Cc: Marvin Häuser <mhaeuser@posteo.de>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Reported-by: Savva Mitrofanov <savvamtr@gmail.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
Features/Ext4Pkg/Ext4Dxe/Extents.c | 32 ++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index 9e4364e50d99..2b82417c9e10 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
@@ -1,7 +1,7 @@
/** @file
Extent related routines
- Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
+ Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -80,13 +80,15 @@ Ext4GetInoExtentHeader (
/**
Checks if an extent header is valid.
@param[in] Header Pointer to the EXT4_EXTENT_HEADER structure.
+ @param[in] MaxEntries Maximum number of entries possible for this tree node.
@return TRUE if valid, FALSE if not.
**/
STATIC
BOOLEAN
Ext4ExtentHeaderValid (
- IN CONST EXT4_EXTENT_HEADER *Header
+ IN CONST EXT4_EXTENT_HEADER *Header,
+ IN UINT16 MaxEntries
)
{
if (Header->eh_depth > EXT4_EXTENT_TREE_MAX_DEPTH) {
@@ -99,6 +101,18 @@ Ext4ExtentHeaderValid (
return FALSE;
}
+ // Note: We do not need to check eh_entries here, as the next branch makes sure max >= entries
+ if (Header->eh_max > MaxEntries) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "[ext4] Invalid extent header max entries (%u eh_max, "
+ "theoretical max is %u) (larger than permitted)\n",
+ Header->eh_max,
+ MaxEntries
+ ));
+ return FALSE;
+ }
+
if (Header->eh_max < Header->eh_entries) {
DEBUG ((
DEBUG_ERROR,
@@ -212,6 +226,9 @@ Ext4ExtentIdxLeafBlock (
return LShiftU64 (Index->ei_leaf_hi, 32) | Index->ei_leaf_lo;
}
+// Results of sizeof(i_data) / sizeof(extent) - 1 = 4
+#define EXT4_NR_INLINE_EXTENTS 4
+
/**
Retrieves an extent from an EXT4 inode.
@param[in] Partition Pointer to the opened EXT4 partition.
@@ -237,6 +254,7 @@ Ext4GetExtent (
EXT4_EXTENT_HEADER *ExtHeader;
EXT4_EXTENT_INDEX *Index;
EFI_STATUS Status;
+ UINT32 MaxExtentsPerNode;
EXT4_BLOCK_NR BlockNumber;
Inode = File->Inode;
@@ -275,12 +293,17 @@ Ext4GetExtent (
ExtHeader = Ext4GetInoExtentHeader (Inode);
- if (!Ext4ExtentHeaderValid (ExtHeader)) {
+ if (!Ext4ExtentHeaderValid (ExtHeader, EXT4_NR_INLINE_EXTENTS)) {
return EFI_VOLUME_CORRUPTED;
}
CurrentDepth = ExtHeader->eh_depth;
+ // A single node fits into a single block, so we can only have (BlockSize / sizeof(EXT4_EXTENT)) - 1
+ // extents in a single node. Note the -1, because both leaf and internal node headers are 12 bytes,
+ // and so are individual entries.
+ MaxExtentsPerNode = (Partition->BlockSize / sizeof (EXT4_EXTENT)) - 1;
+
while (ExtHeader->eh_depth != 0) {
CurrentDepth--;
// While depth != 0, we're traversing the tree itself and not any leaves
@@ -309,6 +332,7 @@ Ext4GetExtent (
}
// Read the leaf block onto the previously-allocated buffer.
+
Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber);
if (EFI_ERROR (Status)) {
FreePool (Buffer);
@@ -317,7 +341,7 @@ Ext4GetExtent (
ExtHeader = Buffer;
- if (!Ext4ExtentHeaderValid (ExtHeader)) {
+ if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) {
FreePool (Buffer);
return EFI_VOLUME_CORRUPTED;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v3 2/2] Ext4Pkg: Advertise CSUM_SEED as supported
2023-05-09 15:49 [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Pedro Falcato
@ 2023-05-09 15:49 ` Pedro Falcato
2023-05-09 16:07 ` [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Marvin Häuser
1 sibling, 0 replies; 4+ messages in thread
From: Pedro Falcato @ 2023-05-09 15:49 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Marvin Häuser
We had added support for CSUM_SEED but accidentally forgot to advertise
it in gSupportedIncompatFeat. This made it (erroneously) impossible to
mount CSUM_SEED filesystems.
Detected by attempting to mount a relatively new mkfs.ext4'd filesystem.
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>
---
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 3f56de93c105..604925b582c1 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -1,7 +1,7 @@
/** @file
Superblock managing routines
- Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
+ Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -18,7 +18,7 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
EXT4_FEATURE_INCOMPAT_64BIT | EXT4_FEATURE_INCOMPAT_DIRDATA |
EXT4_FEATURE_INCOMPAT_FLEX_BG | EXT4_FEATURE_INCOMPAT_FILETYPE |
EXT4_FEATURE_INCOMPAT_EXTENTS | EXT4_FEATURE_INCOMPAT_LARGEDIR |
- EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER;
+ EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER | EXT4_FEATURE_INCOMPAT_CSUM_SEED;
// Future features that may be nice additions in the future:
// 1) Btree support: Required for write support and would speed up lookups in large directories.
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries
2023-05-09 15:49 [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Pedro Falcato
2023-05-09 15:49 ` [PATCH v3 2/2] Ext4Pkg: Advertise CSUM_SEED as supported Pedro Falcato
@ 2023-05-09 16:07 ` Marvin Häuser
2023-05-09 16:50 ` Pedro Falcato
1 sibling, 1 reply; 4+ messages in thread
From: Marvin Häuser @ 2023-05-09 16:07 UTC (permalink / raw)
To: Pedro Falcato; +Cc: devel, Savva Mitrofanov
Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>
> On 9. May 2023, at 17:49, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Improve the extent tree node validation by validating the number of
> entries the node advertises against the theoretical max (derived from
> the size of on-disk structs and the block size (or i_data, if inline
> extents).
>
> Previously, we did not validate the number of entries. This could be
> exploited for out-of-bounds reads and crashes.
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> Reported-by: Savva Mitrofanov <savvamtr@gmail.com>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Extents.c | 32 ++++++++++++++++++++++++++----
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> index 9e4364e50d99..2b82417c9e10 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> @@ -1,7 +1,7 @@
> /** @file
> Extent related routines
>
> - Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
> + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
>
> @@ -80,13 +80,15 @@ Ext4GetInoExtentHeader (
> /**
> Checks if an extent header is valid.
> @param[in] Header Pointer to the EXT4_EXTENT_HEADER structure.
> + @param[in] MaxEntries Maximum number of entries possible for this tree node.
>
> @return TRUE if valid, FALSE if not.
> **/
> STATIC
> BOOLEAN
> Ext4ExtentHeaderValid (
> - IN CONST EXT4_EXTENT_HEADER *Header
> + IN CONST EXT4_EXTENT_HEADER *Header,
> + IN UINT16 MaxEntries
> )
> {
> if (Header->eh_depth > EXT4_EXTENT_TREE_MAX_DEPTH) {
> @@ -99,6 +101,18 @@ Ext4ExtentHeaderValid (
> return FALSE;
> }
>
> + // Note: We do not need to check eh_entries here, as the next branch makes sure max >= entries
> + if (Header->eh_max > MaxEntries) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "[ext4] Invalid extent header max entries (%u eh_max, "
> + "theoretical max is %u) (larger than permitted)\n",
> + Header->eh_max,
> + MaxEntries
> + ));
> + return FALSE;
> + }
> +
> if (Header->eh_max < Header->eh_entries) {
> DEBUG ((
> DEBUG_ERROR,
> @@ -212,6 +226,9 @@ Ext4ExtentIdxLeafBlock (
> return LShiftU64 (Index->ei_leaf_hi, 32) | Index->ei_leaf_lo;
> }
>
> +// Results of sizeof(i_data) / sizeof(extent) - 1 = 4
> +#define EXT4_NR_INLINE_EXTENTS 4
> +
> /**
> Retrieves an extent from an EXT4 inode.
> @param[in] Partition Pointer to the opened EXT4 partition.
> @@ -237,6 +254,7 @@ Ext4GetExtent (
> EXT4_EXTENT_HEADER *ExtHeader;
> EXT4_EXTENT_INDEX *Index;
> EFI_STATUS Status;
> + UINT32 MaxExtentsPerNode;
> EXT4_BLOCK_NR BlockNumber;
>
> Inode = File->Inode;
> @@ -275,12 +293,17 @@ Ext4GetExtent (
>
> ExtHeader = Ext4GetInoExtentHeader (Inode);
>
> - if (!Ext4ExtentHeaderValid (ExtHeader)) {
> + if (!Ext4ExtentHeaderValid (ExtHeader, EXT4_NR_INLINE_EXTENTS)) {
> return EFI_VOLUME_CORRUPTED;
> }
>
> CurrentDepth = ExtHeader->eh_depth;
>
> + // A single node fits into a single block, so we can only have (BlockSize / sizeof(EXT4_EXTENT)) - 1
> + // extents in a single node. Note the -1, because both leaf and internal node headers are 12 bytes,
> + // and so are individual entries.
> + MaxExtentsPerNode = (Partition->BlockSize / sizeof (EXT4_EXTENT)) - 1;
> +
> while (ExtHeader->eh_depth != 0) {
> CurrentDepth--;
> // While depth != 0, we're traversing the tree itself and not any leaves
> @@ -309,6 +332,7 @@ Ext4GetExtent (
> }
>
> // Read the leaf block onto the previously-allocated buffer.
> +
> Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber);
> if (EFI_ERROR (Status)) {
> FreePool (Buffer);
> @@ -317,7 +341,7 @@ Ext4GetExtent (
>
> ExtHeader = Buffer;
>
> - if (!Ext4ExtentHeaderValid (ExtHeader)) {
> + if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) {
> FreePool (Buffer);
> return EFI_VOLUME_CORRUPTED;
> }
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries
2023-05-09 16:07 ` [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Marvin Häuser
@ 2023-05-09 16:50 ` Pedro Falcato
0 siblings, 0 replies; 4+ messages in thread
From: Pedro Falcato @ 2023-05-09 16:50 UTC (permalink / raw)
To: Marvin Häuser; +Cc: devel, Savva Mitrofanov
Thank you! Merged the patchset as 82f4b39 and 9247980, respectively.
On Tue, May 9, 2023 at 5:07 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>
>
> > On 9. May 2023, at 17:49, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > Improve the extent tree node validation by validating the number of
> > entries the node advertises against the theoretical max (derived from
> > the size of on-disk structs and the block size (or i_data, if inline
> > extents).
> >
> > Previously, we did not validate the number of entries. This could be
> > exploited for out-of-bounds reads and crashes.
> >
> > Cc: Marvin Häuser <mhaeuser@posteo.de>
> > Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> > Reported-by: Savva Mitrofanov <savvamtr@gmail.com>
> > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > ---
> > Features/Ext4Pkg/Ext4Dxe/Extents.c | 32 ++++++++++++++++++++++++++----
> > 1 file changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> > index 9e4364e50d99..2b82417c9e10 100644
> > --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> > +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > Extent related routines
> >
> > - Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved.
> > + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> > **/
> >
> > @@ -80,13 +80,15 @@ Ext4GetInoExtentHeader (
> > /**
> > Checks if an extent header is valid.
> > @param[in] Header Pointer to the EXT4_EXTENT_HEADER structure.
> > + @param[in] MaxEntries Maximum number of entries possible for this tree node.
> >
> > @return TRUE if valid, FALSE if not.
> > **/
> > STATIC
> > BOOLEAN
> > Ext4ExtentHeaderValid (
> > - IN CONST EXT4_EXTENT_HEADER *Header
> > + IN CONST EXT4_EXTENT_HEADER *Header,
> > + IN UINT16 MaxEntries
> > )
> > {
> > if (Header->eh_depth > EXT4_EXTENT_TREE_MAX_DEPTH) {
> > @@ -99,6 +101,18 @@ Ext4ExtentHeaderValid (
> > return FALSE;
> > }
> >
> > + // Note: We do not need to check eh_entries here, as the next branch makes sure max >= entries
> > + if (Header->eh_max > MaxEntries) {
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "[ext4] Invalid extent header max entries (%u eh_max, "
> > + "theoretical max is %u) (larger than permitted)\n",
> > + Header->eh_max,
> > + MaxEntries
> > + ));
> > + return FALSE;
> > + }
> > +
> > if (Header->eh_max < Header->eh_entries) {
> > DEBUG ((
> > DEBUG_ERROR,
> > @@ -212,6 +226,9 @@ Ext4ExtentIdxLeafBlock (
> > return LShiftU64 (Index->ei_leaf_hi, 32) | Index->ei_leaf_lo;
> > }
> >
> > +// Results of sizeof(i_data) / sizeof(extent) - 1 = 4
> > +#define EXT4_NR_INLINE_EXTENTS 4
> > +
> > /**
> > Retrieves an extent from an EXT4 inode.
> > @param[in] Partition Pointer to the opened EXT4 partition.
> > @@ -237,6 +254,7 @@ Ext4GetExtent (
> > EXT4_EXTENT_HEADER *ExtHeader;
> > EXT4_EXTENT_INDEX *Index;
> > EFI_STATUS Status;
> > + UINT32 MaxExtentsPerNode;
> > EXT4_BLOCK_NR BlockNumber;
> >
> > Inode = File->Inode;
> > @@ -275,12 +293,17 @@ Ext4GetExtent (
> >
> > ExtHeader = Ext4GetInoExtentHeader (Inode);
> >
> > - if (!Ext4ExtentHeaderValid (ExtHeader)) {
> > + if (!Ext4ExtentHeaderValid (ExtHeader, EXT4_NR_INLINE_EXTENTS)) {
> > return EFI_VOLUME_CORRUPTED;
> > }
> >
> > CurrentDepth = ExtHeader->eh_depth;
> >
> > + // A single node fits into a single block, so we can only have (BlockSize / sizeof(EXT4_EXTENT)) - 1
> > + // extents in a single node. Note the -1, because both leaf and internal node headers are 12 bytes,
> > + // and so are individual entries.
> > + MaxExtentsPerNode = (Partition->BlockSize / sizeof (EXT4_EXTENT)) - 1;
> > +
> > while (ExtHeader->eh_depth != 0) {
> > CurrentDepth--;
> > // While depth != 0, we're traversing the tree itself and not any leaves
> > @@ -309,6 +332,7 @@ Ext4GetExtent (
> > }
> >
> > // Read the leaf block onto the previously-allocated buffer.
> > +
> > Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber);
> > if (EFI_ERROR (Status)) {
> > FreePool (Buffer);
> > @@ -317,7 +341,7 @@ Ext4GetExtent (
> >
> > ExtHeader = Buffer;
> >
> > - if (!Ext4ExtentHeaderValid (ExtHeader)) {
> > + if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) {
> > FreePool (Buffer);
> > return EFI_VOLUME_CORRUPTED;
> > }
> > --
> > 2.40.1
> >
>
--
Pedro
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-09 16:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-09 15:49 [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Pedro Falcato
2023-05-09 15:49 ` [PATCH v3 2/2] Ext4Pkg: Advertise CSUM_SEED as supported Pedro Falcato
2023-05-09 16:07 ` [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries Marvin Häuser
2023-05-09 16:50 ` Pedro Falcato
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox