Ext4Pkg: Various improvements based on Sydr fuzzing results.

Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>

Ext4Pkg/Ext4Dxe/BlockGroup.c

@@ -50,6 +50,11 @@ Ext4ReadInode (
   EXT4_BLOCK_NR          InodeTableStart;
   EFI_STATUS             Status;
 
+  if (!EXT4_IS_VALID_INODE_NR (Partition, InodeNum)) {
+    DEBUG ((DEBUG_ERROR, "[ext4] Error reading inode: inode number %lu isn't valid\n", InodeNum));
+    return EFI_VOLUME_CORRUPTED;
+  }
+
   BlockGroupNumber = (UINT32)DivU64x64Remainder (
                                InodeNum - 1,
                                Partition->SuperBlock.s_inodes_per_group,
@@ -164,14 +169,10 @@ Ext4CalculateBlockGroupDescChecksumGdtCsum (
   )
 {
   UINT16  Csum;
-  UINT16  Dummy;
 
-  Dummy = 0;
+  Csum = CalculateCrc16Ansi (Partition->SuperBlock.s_uuid, sizeof (Partition->SuperBlock.s_uuid), 0xFFFF);
-
-  Csum = CalculateCrc16Ansi (Partition->SuperBlock.s_uuid, 16, 0);
   Csum = CalculateCrc16Ansi (&BlockGroupNum, sizeof (BlockGroupNum), Csum);
   Csum = CalculateCrc16Ansi (BlockGroupDesc, OFFSET_OF (EXT4_BLOCK_GROUP_DESC, bg_checksum), Csum);
-  Csum = CalculateCrc16Ansi (&Dummy, sizeof (Dummy), Csum);
   Csum =
     CalculateCrc16Ansi (
       &BlockGroupDesc->bg_block_bitmap_hi,

Ext4Pkg/Ext4Dxe/BlockMap.c

@@ -131,7 +131,7 @@ Ext4GetBlockPath (
 
 /**
    @brief Get an extent from a block map
-   Note: Also parses file holes and creates uninitialised extents from them.
+   Note: Also parses file holes and creates uninitialized extents from them.
 
    @param[in]  Buffer          Buffer of block pointers
    @param[in]  IndEntries      Number of entries in this block pointer table
@@ -173,7 +173,7 @@ Ext4GetExtentInBlockMap (
       }
     }
 
-    // We mark the extent as uninitialised, although there's a difference between uninit
+    // We mark the extent as uninitialized, although there's a difference between uninit
     // extents and file holes.
     Extent->ee_len = EXT4_EXTENT_MAX_INITIALIZED + Count;
     return;
@@ -206,7 +206,7 @@ Ext4GetExtentInBlockMap (
    @param[in]      LogicalBlock  Block number which the returned extent must cover.
    @param[out]     Extent        Pointer to the output buffer, where the extent will be copied to.
 
-   @retval EFI_SUCCESS        Retrieval was succesful.
+   @retval EFI_SUCCESS        Retrieval was successful.
    @retval EFI_NO_MAPPING     Block has no mapping.
 **/
 EFI_STATUS

Ext4Pkg/Ext4Dxe/Collation.c

@@ -1,7 +1,7 @@
 /** @file
   Unicode collation routines
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -9,6 +9,7 @@
 
 #include <Uefi.h>
 
+#include <Library/DebugLib.h>
 #include <Library/UefiLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/MemoryAllocationLib.h>
@@ -23,6 +24,21 @@ STATIC EFI_UNICODE_COLLATION_PROTOCOL  *gUnicodeCollationInterface = NULL;
  * PS: Maybe all this code could be put in a library? It looks heavily shareable.
 **/
 
+/**
+   Check if unicode collation is initialized
+
+   @retval TRUE if Ext4InitialiseUnicodeCollation() was already called successfully
+   @retval FALSE if Ext4InitialiseUnicodeCollation() was not yet called successfully
+**/
+STATIC
+BOOLEAN
+Ext4IsCollationInitialized (
+  VOID
+  )
+{
+  return gUnicodeCollationInterface != NULL;
+}
+
 /**
   Worker function to initialize Unicode Collation support.
 
@@ -127,6 +143,11 @@ Ext4InitialiseUnicodeCollation (
 
   Status = EFI_UNSUPPORTED;
 
+  // If already done, just return success.
+  if (Ext4IsCollationInitialized ()) {
+    return EFI_SUCCESS;
+  }
+
   //
   // First try to use RFC 4646 Unicode Collation 2 Protocol.
   //
@@ -169,5 +190,6 @@ Ext4StrCmpInsensitive (
   IN CHAR16  *Str2
   )
 {
+  ASSERT (gUnicodeCollationInterface != NULL);
   return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface, Str1, Str2);
 }

Ext4Pkg/Ext4Dxe/Directory.c

@@ -1,7 +1,7 @@
 /** @file
   Directory related routines
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -16,7 +16,8 @@
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
    @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   The filename was succesfully retrieved and converted to UCS2.
+   @retval EFI_SUCCESS              The filename was successfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
    @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
@@ -27,9 +28,16 @@ Ext4GetUcs2DirentName (
 {
   CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
   UINT16      *Str;
+  UINT8       Index;
   EFI_STATUS  Status;
 
-  CopyMem (Utf8NameBuf, Entry->name, Entry->name_len);
+  for (Index = 0; Index < Entry->name_len; ++Index) {
+    if (Entry->name[Index] == '\0') {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    Utf8NameBuf[Index] = Entry->name[Index];
+  }
 
   Utf8NameBuf[Entry->name_len] = '\0';
 
@@ -112,7 +120,6 @@ Ext4RetrieveDirent (
   UINTN           ToCopy;
   UINTN           BlockOffset;
 
-  Status = EFI_NOT_FOUND;
   Buf = AllocatePool (Partition->BlockSize);
 
   if (Buf == NULL) {
@@ -127,7 +134,8 @@ Ext4RetrieveDirent (
   DivU64x32Remainder (DirInoSize, Partition->BlockSize, &BlockRemainder);
   if (BlockRemainder != 0) {
     // Directory inodes need to have block aligned sizes
-    return EFI_VOLUME_CORRUPTED;
+    Status = EFI_VOLUME_CORRUPTED;
+    goto Out;
   }
 
   while (Off < DirInoSize) {
@@ -136,8 +144,7 @@ Ext4RetrieveDirent (
     Status = Ext4Read (Partition, Directory, Buf, Off, &Length);
 
     if (Status != EFI_SUCCESS) {
-      FreePool (Buf);
+      goto Out;
-      return Status;
     }
 
     for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) {
@@ -145,19 +152,19 @@ Ext4RetrieveDirent (
       RemainingBlock = Partition->BlockSize - BlockOffset;
       // Check if the minimum directory entry fits inside [BlockOffset, EndOfBlock]
       if (RemainingBlock < EXT4_MIN_DIR_ENTRY_LEN) {
-        FreePool (Buf);
+        Status = EFI_VOLUME_CORRUPTED;
-        return EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       if (!Ext4ValidDirent (Entry)) {
-        FreePool (Buf);
+        Status = EFI_VOLUME_CORRUPTED;
-        return EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       if ((Entry->name_len > RemainingBlock) || (Entry->rec_len > RemainingBlock)) {
         // Corrupted filesystem
-        FreePool (Buf);
+        Status = EFI_VOLUME_CORRUPTED;
-        return EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       // Unused entry
@@ -174,20 +181,26 @@ Ext4RetrieveDirent (
        * need to form valid ASCII/UTF-8 sequences.
        */
       if (EFI_ERROR (Status)) {
-        // If we error out, skip this entry
+        if (Status == EFI_INVALID_PARAMETER) {
+          // If we error out due to a bad UTF-8 sequence (see Ext4GetUcs2DirentName), skip this entry.
           // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
           BlockOffset += Entry->rec_len;
           continue;
         }
 
+        // Other sorts of errors should just error out.
+        FreePool (Buf);
+        return Status;
+      }
+
       if ((Entry->name_len == StrLen (Name)) &&
           !Ext4StrCmpInsensitive (DirentUcs2Name, (CHAR16 *)Name))
       {
         ToCopy = MIN (Entry->rec_len, sizeof (EXT4_DIR_ENTRY));
 
         CopyMem (Result, Entry, ToCopy);
-        FreePool (Buf);
+        Status = EFI_SUCCESS;
-        return EFI_SUCCESS;
+        goto Out;
       }
 
       BlockOffset += Entry->rec_len;
@@ -196,8 +209,11 @@ Ext4RetrieveDirent (
     Off += Partition->BlockSize;
   }
 
+  Status = EFI_NOT_FOUND;
+
+Out:
   FreePool (Buf);
-  return EFI_NOT_FOUND;
+  return Status;
 }
 
 /**
@@ -248,13 +264,18 @@ Ext4OpenDirent (
     // Using the parent's parent's dentry
     File->Dentry = Directory->Dentry->Parent;
 
-    ASSERT (File->Dentry != NULL);
+    if (!File->Dentry) {
+      // Someone tried .. on root, so direct them to /
+      // This is an illegal EFI Open() but is possible to hit from a variety of internal code
+      File->Dentry = Directory->Dentry;
+    }
 
     Ext4RefDentry (File->Dentry);
   } else {
     File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
 
-    if (!File->Dentry) {
+    if (File->Dentry == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
       goto Error;
     }
   }
@@ -436,6 +457,7 @@ Ext4ReadDir (
   EXT4_FILE       *TempFile;
   BOOLEAN         ShouldSkip;
   BOOLEAN         IsDotOrDotDot;
+  CHAR16          DirentUcs2Name[EXT4_NAME_MAX + 1];
 
   DirIno     = File->Inode;
   Status     = EFI_SUCCESS;
@@ -491,18 +513,35 @@ Ext4ReadDir (
     // or a checksum at the end of the directory block.
     // memcmp (and CompareMem) return 0 when the passed length is 0.
 
-    IsDotOrDotDot = Entry.name_len != 0 &&
+    // We must bound name_len as > 0 and <= 2 to avoid any out-of-bounds accesses or bad detection of
-                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
+    // "." and "..".
-                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
+    IsDotOrDotDot = Entry.name_len > 0 && Entry.name_len <= 2 &&
+                    CompareMem (Entry.name, "..", Entry.name_len) == 0;
 
-    // When inode = 0, it's unused.
+    // When inode = 0, it's unused. When name_len == 0, it's a nameless entry
-    ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
+    // (which we should not expose to ReadDir).
+    ShouldSkip = Entry.inode == 0 || Entry.name_len == 0 || IsDotOrDotDot;
 
     if (ShouldSkip) {
       Offset += Entry.rec_len;
       continue;
     }
 
+    // Test if the dirent is valid utf-8. This is already done inside Ext4OpenDirent but EFI_INVALID_PARAMETER
+    // has the danger of its meaning being overloaded in many places, so we can't skip according to that.
+    // So test outside of it, explicitly.
+    Status = Ext4GetUcs2DirentName (&Entry, DirentUcs2Name);
+
+    if (EFI_ERROR (Status)) {
+      if (Status == EFI_INVALID_PARAMETER) {
+        // Bad UTF-8, skip.
+        Offset += Entry.rec_len;
+        continue;
+      }
+
+      goto Out;
+    }
+
     Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
 
     if (EFI_ERROR (Status)) {

Ext4Pkg/Ext4Dxe/DiskUtil.c

@@ -54,17 +54,20 @@ Ext4ReadBlocks (
   UINT64  Offset;
   UINTN   Length;
 
+  ASSERT (NumberBlocks != 0);
+  ASSERT (BlockNumber != EXT4_BLOCK_FILE_HOLE);
+
   Offset = MultU64x32 (BlockNumber, Partition->BlockSize);
   Length = NumberBlocks * Partition->BlockSize;
 
   // Check for overflow on the block -> byte conversions.
   // Partition->BlockSize is never 0, so we don't need to check for that.
 
-  if (Offset > DivU64x32 ((UINT64)-1, Partition->BlockSize)) {
+  if (DivU64x64Remainder (Offset, BlockNumber, NULL) != Partition->BlockSize) {
     return EFI_INVALID_PARAMETER;
   }
 
-  if (Length > (UINTN)-1/Partition->BlockSize) {
+  if (Length / NumberBlocks != Partition->BlockSize) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -92,14 +95,21 @@ Ext4AllocAndReadBlocks (
   VOID   *Buf;
   UINTN  Length;
 
+  // Check that number of blocks isn't empty, because
+  // this is incorrect condition for opened partition,
+  // so we just early-exit
+  if ((NumberBlocks == 0) || (BlockNumber == EXT4_BLOCK_FILE_HOLE)) {
+    return NULL;
+  }
+
   Length = NumberBlocks * Partition->BlockSize;
 
-  if (Length > (UINTN)-1/Partition->BlockSize) {
+  // Check for integer overflow
+  if (Length / NumberBlocks != Partition->BlockSize) {
     return NULL;
   }
 
   Buf = AllocatePool (Length);
-
   if (Buf == NULL) {
     return NULL;
   }

Ext4Pkg/Ext4Dxe/Ext4Disk.h

@@ -1,7 +1,7 @@
 /** @file
   Raw filesystem data structures
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   Layout of an EXT2/3/4 filesystem:
@@ -397,12 +397,29 @@ typedef struct _Ext4Inode {
   UINT32       i_projid;
 } EXT4_INODE;
 
+#define EXT4_NAME_MAX  255
+
 typedef struct {
+  // offset 0x0: inode number (if 0, unused entry, should skip.)
   UINT32    inode;
+  // offset 0x4: Directory entry's length.
+  //             Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and rec_len % 4 == 0.
   UINT16    rec_len;
+  // offset 0x6: Directory entry's name's length
   UINT8     name_len;
+  // offset 0x7: Directory entry's file type indicator
   UINT8     file_type;
-  CHAR8     name[255];
+  // offset 0x8: name[name_len]: Variable length character array; not null-terminated.
+  CHAR8     name[EXT4_NAME_MAX];
+  // Further notes on names:
+  // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of use around the driver.
+  //
+  // 2) ext4 directories are defined, as the documentation puts it, as:
+  // "a directory is more or less a flat file that maps an arbitrary byte string
+  // (usually ASCII) to an inode number on the filesystem". So, they are not
+  // necessarily encoded with ASCII, UTF-8, or any of the sort. We must treat it
+  // as a bag of bytes. When interacting with EFI interfaces themselves (which expect UCS-2)
+  // we skip any directory entry that is not valid UTF-8.
 } EXT4_DIR_ENTRY;
 
 #define EXT4_MIN_DIR_ENTRY_LEN  8
@@ -467,8 +484,17 @@ typedef UINT64  EXT4_BLOCK_NR;
 typedef UINT32  EXT2_BLOCK_NR;
 typedef UINT32  EXT4_INO_NR;
 
-// 2 is always the root inode number in ext4
+/* Special inode numbers */
 #define EXT4_ROOT_INODE_NR         2
+#define EXT4_USR_QUOTA_INODE_NR    3
+#define EXT4_GRP_QUOTA_INODE_NR    4
+#define EXT4_BOOT_LOADER_INODE_NR  5
+#define EXT4_UNDEL_DIR_INODE_NR    6
+#define EXT4_RESIZE_INODE_NR       7
+#define EXT4_JOURNAL_INODE_NR      8
+
+/* First non-reserved inode for old ext4 filesystems */
+#define EXT4_GOOD_OLD_FIRST_INODE_NR  11
 
 #define EXT4_BLOCK_FILE_HOLE  0
 

Ext4Pkg/Ext4Dxe/Ext4Dxe.c

@@ -1,7 +1,7 @@
 /** @file
   Driver entry point
 
-  Copyright (c) 2021 Pedro Falcato All rights reserved.
+  Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
@@ -513,9 +513,7 @@ Ext4EntryPoint (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  EFI_STATUS  Status;
+  return EfiLibInstallAllDriverProtocols2 (
-
-  Status = EfiLibInstallAllDriverProtocols2 (
            ImageHandle,
            SystemTable,
            &gExt4BindingProtocol,
@@ -527,12 +525,6 @@ Ext4EntryPoint (
            NULL,
            NULL
            );
-
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  return Ext4InitialiseUnicodeCollation (ImageHandle);
 }
 
 /**
@@ -761,6 +753,17 @@ Ext4Bind (
   BlockIo = NULL;
   DiskIo  = NULL;
 
+  // Note: We initialize collation here since this is called in BDS, when we are likely
+  // to have the Unicode Collation protocols available.
+  Status = Ext4InitialiseUnicodeCollation (BindingProtocol->ImageHandle);
+  if (EFI_ERROR (Status)) {
+    // Lets throw a loud error into the log
+    // It is very unlikely something like this may fire out of the blue. Chances are either
+    // the platform configuration is wrong, or we are.
+    DEBUG ((DEBUG_ERROR, "[ext4] Error: Unicode Collation not available - failure to Start() - error %r\n", Status));
+    goto Error;
+  }
+
   Status = gBS->OpenProtocol (
                   ControllerHandle,
                   &gEfiDiskIoProtocolGuid,
@@ -774,7 +777,7 @@ Ext4Bind (
     goto Error;
   }
 
-  DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO\n"));
+  DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO\n"));
 
   Status = gBS->OpenProtocol (
                   ControllerHandle,
@@ -787,7 +790,7 @@ Ext4Bind (
   // It's okay to not support DISK_IO2
 
   if (DiskIo2 != NULL) {
-    DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO2\n"));
+    DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO2\n"));
   }
 
   Status = gBS->OpenProtocol (

Ext4Pkg/Ext4Dxe/Ext4Dxe.h

@@ -1,7 +1,7 @@
 /** @file
   Common header for the driver
 
-  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
 **/
 
@@ -32,7 +32,6 @@
 #include "Ext4Disk.h"
 
 #define SYMLOOP_MAX  8
-#define EXT4_NAME_MAX  255
 //
 // We need to specify path length limit for security purposes, to prevent possible
 // overflows and dead-loop conditions. Originally this limit is absent in FS design,
@@ -41,6 +40,20 @@
 #define EXT4_EFI_PATH_MAX    4096
 #define EXT4_DRIVER_VERSION  0x0000
 
+//
+// The EXT4 Specification doesn't strictly limit block size and this value could be up to 2^31,
+// but in practice it is limited by PAGE_SIZE due to performance significant impact.
+// Many EXT4 implementations have size of block limited to PAGE_SIZE. In many cases it's limited
+// to 4096, which is a commonly supported page size on most MMU-capable hardware, and up to 65536.
+// So, to take a balance between compatibility and security measures, it is decided to use the
+// value of 2MiB as the limit, which is equal to large page size on new hardware.
+// As for supporting big block sizes, EXT4 has a RO_COMPAT_FEATURE called BIGALLOC, which changes
+// EXT4 to use clustered allocation, so that each bit in the ext4 block allocation bitmap addresses
+// a power of two number of blocks. So it would be wiser to implement and use this feature
+// if there is such a need instead of big block size.
+//
+#define EXT4_LOG_BLOCK_SIZE_MAX  11
+
 /**
    Opens an ext4 partition and installs the Simple File System protocol.
 
@@ -156,7 +169,7 @@ Ext4UnrefDentry (
 
    @param[out]     Partition Partition structure to fill with filesystem
 details.
-   @retval EFI_SUCCESS       Parsing was succesful and the partition is a
+   @retval EFI_SUCCESS       Parsing was successful and the partition is a
                              valid ext4 partition.
 **/
 EFI_STATUS
@@ -288,6 +301,16 @@ Ext4GetBlockGroupDesc (
   IN UINT32          BlockGroup
   );
 
+/**
+   Checks inode number validity across superblock of the opened partition.
+
+   @param[in]  Partition      Pointer to the opened ext4 partition.
+
+   @return TRUE if inode number is valid.
+**/
+#define EXT4_IS_VALID_INODE_NR(Partition, InodeNum)                            \
+  (((InodeNum) > 0) && (InodeNum) <= (Partition->SuperBlock.s_inodes_count))
+
 /**
    Reads an inode from disk.
 
@@ -323,7 +346,7 @@ Ext4ReadInode (
    @param[out]     Buffer        Pointer to the buffer.
    @param[in]      Offset        Offset of the read.
    @param[in out]  Length        Pointer to the length of the buffer, in bytes.
-                                 After a succesful read, it's updated to the
+                                 After a successful read, it's updated to the
 number of read bytes.
 
    @return Status of the read operation.
@@ -356,7 +379,7 @@ cover.
    @param[out]     Extent        Pointer to the output buffer, where the extent
 will be copied to.
 
-   @retval EFI_SUCCESS        Retrieval was succesful.
+   @retval EFI_SUCCESS        Retrieval was successful.
    @retval EFI_NO_MAPPING     Block has no mapping.
 **/
 EFI_STATUS
@@ -945,10 +968,10 @@ Ext4StrCmpInsensitive (
    Retrieves the filename of the directory entry and converts it to UTF-16/UCS-2
 
    @param[in]      Entry   Pointer to a EXT4_DIR_ENTRY.
-   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size
+   @param[out]      Ucs2FileName   Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1.
-EXT4_NAME_MAX + 1.
 
-   @retval EFI_SUCCESS   Unicode collation was successfully initialised.
+   @retval EFI_SUCCESS              The filename was successfully retrieved and converted to UCS2.
+   @retval EFI_INVALID_PARAMETER    The filename is not valid UTF-8.
    @retval !EFI_SUCCESS             Failure.
 **/
 EFI_STATUS
@@ -1107,7 +1130,7 @@ Ext4CalculateBlockGroupDescChecksum (
   );
 
 /**
-   Verifies the existance of a particular RO compat feature set.
+   Verifies the existence of a particular RO compat feature set.
    @param[in]      Partition           Pointer to the opened EXT4 partition.
    @param[in]      RoCompatFeatureSet  Feature set to test.
 
@@ -1117,7 +1140,7 @@ Ext4CalculateBlockGroupDescChecksum (
   ((Partition->FeaturesRoCompat & RoCompatFeatureSet) == RoCompatFeatureSet)
 
 /**
-   Verifies the existance of a particular compat feature set.
+   Verifies the existence of a particular compat feature set.
    @param[in]      Partition           Pointer to the opened EXT4 partition.
    @param[in]      CompatFeatureSet  Feature set to test.
 
@@ -1127,7 +1150,7 @@ Ext4CalculateBlockGroupDescChecksum (
   ((Partition->FeaturesCompat & CompatFeatureSet) == CompatFeatureSet)
 
 /**
-   Verifies the existance of a particular compat feature set.
+   Verifies the existence of a particular compat feature set.
    @param[in]      Partition           Pointer to the opened EXT4 partition.
    @param[in]      IncompatFeatureSet  Feature set to test.
 
@@ -1218,7 +1241,7 @@ Ext4GetExtentLength (
    @param[in]      LogicalBlock  Block number which the returned extent must cover.
    @param[out]     Extent        Pointer to the output buffer, where the extent will be copied to.
 
-   @retval EFI_SUCCESS        Retrieval was succesful.
+   @retval EFI_SUCCESS        Retrieval was successful.
    @retval EFI_NO_MAPPING     Block has no mapping.
 **/
 EFI_STATUS

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
 **/
 
@@ -39,8 +39,11 @@ Ext4CalculateExtentChecksum (
    @param[in]      File        Pointer to the open file.
    @param[in]      Extents     Pointer to an array of extents.
    @param[in]      NumberExtents Length of the array.
+
+   @return         Result of the caching
 **/
-VOID
+STATIC
+EFI_STATUS
 Ext4CacheExtents (
   IN EXT4_FILE          *File,
   IN CONST EXT4_EXTENT  *Extents,
@@ -80,13 +83,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 +104,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 +229,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.
@@ -219,7 +239,7 @@ Ext4ExtentIdxLeafBlock (
    @param[in]      LogicalBlock  Block number which the returned extent must cover.
    @param[out]     Extent        Pointer to the output buffer, where the extent will be copied to.
 
-   @retval EFI_SUCCESS        Retrieval was succesful.
+   @retval EFI_SUCCESS        Retrieval was successful.
    @retval EFI_NO_MAPPING     Block has no mapping.
 **/
 EFI_STATUS
@@ -237,6 +257,8 @@ Ext4GetExtent (
   EXT4_EXTENT_HEADER  *ExtHeader;
   EXT4_EXTENT_INDEX   *Index;
   EFI_STATUS          Status;
+  UINT16              MaxExtentsPerNode;
+  EXT4_BLOCK_NR       BlockNumber;
 
   Inode  = File->Inode;
   Ext    = NULL;
@@ -264,7 +286,13 @@ Ext4GetExtent (
     Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Extent);
 
     if (!EFI_ERROR (Status)) {
-      Ext4CacheExtents (File, Extent, 1);
+      Status = Ext4CacheExtents (File, Extent, 1);
+
+      if (EFI_ERROR (Status) && (Status != EFI_OUT_OF_RESOURCES)) {
+        return Status;
+      }
+
+      Status = EFI_SUCCESS;
     }
 
     return Status;
@@ -274,12 +302,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 = (UINT16)((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,6 +322,16 @@ Ext4GetExtent (
     // (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;
+    }
 
     if (Buffer == NULL) {
       Buffer = AllocatePool (Partition->BlockSize);
@@ -299,7 +342,7 @@ Ext4GetExtent (
 
     // Read the leaf block onto the previously-allocated buffer.
 
-    Status = Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlock (Index));
+    Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber);
     if (EFI_ERROR (Status)) {
       FreePool (Buffer);
       return Status;
@@ -307,7 +350,7 @@ Ext4GetExtent (
 
     ExtHeader = Buffer;
 
-    if (!Ext4ExtentHeaderValid (ExtHeader)) {
+    if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) {
       FreePool (Buffer);
       return EFI_VOLUME_CORRUPTED;
     }
@@ -329,7 +372,15 @@ Ext4GetExtent (
    * by linux (and possibly other systems) is quite fancy and usually it results in a small number of extents.
    * Therefore, we shouldn't have any memory issues.
   **/
-  Ext4CacheExtents (File, (EXT4_EXTENT *)(ExtHeader + 1), ExtHeader->eh_entries);
+  Status = Ext4CacheExtents (File, (EXT4_EXTENT *)(ExtHeader + 1), ExtHeader->eh_entries);
+
+  if (EFI_ERROR (Status) && (Status != EFI_OUT_OF_RESOURCES)) {
+    if (Buffer != NULL) {
+      FreePool (Buffer);
+    }
+
+    return Status;
+  }
 
   Ext = Ext4BinsearchExtentExt (ExtHeader, LogicalBlock);
 
@@ -485,8 +536,11 @@ Ext4FreeExtentsMap (
    @param[in]      File        Pointer to the open file.
    @param[in]      Extents     Pointer to an array of extents.
    @param[in]      NumberExtents Length of the array.
+
+   @return         Result of the caching
 **/
-VOID
+STATIC
+EFI_STATUS
 Ext4CacheExtents (
   IN EXT4_FILE          *File,
   IN CONST EXT4_EXTENT  *Extents,
@@ -502,10 +556,15 @@ Ext4CacheExtents (
    */
 
   for (Idx = 0; Idx < NumberExtents; Idx++, Extents++) {
+    if (Extents->ee_len == 0) {
+      // 0-sized extent, must be corruption
+      return EFI_VOLUME_CORRUPTED;
+    }
+
     Extent = AllocatePool (sizeof (EXT4_EXTENT));
 
     if (Extent == NULL) {
-      return;
+      return EFI_OUT_OF_RESOURCES;
     }
 
     CopyMem (Extent, Extents, sizeof (EXT4_EXTENT));
@@ -519,9 +578,11 @@ Ext4CacheExtents (
         continue;
       }
 
-      return;
+      return EFI_SUCCESS;
     }
   }
+
+  return EFI_SUCCESS;
 }
 
 /**
@@ -615,7 +676,7 @@ Ext4GetExtentLength (
   IN CONST EXT4_EXTENT  *Extent
   )
 {
-  // If it's an unintialized extent, the true length is ee_len - 2^15
+  // If it's an uninitialized extent, the true length is ee_len - 2^15
   if (EXT4_EXTENT_IS_UNINITIALIZED (Extent)) {
     return Extent->ee_len - EXT4_EXTENT_MAX_INITIALIZED;
   }

Ext4Pkg/Ext4Dxe/File.c

@@ -105,7 +105,7 @@ Ext4IsLastPathSegment (
    @param[in out] File         Pointer to the file we're opening.
    @param[in]     OpenMode     Mode in which to open the file.
 
-   @return           True if the open was succesful, false if we don't have
+   @return           True if the open was successful, false if we don't have
                      enough permissions.
 **/
 STATIC
@@ -207,6 +207,11 @@ Ext4OpenInternal (
   Level     = 0;
 
   DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
+
+  if (!Ext4FileIsDir (Current)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   // If the path starts with a backslash, we treat the root directory as the base directory
   if (FileName[0] == L'\\') {
     FileName++;
@@ -219,6 +224,10 @@ Ext4OpenInternal (
       return EFI_ACCESS_DENIED;
     }
 
+    if (!Ext4FileIsDir (Current)) {
+      return EFI_INVALID_PARAMETER;
+    }
+
     // Discard leading path separators
     while (FileName[0] == L'\\') {
       FileName++;
@@ -242,10 +251,6 @@ Ext4OpenInternal (
 
     DEBUG ((DEBUG_FS, "[ext4] Opening %s\n", PathSegment));
 
-    if (!Ext4FileIsDir (Current)) {
-      return EFI_INVALID_PARAMETER;
-    }
-
     if (!Ext4IsLastPathSegment (FileName)) {
       if (!Ext4DirCanLookup (Current)) {
         return EFI_ACCESS_DENIED;
@@ -715,6 +720,10 @@ Ext4GetVolumeName (
     VolNameLength = StrLen (VolumeName);
   } else {
     VolumeName = AllocateZeroPool (sizeof (CHAR16));
+    if (VolumeName == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
     VolNameLength = 0;
   }
 
@@ -781,7 +790,9 @@ Ext4GetFilesystemInfo (
   Info->VolumeSize = MultU64x32 (TotalBlocks, Part->BlockSize);
   Info->FreeSpace  = MultU64x32 (FreeBlocks, Part->BlockSize);
 
-  StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
+  Status = StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
+
+  ASSERT_EFI_ERROR (Status);
 
   FreePool (VolumeName);
 

Ext4Pkg/Ext4Dxe/Inode.c

@@ -76,7 +76,7 @@ Ext4CalculateInodeChecksum (
    @param[out]     Buffer        Pointer to the buffer.
    @param[in]      Offset        Offset of the read.
    @param[in out]  Length        Pointer to the length of the buffer, in bytes.
-                                 After a succesful read, it's updated to the number of read bytes.
+                                 After a successful read, it's updated to the number of read bytes.
 
    @return Status of the read operation.
 **/
@@ -152,7 +152,7 @@ Ext4Read (
       } else {
         // Uninitialized extents behave exactly the same as file holes, except they have
         // blocks already allocated to them.
-        HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) - HoleOff;
+        HoleLen = MultU64x32 (Ext4GetExtentLength (&Extent), Partition->BlockSize) - HoleOff;
       }
 
       WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen;
@@ -166,7 +166,7 @@ Ext4Read (
                            Partition->BlockSize
                            );
       ExtentLengthBytes  = Extent.ee_len * Partition->BlockSize;
-      ExtentLogicalBytes = (UINT64)Extent.ee_block * Partition->BlockSize;
+      ExtentLogicalBytes = MultU64x32 ((UINT64)Extent.ee_block, Partition->BlockSize);
       ExtentOffset       = CurrentSeek - ExtentLogicalBytes;
       ExtentMayRead      = (UINTN)(ExtentLengthBytes - ExtentOffset);
 
@@ -230,7 +230,7 @@ Ext4AllocateInode (
 
   Inode = AllocateZeroPool (InodeSize);
 
-  if (!Inode) {
+  if (Inode == NULL) {
     return NULL;
   }
 

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.
@@ -151,7 +151,7 @@ Ext4VerifySuperblockChecksum (
    Opens and parses the superblock.
 
    @param[out]     Partition Partition structure to fill with filesystem details.
-   @retval EFI_SUCCESS       Parsing was succesful and the partition is a
+   @retval EFI_SUCCESS       Parsing was successful and the partition is a
                              valid ext4 partition.
 **/
 EFI_STATUS
@@ -203,7 +203,7 @@ Ext4OpenSuperblock (
 
   // Now, check for the feature set of the filesystem
   // It's essential to check for this to avoid filesystem corruption and to avoid
-  // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disasterous.
+  // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disastrous.
 
   if (Partition->FeaturesIncompat & ~gSupportedIncompatFeat) {
     DEBUG ((
@@ -220,13 +220,11 @@ Ext4OpenSuperblock (
   }
 
   // At the time of writing, it's the only supported checksum.
-  if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
+  if (EXT4_HAS_METADATA_CSUM (Partition) && (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C)) {
-      (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C))
-  {
     return EFI_UNSUPPORTED;
   }
 
-  if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) != 0) {
+  if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_CSUM_SEED)) {
     Partition->InitialSeed = Sb->s_checksum_seed;
   } else {
     Partition->InitialSeed = Ext4CalculateChecksum (Partition, Sb->s_uuid, 16, ~0U);
@@ -245,6 +243,16 @@ Ext4OpenSuperblock (
 
   DEBUG ((DEBUG_FS, "Read only = %u\n", Partition->ReadOnly));
 
+  if (Sb->s_inodes_per_group == 0) {
+    DEBUG ((DEBUG_ERROR, "[ext4] Inodes per group can not be zero\n"));
+    return EFI_VOLUME_CORRUPTED;
+  }
+
+  if (Sb->s_log_block_size > EXT4_LOG_BLOCK_SIZE_MAX) {
+    DEBUG ((DEBUG_ERROR, "[ext4] SuperBlock s_log_block_size %lu is too big\n", Sb->s_log_block_size));
+    return EFI_UNSUPPORTED;
+  }
+
   Partition->BlockSize = (UINT32)LShiftU64 (1024, Sb->s_log_block_size);
 
   // The size of a block group can also be calculated as 8 * Partition->BlockSize
@@ -312,7 +320,7 @@ Ext4OpenSuperblock (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  // Note that the cast below is completely safe, because EXT4_FILE is a specialisation of EFI_FILE_PROTOCOL
+  // Note that the cast below is completely safe, because EXT4_FILE is a specialization of EFI_FILE_PROTOCOL
   Status = Ext4OpenVolume (&Partition->Interface, (EFI_FILE_PROTOCOL **)&Partition->Root);
 
   if (EFI_ERROR (Status)) {

Ext4Pkg/Ext4Dxe/Symlink.c

@@ -1,7 +1,7 @@
 /** @file
   Symbolic links routines
 
-  Copyright (c) 2022 Savva Mitrofanov All rights reserved.
+  Copyright (c) 2022-2023 Savva Mitrofanov All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
@@ -155,19 +155,20 @@ Ext4ReadSlowSymlink (
     return Status;
   }
 
-  //
-  // Add null-terminator
-  //
-  SymlinkTmp[SymlinkSizeTmp] = '\0';
-
   if (SymlinkSizeTmp != ReadSize) {
     DEBUG ((
       DEBUG_FS,
       "[ext4] Error! The size of the read block doesn't match the value from the inode!\n"
       ));
+    FreePool (SymlinkTmp);
     return EFI_VOLUME_CORRUPTED;
   }
 
+  //
+  // Add null-terminator
+  //
+  SymlinkTmp[ReadSize] = '\0';
+
   *AsciiSymlinkSize = SymlinkAllocateSize;
   *AsciiSymlink     = SymlinkTmp;
 
@@ -201,7 +202,7 @@ Ext4ReadSymlink (
   CHAR16      *Needle;
 
   //
-  // Assume that we alread read Inode via Ext4ReadInode
+  // Assume that we already read Inode via Ext4ReadInode
   // Skip reading, just check encryption flag
   //
   if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
@@ -242,7 +243,6 @@ Ext4ReadSymlink (
       Status
       ));
     FreePool (Symlink16Tmp);
-    FreePool (SymlinkTmp);
     return Status;
   }