public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 1/2] Ext4Pkg: Improve extent node validation on the number of entries
@ 2023-05-09  0:42 Pedro Falcato
  2023-05-09  0:42 ` [PATCH 2/2] Ext4Pkg: Advertise CSUM_SEED as supported Pedro Falcato
  2023-05-09  7:05 ` [PATCH 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  0:42 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 | 47 +++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index 9e4364e50d99..66d085938549 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;
   }
 
+  if ((Header->eh_max > MaxEntries) || (Header->eh_entries > MaxEntries)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "[ext4] Invalid extent header entries (extent header: %u max,"
+      " %u entries, theoretical max: %u) (larger than permitted)\n",
+      Header->eh_max,
+      Header->eh_entries,
+      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,7 +254,7 @@ Ext4GetExtent (
   EXT4_EXTENT_HEADER  *ExtHeader;
   EXT4_EXTENT_INDEX   *Index;
   EFI_STATUS          Status;
-  EXT4_BLOCK_NR       BlockNumber;
+  UINT32              MaxExtentsPerNode;
 
   Inode  = File->Inode;
   Ext    = NULL;
@@ -275,12 +292,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
@@ -289,17 +311,7 @@ Ext4GetExtent (
     // Therefore, we can use binary search, and it's actually the standard for doing so
     // (see FreeBSD).
 
-    Index       = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock);
-    BlockNumber = Ext4ExtentIdxLeafBlock (Index);
-
-    // Check that block isn't file hole
-    if (BlockNumber == EXT4_BLOCK_FILE_HOLE) {
-      if (Buffer != NULL) {
-        FreePool (Buffer);
-      }
-
-      return EFI_VOLUME_CORRUPTED;
-    }
+    Index = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock);
 
     if (Buffer == NULL) {
       Buffer = AllocatePool (Partition->BlockSize);
@@ -309,7 +321,8 @@ Ext4GetExtent (
     }
 
     // Read the leaf block onto the previously-allocated buffer.
-    Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber);
+
+    Status = Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlock (Index));
     if (EFI_ERROR (Status)) {
       FreePool (Buffer);
       return Status;
@@ -317,7 +330,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
end of thread, other threads:[~2023-05-09  7:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-09  0:42 [PATCH 1/2] Ext4Pkg: Improve extent node validation on the number of entries Pedro Falcato
2023-05-09  0:42 ` [PATCH 2/2] Ext4Pkg: Advertise CSUM_SEED as supported Pedro Falcato
2023-05-09  7:06   ` Marvin Häuser
2023-05-09  7:05 ` [PATCH 1/2] Ext4Pkg: Improve extent node validation on the number of entries Marvin Häuser

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox