[edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements

["Savva Mitrofanov" <savvamtr@gmail.com>]

This changes tends to improve security of code sections by fixing
integer overflows, missing aligment checks, unsafe casts, also
simplified some routines, fixed compiler warnings and corrected some
code mistakes.

- Set HoleLen to UINT64 to perform safe cast to UINTN in ternary
operator at WasRead assignment.
- Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because
we consider BlockMap is 32-bit fs ext2/3 feature.
- Replace UNREACHABLE with ASSERT (FALSE) in case of new checksum
algorithms, due to it is an invariant violation rather than unreachable
path.
- Solve compiler warnings. Init all fields in gExt4BindingProtocol.
Fix comparison of integer expressions of different signedness.
- Field name_len has type CHAR8, while filename limit is 255
(EXT4_NAME_MAX), so because structure EXT4_DIR_ENTRY would be
unchangeable in future, we could drop this check without any
assertions
- Simplify Ext4RemoveDentry logic by using IsNodeInList
- Fix possible int overflow in Ext4ExtentsMapKeyCompare
- Return bad block type in Ext4GetBlockpath
- Adds 4-byte aligned check for superblock group descriptor size field

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |  3 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  2 +-
 Features/Ext4Pkg/Ext4Dxe/BlockMap.c   | 18 ++++++++----
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 29 ++------------------
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 10 ++++---
 Features/Ext4Pkg/Ext4Dxe/Extents.c    |  5 ++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |  8 +++---
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 13 +++++----
 8 files changed, 38 insertions(+), 50 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index a55cd2fa68ad..7a19d2f79d53 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -338,7 +338,7 @@ STATIC_ASSERT (
 #define EXT4_TIND_BLOCK  14
 #define EXT4_NR_BLOCKS   15
 
-#define EXT4_GOOD_OLD_INODE_SIZE  128
+#define EXT4_GOOD_OLD_INODE_SIZE  128U
 
 typedef struct _Ext4_I_OSD2_Linux {
   UINT16    l_i_blocks_high;
@@ -463,6 +463,7 @@ typedef struct {
 #define EXT4_EXTENT_MAX_INITIALIZED  (1 << 15)
 
 typedef UINT64  EXT4_BLOCK_NR;
+typedef UINT32  EXT2_BLOCK_NR;
 typedef UINT32  EXT4_INO_NR;
 
 // 2 is always the root inode number in ext4
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index b1508482b0a7..b446488b2112 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1165,7 +1165,7 @@ EFI_STATUS
 Ext4GetBlocks (
   IN  EXT4_PARTITION  *Partition,
   IN  EXT4_FILE       *File,
-  IN  EXT4_BLOCK_NR   LogicalBlock,
+  IN  EXT2_BLOCK_NR   LogicalBlock,
   OUT EXT4_EXTENT     *Extent
   );
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
index 1a06ac9fbf86..2bc629fe9d38 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
+++ b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
@@ -70,7 +70,7 @@ UINTN
 Ext4GetBlockPath (
   IN  CONST EXT4_PARTITION  *Partition,
   IN  UINT32                LogicalBlock,
-  OUT EXT4_BLOCK_NR         BlockPath[EXT4_MAX_BLOCK_PATH]
+  OUT EXT2_BLOCK_NR         BlockPath[EXT4_MAX_BLOCK_PATH]
   )
 {
   // The logic behind the block map is very much like a page table
@@ -123,7 +123,7 @@ Ext4GetBlockPath (
       break;
     default:
       // EXT4_TYPE_BAD_BLOCK
-      return -1;
+      break;
   }
 
   return Type + 1;
@@ -213,12 +213,12 @@ EFI_STATUS
 Ext4GetBlocks (
   IN  EXT4_PARTITION  *Partition,
   IN  EXT4_FILE       *File,
-  IN  EXT4_BLOCK_NR   LogicalBlock,
+  IN  EXT2_BLOCK_NR   LogicalBlock,
   OUT EXT4_EXTENT     *Extent
   )
 {
   EXT4_INODE     *Inode;
-  EXT4_BLOCK_NR  BlockPath[EXT4_MAX_BLOCK_PATH];
+  EXT2_BLOCK_NR  BlockPath[EXT4_MAX_BLOCK_PATH];
   UINTN          BlockPathLength;
   UINTN          Index;
   UINT32         *Buffer;
@@ -230,7 +230,7 @@ Ext4GetBlocks (
 
   BlockPathLength = Ext4GetBlockPath (Partition, LogicalBlock, BlockPath);
 
-  if (BlockPathLength == (UINTN)-1) {
+  if (BlockPathLength - 1 == EXT4_TYPE_BAD_BLOCK) {
     // Bad logical block (out of range)
     return EFI_NO_MAPPING;
   }
@@ -272,7 +272,13 @@ Ext4GetBlocks (
     }
   }
 
-  Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof (UINT32), BlockPath[BlockPathLength - 1], Extent);
+  Ext4GetExtentInBlockMap (
+    Buffer,
+    Partition->BlockSize / sizeof (UINT32),
+    BlockPath[BlockPathLength - 1],
+    Extent
+    );
+
   FreePool (Buffer);
 
   return EFI_SUCCESS;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 682f66ad5525..4441e6d192b6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -74,7 +74,7 @@ Ext4ValidDirent (
   }
 
   // Dirent sizes need to be 4 byte aligned
-  if (Dirent->rec_len % 4) {
+  if ((Dirent->rec_len % 4) != 0) {
     return FALSE;
   }
 
@@ -160,17 +160,6 @@ Ext4RetrieveDirent (
         return EFI_VOLUME_CORRUPTED;
       }
 
-      // Ignore names bigger than our limit.
-
-      /* Note: I think having a limit is sane because:
-        1) It's nicer to work with.
-        2) Linux and a number of BSDs also have a filename limit of 255.
-      */
-      if (Entry->name_len > EXT4_NAME_MAX) {
-        BlockOffset += Entry->rec_len;
-        continue;
-      }
-
       // Unused entry
       if (Entry->inode == 0) {
         BlockOffset += Entry->rec_len;
@@ -548,20 +537,8 @@ Ext4RemoveDentry (
   IN OUT EXT4_DENTRY  *ToBeRemoved
   )
 {
-  EXT4_DENTRY  *D;
-  LIST_ENTRY   *Entry;
-  LIST_ENTRY   *NextEntry;
-
-  BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {
-    D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
-
-    if (D == ToBeRemoved) {
-      RemoveEntryList (Entry);
-      return;
-    }
-  }
-
-  DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for dentry\n"));
+  ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent->Children));
+  RemoveEntryList (&ToBeRemoved->ListNode);
 }
 
 /**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index 43b9340d3956..2a4f5a7bd0ef 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -260,10 +260,12 @@ Ext4Stop (
 
 EFI_DRIVER_BINDING_PROTOCOL  gExt4BindingProtocol =
 {
-  Ext4IsBindingSupported,
-  Ext4Bind,
-  Ext4Stop,
-  EXT4_DRIVER_VERSION
+  .Supported           = Ext4IsBindingSupported,
+  .Start               = Ext4Bind,
+  .Stop                = Ext4Stop,
+  .Version             = EXT4_DRIVER_VERSION,
+  .ImageHandle         = NULL,
+  .DriverBindingHandle = NULL
 };
 
 /**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index c3874df71751..d9ff69f21c14 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
@@ -259,7 +259,8 @@ Ext4GetExtent (
 
   if (!(Inode->i_flags & EXT4_EXTENTS_FL)) {
     // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent using the block map
-    Status = Ext4GetBlocks (Partition, File, LogicalBlock, Extent);
+    // We cast LogicalBlock to UINT32, considering ext2/3 are 32-bit
+    Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Extent);
 
     if (!EFI_ERROR (Status)) {
       Ext4CacheExtents (File, Extent, 1);
@@ -420,7 +421,7 @@ Ext4ExtentsMapKeyCompare (
   Extent = UserStruct;
   Block  = (UINT32)(UINTN)StandaloneKey;
 
-  if ((Block >= Extent->ee_block) && (Block < Extent->ee_block + Ext4GetExtentLength (Extent))) {
+  if ((Block >= Extent->ee_block) && (Block - Extent->ee_block < Ext4GetExtentLength (Extent))) {
     return 0;
   }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 831f5946e870..4860cf576377 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -100,7 +100,7 @@ Ext4Read (
   EFI_STATUS   Status;
   BOOLEAN      HasBackingExtent;
   UINT32       HoleOff;
-  UINTN        HoleLen;
+  UINT64       HoleLen;
   UINT64       ExtentStartBytes;
   UINT64       ExtentLengthBytes;
   UINT64       ExtentLogicalBytes;
@@ -155,10 +155,10 @@ Ext4Read (
         HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) - HoleOff;
       }
 
-      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
+      WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen;
       // Potential improvement: In the future, we could get the file hole's total
       // size and memset all that
-      SetMem (Buffer, WasRead, 0);
+      ZeroMem (Buffer, WasRead);
     } else {
       ExtentStartBytes = MultU64x32 (
                            LShiftU64 (Extent.ee_start_hi, 32) |
@@ -431,7 +431,7 @@ Ext4FileCreateTime (
   Inode = File->Inode;
 
   if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) {
-    SetMem (Time, sizeof (EFI_TIME), 0);
+    ZeroMem (Time, sizeof (EFI_TIME));
     return;
   }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 47fc3a65507a..a57728a9abe6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -257,16 +257,17 @@ Ext4OpenSuperblock (
     ));
 
   if (EXT4_IS_64_BIT (Partition)) {
+    // s_desc_size should be 4 byte aligned and
+    // 64 bit filesystems need DescSize to be 64 bytes
+    if (((Sb->s_desc_size % 4) != 0) || (Sb->s_desc_size < EXT4_64BIT_BLOCK_DESC_SIZE)) {
+      return EFI_VOLUME_CORRUPTED;
+    }
+
     Partition->DescSize = Sb->s_desc_size;
   } else {
     Partition->DescSize = EXT4_OLD_BLOCK_DESC_SIZE;
   }
 
-  if ((Partition->DescSize < EXT4_64BIT_BLOCK_DESC_SIZE) && EXT4_IS_64_BIT (Partition)) {
-    // 64 bit filesystems need DescSize to be 64 bytes
-    return EFI_VOLUME_CORRUPTED;
-  }
-
   if (!Ext4VerifySuperblockChecksum (Partition, Sb)) {
     DEBUG ((DEBUG_ERROR, "[ext4] Bad superblock checksum %lx\n", Ext4CalculateSuperblockChecksum (Partition, Sb)));
     return EFI_VOLUME_CORRUPTED;
@@ -342,7 +343,7 @@ Ext4CalculateChecksum (
       // For some reason, EXT4 really likes non-inverted CRC32C checksums, so we stick to that here.
       return ~CalculateCrc32c(Buffer, Length, ~InitialValue);
     default:
-      UNREACHABLE ();
+      ASSERT (FALSE);
       return 0;
   }
 }
-- 
2.37.0