[edk2-platforms][PATCH v1 00/12] Ext4Pkg: Code correctness and security improvements

[edk2-platforms][PATCH v1 00/12] Ext4Pkg: Code correctness and security improvements

[Savva Mitrofanov]

Hi all,

This patchset fixes several code problems found by fuzzing Ext4Dxe like
buffer and integer overflows, memory leaks, logic bugs and so on.

REF: https://github.com/savvamitrofanov/edk2-platforms/tree/master

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>

Savva Mitrofanov (12):
  Ext4Pkg: Fix memory leak in Ext4RetrieveDirent
  Ext4Pkg: Move EXT4_NAME_MAX definition to Ext4Disk.h
  Ext4Pkg: Fix global buffer overflow in Ext4ReadDir
  Ext4Pkg: Fix incorrect checksum metadata feature check
  Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group
  Ext4Pkg: Add comparison between Position and FileSize in
    Ext4SetPosition
  Ext4Pkg: Add inode number validity check
  Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock
  Ext4Pkg: Correct integer overflow check on multiplication in DiskUtil
  Ext4Pkg: Check that source file is directory in Ext4OpenInternal
  Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName
  Ext4Pkg: Add missing exit Status in Ext4OpenDirent

 Features/Ext4Pkg/Ext4Pkg.dsc          |  2 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 17 +++++-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 59 ++++++++++++++++----
 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 51 ++++++++++-------
 Features/Ext4Pkg/Ext4Dxe/DiskUtil.c   |  8 +--
 Features/Ext4Pkg/Ext4Dxe/File.c       | 44 ++++++++++-----
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |  2 +-
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 16 ++++--
 9 files changed, 147 insertions(+), 57 deletions(-)

-- 
2.38.1

[edk2-platforms][PATCH v1 01/12] Ext4Pkg: Fix memory leak in Ext4RetrieveDirent

[Savva Mitrofanov]

We need to free buffer on return if BlockRemainder != 0. Also changed
return logic from function to use use common exit to prevent code
duplication.

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/Directory.c | 30 +++++++++++---------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 4441e6d192b6..8b8fce568e43 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -112,8 +112,7 @@ Ext4RetrieveDirent (
   UINTN           ToCopy;
   UINTN           BlockOffset;
 
-  Status = EFI_NOT_FOUND;
-  Buf    = AllocatePool (Partition->BlockSize);
+  Buf = AllocatePool (Partition->BlockSize);
 
   if (Buf == NULL) {
     return EFI_OUT_OF_RESOURCES;
@@ -127,7 +126,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 +136,7 @@ Ext4RetrieveDirent (
     Status = Ext4Read (Partition, Directory, Buf, Off, &Length);
 
     if (Status != EFI_SUCCESS) {
-      FreePool (Buf);
-      return Status;
+      goto Out;
     }
 
     for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) {
@@ -145,19 +144,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);
-        return EFI_VOLUME_CORRUPTED;
+        Status = EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       if (!Ext4ValidDirent (Entry)) {
-        FreePool (Buf);
-        return EFI_VOLUME_CORRUPTED;
+        Status = EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       if ((Entry->name_len > RemainingBlock) || (Entry->rec_len > RemainingBlock)) {
         // Corrupted filesystem
-        FreePool (Buf);
-        return EFI_VOLUME_CORRUPTED;
+        Status = EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       // Unused entry
@@ -186,8 +185,8 @@ Ext4RetrieveDirent (
         ToCopy = MIN (Entry->rec_len, sizeof (EXT4_DIR_ENTRY));
 
         CopyMem (Result, Entry, ToCopy);
-        FreePool (Buf);
-        return EFI_SUCCESS;
+        Status = EFI_SUCCESS;
+        goto Out;
       }
 
       BlockOffset += Entry->rec_len;
@@ -196,8 +195,11 @@ Ext4RetrieveDirent (
     Off += Partition->BlockSize;
   }
 
+  Status = EFI_NOT_FOUND;
+
+Out:
   FreePool (Buf);
-  return EFI_NOT_FOUND;
+  return Status;
 }
 
 /**
-- 
2.38.1

[edk2-platforms][PATCH v1 02/12] Ext4Pkg: Move EXT4_NAME_MAX definition to Ext4Disk.h

[Savva Mitrofanov]

Constant EXT4_NAME_MAX is related to EXT4_DIR_ENTRY FS structure, so it
should be placed into Ext4Disk.h header

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 | 4 +++-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 4fd91a423324..1285644dcb25 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -397,12 +397,14 @@ typedef struct _Ext4Inode {
   UINT32       i_projid;
 } EXT4_INODE;
 
+#define EXT4_NAME_MAX 255
+
 typedef struct {
   UINT32    inode;
   UINT16    rec_len;
   UINT8     name_len;
   UINT8     file_type;
-  CHAR8     name[255];
+  CHAR8     name[EXT4_NAME_MAX];
 } EXT4_DIR_ENTRY;
 
 #define EXT4_MIN_DIR_ENTRY_LEN  8
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index adf3c13f6ea9..dde4f4cb0e06 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -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,
-- 
2.38.1

[edk2-platforms][PATCH v1 03/12] Ext4Pkg: Fix global buffer overflow in Ext4ReadDir

[Savva Mitrofanov]

Directory entry structure can contain name_len bigger than size of "."
or "..", that's why CompareMem in such cases leads to global buffer
overflow. So there are two problems. The first is that statement doesn't
check cases when name_len != 0 but > 2 and the second is that we passing
big Length to CompareMem routine.
The correct way here is to check that name_len <= 2 and check for
null-terminator presence

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/Directory.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 8b8fce568e43..ffc0e8043076 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -491,11 +491,9 @@ Ext4ReadDir (
 
     // Entry.name_len may be 0 if it's a nameless entry, like an unused entry
     // 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 &&
-                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
-                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
+    IsDotOrDotDot = Entry.name_len <= 2 &&
+                    ((Entry.name[0] == '.') &&
+                     (Entry.name[1] == '.' || Entry.name[1] == '\0'));
 
     // When inode = 0, it's unused.
     ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
-- 
2.38.1

[edk2-platforms][PATCH v1 04/12] Ext4Pkg: Fix incorrect checksum metadata feature check

[Savva Mitrofanov]

Missing comparison != 0 leads to broken logic condition. Also replaced
CSUM_SEED feature_incompat check with predefined macro EXT4_HAS_INCOMPAT

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/Superblock.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index edee051c41e8..4c662bd1784f 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -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 &&
-      (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C))
-  {
+  if (EXT4_HAS_METADATA_CSUM (Partition) && (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);
-- 
2.38.1

[edk2-platforms][PATCH v1 05/12] Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group

[Savva Mitrofanov]

Superblock s_inodes_per_group field can't be zero, it leads to division
by zero in BlockGroup routine Ext4ReadInode

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/Superblock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 4c662bd1784f..adaf475ea54d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -243,6 +243,11 @@ 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;
+  }
+
   Partition->BlockSize = (UINT32)LShiftU64 (1024, Sb->s_log_block_size);
 
   // The size of a block group can also be calculated as 8 * Partition->BlockSize
-- 
2.38.1

[edk2-platforms][PATCH v1 06/12] Ext4Pkg: Add comparison between Position and FileSize in Ext4SetPosition

[Savva Mitrofanov]

Missing such comparison leads to infinite loop states, for example code
which trying to read entire file can easily get out of bound of
file size by passing position value which exceeds file size without this
check. So we need to add there missing comparison between the desired
position to be set and file size

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/Ext4Dxe.h | 19 +++++++++---------
 Features/Ext4Pkg/Ext4Dxe/File.c    | 21 +++++++++++++-------
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index dde4f4cb0e06..1dcb644e3b35 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -31,7 +31,7 @@
 
 #include "Ext4Disk.h"
 
-#define SYMLOOP_MAX    8
+#define SYMLOOP_MAX  8
 //
 // 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,
@@ -715,16 +715,15 @@ Ext4GetPosition (
 /**
   Sets a file's current position.
 
-  @param[in]  This            A pointer to the EFI_FILE_PROTOCOL instance that
-is the file handle to set the requested position on.
-  @param[in] Position        The byte position from the start of the file to
-set.
+  @param[in]  This            A pointer to the EFI_FILE_PROTOCOL instance that is the
+                              file handle to set the requested position on.
+  @param[in]  Position        The byte position from the start of the file to set.
 
-  @retval EFI_SUCCESS      The position was set.
-  @retval EFI_UNSUPPORTED  The seek request for nonzero is not valid on open
-                           directories.
-  @retval EFI_DEVICE_ERROR An attempt was made to set the position of a deleted
-file.
+  @retval EFI_SUCCESS            The position was set.
+  @retval EFI_INVALID_PARAMETER  The seek request for non-zero position is not valid on open
+                                 directories.
+  @retval EFI_UNSUPPORTED        The seek request for position is exceeds FileSize.
+  @retval EFI_DEVICE_ERROR       An attempt was made to set the position of a deleted file.
 
 **/
 EFI_STATUS
diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index 04198a53bfc0..b4ed78847258 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -587,12 +587,13 @@ Ext4GetPosition (
 
   @param[in]  This            A pointer to the EFI_FILE_PROTOCOL instance that is the
                               file handle to set the requested position on.
-  @param[in] Position        The byte position from the start of the file to set.
+  @param[in]  Position        The byte position from the start of the file to set.
 
-  @retval EFI_SUCCESS      The position was set.
-  @retval EFI_UNSUPPORTED  The seek request for nonzero is not valid on open
-                           directories.
-  @retval EFI_DEVICE_ERROR An attempt was made to set the position of a deleted file.
+  @retval EFI_SUCCESS            The position was set.
+  @retval EFI_INVALID_PARAMETER  The seek request for non-zero position is not valid on open
+                                 directories.
+  @retval EFI_UNSUPPORTED        The seek request for position is exceeds FileSize.
+  @retval EFI_DEVICE_ERROR       An attempt was made to set the position of a deleted file.
 
 **/
 EFI_STATUS
@@ -603,17 +604,23 @@ Ext4SetPosition (
   )
 {
   EXT4_FILE  *File;
+  UINT64     FileSize;
 
   File = EXT4_FILE_FROM_THIS (This);
 
   // Only seeks to 0 (so it resets the ReadDir operation) are allowed
   if (Ext4FileIsDir (File) && (Position != 0)) {
-    return EFI_UNSUPPORTED;
+    return EFI_INVALID_PARAMETER;
   }
 
+  FileSize = EXT4_INODE_SIZE (File->Inode);
+
   // -1 (0xffffff.......) seeks to the end of the file
   if (Position == (UINT64)-1) {
-    Position = EXT4_INODE_SIZE (File->Inode);
+    Position = FileSize;
+  } else if (Position > FileSize) {
+    DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to #%Lx of %Lx\n", Position, FileSize));
+    return EFI_UNSUPPORTED;
   }
 
   File->Position = Position;
-- 
2.38.1

[edk2-platforms][PATCH v1 07/12] Ext4Pkg: Add inode number validity check

[Savva Mitrofanov]

We need to validate inode number to prevent possible null-pointer
dereference of directory parent in Ext4OpenDirent. Also checks that
inode number valid across opened partition before we read it in
Ext4ReadInode.

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   | 15 +++++++++---
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 25 ++++++++++++++++++++
 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++++
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 10 ++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 1285644dcb25..6b56ce6813fc 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -397,7 +397,7 @@ typedef struct _Ext4Inode {
   UINT32       i_projid;
 } EXT4_INODE;
 
-#define EXT4_NAME_MAX 255
+#define EXT4_NAME_MAX  255
 
 typedef struct {
   UINT32    inode;
@@ -469,8 +469,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
-#define EXT4_ROOT_INODE_NR  2
+/* 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
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 1dcb644e3b35..d135892633af 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -287,6 +287,31 @@ Ext4GetBlockGroupDesc (
   IN UINT32          BlockGroup
   );
 
+/**
+   Retrieves the first usable non-reserved inode number from the superblock
+   of the opened partition.
+
+   @param[in]  Partition      Pointer to the opened ext4 partition.
+
+   @return The first usable inode number (non-reserved).
+**/
+#define EXT4_FIRST_INODE_NR(Partition)                                         \
+  ((Partition->SuperBlock.s_rev_level == EXT4_GOOD_OLD_REV) ?                  \
+         EXT4_GOOD_OLD_FIRST_INODE_NR :                                        \
+         Partition->SuperBlock.s_first_ino)
+
+/**
+   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 == EXT4_ROOT_INODE_NR ||                                           \
+        (InodeNum >= EXT4_FIRST_INODE_NR(Partition) &&                         \
+         InodeNum <= Partition->SuperBlock.s_inodes_count))
+
 /**
    Reads an inode from disk.
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
index cba96cd95afc..f34cdc5dbad7 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
+++ b/Features/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,
diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index ffc0e8043076..ff476c8641e8 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -163,6 +163,10 @@ Ext4RetrieveDirent (
       if (Entry->inode == 0) {
         BlockOffset += Entry->rec_len;
         continue;
+      } else if (!EXT4_IS_VALID_INODE_NR (Partition, Entry->inode)) {
+        DEBUG ((DEBUG_ERROR, "[ext4] Ext4RetrieveDirent directory entry inode number %u isn't valid\n", Entry->inode));
+        Status = EFI_VOLUME_CORRUPTED;
+        goto Out;
       }
 
       Status = Ext4GetUcs2DirentName (Entry, DirentUcs2Name);
@@ -498,6 +502,12 @@ Ext4ReadDir (
     // When inode = 0, it's unused.
     ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
 
+    if ((Entry.inode != 0) && !EXT4_IS_VALID_INODE_NR (Partition, Entry.inode)) {
+      DEBUG ((DEBUG_ERROR, "[ext4] Ext4ReadDir directory entry inode number %u isn't valid\n", Entry.inode));
+      Status = EFI_VOLUME_CORRUPTED;
+      goto Out;
+    }
+
     if (ShouldSkip) {
       Offset += Entry.rec_len;
       continue;
-- 
2.38.1

[edk2-platforms][PATCH v1 08/12] Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock

[Savva Mitrofanov]

Missing check for wrong s_log_block_size exponent leads to shift out of
bounds. Limit block size to 2 MiB

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/Ext4Dxe.h    | 14 ++++++++++++++
 Features/Ext4Pkg/Ext4Dxe/Superblock.c |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index d135892633af..0600a75d6750 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -40,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 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.
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index adaf475ea54d..ffe66a8bb847 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -248,6 +248,11 @@ Ext4OpenSuperblock (
     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
-- 
2.38.1

[edk2-platforms][PATCH v1 09/12] Ext4Pkg: Correct integer overflow check on multiplication in DiskUtil

[Savva Mitrofanov]

Multiplication overflow could result into small numbers, so we need also
check it

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/Ext4Pkg.dsc        | 2 +-
 Features/Ext4Pkg/Ext4Dxe/DiskUtil.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Pkg.dsc b/Features/Ext4Pkg/Ext4Pkg.dsc
index 59bc327ebf6e..621c63eaf92d 100644
--- a/Features/Ext4Pkg/Ext4Pkg.dsc
+++ b/Features/Ext4Pkg/Ext4Pkg.dsc
@@ -46,7 +46,7 @@
   DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
   BaseUcs2Utf8Lib|RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
-  
+
   #
   # Required for stack protector support
   #
diff --git a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
index 32da35f7d9f5..c4af956da926 100644
--- a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
+++ b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
@@ -60,11 +60,11 @@ Ext4ReadBlocks (
   // 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 ((NumberBlocks != 0) && (DivU64x64Remainder (Offset, BlockNumber, NULL) != Partition->BlockSize)) {
     return EFI_INVALID_PARAMETER;
   }
 
-  if (Length > (UINTN)-1/Partition->BlockSize) {
+  if ((NumberBlocks != 0) && (Length / NumberBlocks != Partition->BlockSize)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -94,12 +94,12 @@ Ext4AllocAndReadBlocks (
 
   Length = NumberBlocks * Partition->BlockSize;
 
-  if (Length > (UINTN)-1/Partition->BlockSize) {
+  // Check for integer overflow
+  if ((NumberBlocks != 0) && (Length / NumberBlocks != Partition->BlockSize)) {
     return NULL;
   }
 
   Buf = AllocatePool (Length);
-
   if (Buf == NULL) {
     return NULL;
   }
-- 
2.38.1

[edk2-platforms][PATCH v1 10/12] Ext4Pkg: Check that source file is directory in Ext4OpenInternal

[Savva Mitrofanov]

This check already present in the while loop below, but absent for cases
when input file is nameless, so to handle assertion in Ext4ReadFile we
need to add it at the top of function

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/File.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index b4ed78847258..2d76e1818021 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -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;
-- 
2.38.1

[edk2-platforms][PATCH v1 11/12] Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName

[Savva Mitrofanov]

Missing check in some cases leads to failed StrCpyS call in
Ext4GetVolumeLabelInfo. Also correct condition that checks Inode pointer
for being NULL in Ext4AllocateInode

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/File.c  | 10 ++++++++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index 2d76e1818021..7939fcd3acef 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -726,7 +726,11 @@ Ext4GetVolumeName (
 
     VolNameLength = StrLen (VolumeName);
   } else {
-    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
+    VolumeName = AllocateZeroPool (sizeof (CHAR16));
+    if (VolumeName == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
     VolNameLength = 0;
   }
 
@@ -793,7 +797,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);
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 5ccb4d2bfc42..2977238d687c 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -230,7 +230,7 @@ Ext4AllocateInode (
 
   Inode = AllocateZeroPool (InodeSize);
 
-  if (!Inode) {
+  if (Inode == NULL) {
     return NULL;
   }
 
-- 
2.38.1

[edk2-platforms][PATCH v1 12/12] Ext4Pkg: Add missing exit Status in Ext4OpenDirent

[Savva Mitrofanov]

Missing EFI_OUT_OF_RESOURCES exit status on failed Ext4CreateDentry
leads to NULL-pointer dereference in Ext4GetFileInfo (passing NULL
buffer in Ext4ReadDir)

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/Directory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index ff476c8641e8..efdce1477246 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -260,7 +260,8 @@ Ext4OpenDirent (
   } else {
     File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
 
-    if (!File->Dentry) {
+    if (File->Dentry == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
       goto Error;
     }
   }
-- 
2.38.1

Re: [edk2-platforms][PATCH v1 06/12] Ext4Pkg: Add comparison between Position and FileSize in Ext4SetPosition

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:

> Missing such comparison leads to infinite loop states, for example code
> which trying to read entire file can easily get out of bound of
> file size by passing position value which exceeds file size without this
> check. So we need to add there missing comparison between the desired
> position to be set and file size
>
> +  FileSize = EXT4_INODE_SIZE (File->Inode);
> +
>    // -1 (0xffffff.......) seeks to the end of the file
>    if (Position == (UINT64)-1) {
> -    Position = EXT4_INODE_SIZE (File->Inode);
> +    Position = FileSize;
> +  } else if (Position > FileSize) {
> +    DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to #%Lx of
> %Lx\n", Position, FileSize));
> +    return EFI_UNSUPPORTED;
>    }
>
>    File->Position = Position;
>

On further inspection, this case is covered in the UEFI spec.

https://uefi.org/specs/UEFI/2.10/13_Protocols_Media_Access.html#efi-file-protocol-read
says:

> EFI_DEVICE_ERROR     On entry, the current file position is beyond the
end of the file.

while the standard does not say SetPosition() can error out for bad seeks.

So I think we should allow this in SetPosition() and error out in Read().
Does this look good to you?

Pedro

[-- Attachment #2: Type: text/html, Size: 1945 bytes --]

Re: [edk2-platforms][PATCH v1 09/12] Ext4Pkg: Correct integer overflow check on multiplication in DiskUtil

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:

> Multiplication overflow could result into small numbers, so we need also
> check it
>

Can you fix the commit message? Overflow was (in theory) already getting
checked, I just did a remarkably poor job open-coding it.

Pedro

[-- Attachment #2: Type: text/html, Size: 668 bytes --]

Re: [edk2-platforms][PATCH v1 00/12] Ext4Pkg: Code correctness and security improvements

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:

> Hi all,
>
> This patchset fixes several code problems found by fuzzing Ext4Dxe like
> buffer and integer overflows, memory leaks, logic bugs and so on.
>
> REF: https://github.com/savvamitrofanov/edk2-platforms/tree/master
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>
> Savva Mitrofanov (12):
>   Ext4Pkg: Fix memory leak in Ext4RetrieveDirent
>   Ext4Pkg: Move EXT4_NAME_MAX definition to Ext4Disk.h
>   Ext4Pkg: Fix global buffer overflow in Ext4ReadDir
>   Ext4Pkg: Fix incorrect checksum metadata feature check
>   Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group
>   Ext4Pkg: Add comparison between Position and FileSize in
>     Ext4SetPosition
>   Ext4Pkg: Add inode number validity check
>   Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock
>   Ext4Pkg: Correct integer overflow check on multiplication in DiskUtil
>   Ext4Pkg: Check that source file is directory in Ext4OpenInternal
>   Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName
>   Ext4Pkg: Add missing exit Status in Ext4OpenDirent
>

Hi!

Thanks for the patches (and the fuzzing!). They all mostly lgtm, just some
small nits. Please fix them so I can test and merge.

Also, could you add a Fixes tag to each patch (like in the LKML and
elsewhere in OVMF) so we can more easily track what each patch fixes? Using
something simple like the oldest git blame of what you're fixing should be
enough in this case, no need for git bisect. I just want to establish a
good, clean track record here for me and for downstream users to better
know what they need to pick up!

Thanks,
Pedro

[-- Attachment #2: Type: text/html, Size: 2586 bytes --]

Re: [edk2-platforms][PATCH v1 06/12] Ext4Pkg: Add comparison between Position and FileSize in Ext4SetPosition

[Savva Mitrofanov]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

Seems I misunderstood the usage of SetPosition, thanks for pointing out.  So we can just drop this commit
and keep everything as is, because this check is already present in Ext4Read.

Savva Mitrofanov

> On 10 Dec 2022, at 04:12, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
> Missing such comparison leads to infinite loop states, for example code
> which trying to read entire file can easily get out of bound of
> file size by passing position value which exceeds file size without this
> check. So we need to add there missing comparison between the desired
> position to be set and file size
> 
> +  FileSize = EXT4_INODE_SIZE (File->Inode);
> +
>    // -1 (0xffffff.......) seeks to the end of the file
>    if (Position == (UINT64)-1) {
> -    Position = EXT4_INODE_SIZE (File->Inode);
> +    Position = FileSize;
> +  } else if (Position > FileSize) {
> +    DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to #%Lx of %Lx\n", Position, FileSize));
> +    return EFI_UNSUPPORTED;
>    }
> 
>    File->Position = Position;
> 
> On further inspection, this case is covered in the UEFI spec.
> 
> https://uefi.org/specs/UEFI/2.10/13_Protocols_Media_Access.html#efi-file-protocol-read <https://uefi.org/specs/UEFI/2.10/13_Protocols_Media_Access.html#efi-file-protocol-read> says:
> 
> > EFI_DEVICE_ERROR     On entry, the current file position is beyond the end of the file.
> 
> while the standard does not say SetPosition() can error out for bad seeks.
> 
> So I think we should allow this in SetPosition() and error out in Read(). Does this look good to you?
>  
> Pedro


[-- Attachment #2: Type: text/html, Size: 3220 bytes --]

Re: [edk2-platforms][PATCH v1 00/12] Ext4Pkg: Code correctness and security improvements

[Savva Mitrofanov]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

Hi! 

Thanks for your review, I did changes in my branch of edk2-platforms and will send corrected patchset soon.

Best regards,
Savva Mitrofanov

> On 10 Dec 2022, at 04:28, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
> Hi all,
> 
> This patchset fixes several code problems found by fuzzing Ext4Dxe like
> buffer and integer overflows, memory leaks, logic bugs and so on.
> 
> REF: https://github.com/savvamitrofanov/edk2-platforms/tree/master <https://github.com/savvamitrofanov/edk2-platforms/tree/master>
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
> Cc: Pedro Falcato <pedro.falcato@gmail.com <mailto:pedro.falcato@gmail.com>>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
> 
> Savva Mitrofanov (12):
>   Ext4Pkg: Fix memory leak in Ext4RetrieveDirent
>   Ext4Pkg: Move EXT4_NAME_MAX definition to Ext4Disk.h
>   Ext4Pkg: Fix global buffer overflow in Ext4ReadDir
>   Ext4Pkg: Fix incorrect checksum metadata feature check
>   Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group
>   Ext4Pkg: Add comparison between Position and FileSize in
>     Ext4SetPosition
>   Ext4Pkg: Add inode number validity check
>   Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock
>   Ext4Pkg: Correct integer overflow check on multiplication in DiskUtil
>   Ext4Pkg: Check that source file is directory in Ext4OpenInternal
>   Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName
>   Ext4Pkg: Add missing exit Status in Ext4OpenDirent
> 
> Hi!
> 
> Thanks for the patches (and the fuzzing!). They all mostly lgtm, just some small nits. Please fix them so I can test and merge.
> 
> Also, could you add a Fixes tag to each patch (like in the LKML and elsewhere in OVMF) so we can more easily track what each patch fixes? Using something simple like the oldest git blame of what you're fixing should be enough in this case, no need for git bisect. I just want to establish a good, clean track record here for me and for downstream users to better know what they need to pick up!
> 
> Thanks,
> Pedro


[-- Attachment #2: Type: text/html, Size: 3841 bytes --]