[edk2-platforms][PATCH v3 00/11] Ext4Pkg: Code correctness and security improvements

[edk2-platforms][PATCH v3 00/11] Ext4Pkg: Code correctness and security improvements

[Savva Mitrofanov]

Hi all,

In v3 I rebased patches according upstream and removed already applied patches.
Also in this revision I corrected 'Fixes' tag formatting, added corrections to
Ext4GetUcs2DirentName to filter out directory entry names containing \0 as invalid
and added a fix for building using MSVC.

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 (11):
  Ext4Pkg: Fix memory leak in Ext4RetrieveDirent
  Ext4Pkg: Fix incorrect checksum metadata feature check
  Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group
  Ext4Pkg: Add inode number validity check
  Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock
  Ext4Pkg: Corrects integer overflow check logic 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
  Ext4Pkg: Fixes build on MSVC
  Ext4Pkg: Filter out directory entry names containing \0 as invalid

 Features/Ext4Pkg/Ext4Pkg.dsc          |  2 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 13 ++++-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 39 +++++++++++++++
 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 52 ++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/DiskUtil.c   |  8 +--
 Features/Ext4Pkg/Ext4Dxe/File.c       | 23 ++++++---
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |  6 +--
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 16 ++++--
 Features/Ext4Pkg/Ext4Dxe/Symlink.c    | 12 ++---
 10 files changed, 134 insertions(+), 42 deletions(-)

-- 
2.39.0

[edk2-platforms][PATCH v3 01/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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 456916453952..f80b1aacd459 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -113,8 +113,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;
@@ -128,7 +127,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) {
@@ -137,8 +137,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; ) {
@@ -146,19 +145,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
@@ -193,8 +192,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;
@@ -203,8 +202,11 @@ Ext4RetrieveDirent (
     Off += Partition->BlockSize;
   }
 
+  Status = EFI_NOT_FOUND;
+
+Out:
   FreePool (Buf);
-  return EFI_NOT_FOUND;
+  return Status;
 }
 
 /**
-- 
2.39.0

[edk2-platforms][PATCH v3 02/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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 5a3c7f478187..35dcf3c007c8 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.39.0

[edk2-platforms][PATCH v3 03/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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 35dcf3c007c8..be3527e4d618 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.39.0

[edk2-platforms][PATCH v3 04/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 13 ++++++++--
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 25 ++++++++++++++++++++
 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++++
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 10 ++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index d0a455d0e572..70cb6c3209dd 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -484,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
-#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 f608def7c9eb..2e489ce4dd86 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 f80b1aacd459..2e9a58a7e329 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -164,6 +164,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);
@@ -510,6 +514,12 @@ Ext4ReadDir (
     // (which we should not expose to ReadDir).
     ShouldSkip = Entry.inode == 0 || Entry.name_len == 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.39.0

[edk2-platforms][PATCH v3 05/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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 2e489ce4dd86..a23323319a59 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 be3527e4d618..3f56de93c105 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.39.0

[edk2-platforms][PATCH v3 06/11] Ext4Pkg: Corrects integer overflow check logic in DiskUtil

[Savva Mitrofanov]

Corrects multiplication overflow check code

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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.39.0

[edk2-platforms][PATCH v3 07/11] 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>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
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 8dfe324255f4..9dde4a5d1a2d 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.39.0

[edk2-platforms][PATCH v3 08/11] 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>
Fixes: cfbbae595eec ("Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().")
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 9dde4a5d1a2d..677caf88fbdc 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -719,7 +719,11 @@ Ext4GetVolumeName (
 
     VolNameLength = StrLen (VolumeName);
   } else {
-    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
+    VolumeName = AllocateZeroPool (sizeof (CHAR16));
+    if (VolumeName == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
     VolNameLength = 0;
   }
 
@@ -786,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);
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index e44b5638599f..90e3eb88f523 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.39.0

[edk2-platforms][PATCH v3 09/11] 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>
Fixes: 21b1853880d5 ("Ext4Pkg: Add a directory entry tree.")
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 2e9a58a7e329..0753a20b5377 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -267,7 +267,8 @@ Ext4OpenDirent (
   } else {
     File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
 
-    if (!File->Dentry) {
+    if (File->Dentry == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
       goto Error;
     }
   }
-- 
2.39.0

[edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC

[Savva Mitrofanov]

Accessing array using index of uint64 type makes MSVC compiler to
include `__allmul` function in NOOPT which is not referenced in IA32.
So we null-terminates string using ReadSize, which should be equal to
SymlinkSizeTmp after correct reading. Also adds missing MultU64x32
in Ext4Read.

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Fixes: 7c46116b0e18 ("Ext4Pkg: Add ext2/3 support")
Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links support")
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Inode.c   |  4 ++--
 Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 90e3eb88f523..8db051d3c444 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -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);
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
index 19b357ac6ba0..8b1511a38b55 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c
+++ b/Features/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,11 +155,6 @@ Ext4ReadSlowSymlink (
     return Status;
   }
 
-  //
-  // Add null-terminator
-  //
-  SymlinkTmp[SymlinkSizeTmp] = '\0';
-
   if (SymlinkSizeTmp != ReadSize) {
     DEBUG ((
       DEBUG_FS,
@@ -168,6 +163,11 @@ Ext4ReadSlowSymlink (
     return EFI_VOLUME_CORRUPTED;
   }
 
+  //
+  // Add null-terminator
+  //
+  SymlinkTmp[ReadSize] = '\0';
+
   *AsciiSymlinkSize = SymlinkAllocateSize;
   *AsciiSymlink     = SymlinkTmp;
 
-- 
2.39.0

[edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Savva Mitrofanov]

The directory entry name conventions forbid having null-terminator
symbols in its body and can lead to undefined behavior conditions
and crashes

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 0753a20b5377..465749c9b51d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
 {
   CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
   UINT16      *Str;
+  UINTN       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';
 
-- 
2.39.0

Re: [edk2-platforms][PATCH v3 02/11] Ext4Pkg: Fix incorrect checksum metadata feature check

[Marvin Häuser]

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> 
> Missing comparison != 0 leads to broken logic condition.

The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.

> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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 5a3c7f478187..35dcf3c007c8 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.39.0
> 

Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Marvin Häuser]

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> 
> The directory entry name conventions forbid having null-terminator
> symbols in its body and can lead to undefined behavior conditions
> and crashes
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> index 0753a20b5377..465749c9b51d 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
> {
>   CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
>   UINT16      *Str;
> +  UINTN       Index;

I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.

>   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';
> 
> -- 
> 2.39.0
> 

Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Pedro Falcato]

On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> >
> > The directory entry name conventions forbid having null-terminator
> > symbols in its body and can lead to undefined behavior conditions
> > and crashes
> >
> > Cc: Marvin Häuser <mhaeuser@posteo.de>
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> > Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
> > Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> > ---
> > Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> > index 0753a20b5377..465749c9b51d 100644
> > --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> > +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> > @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
> > {
> >   CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
> >   UINT16      *Str;
> > +  UINTN       Index;
>
> I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.
Considering this is a size (length of name) and it gets explicitly
bounded by name_len (a UINT8) I'm okay with it. But if for some reason
you need to submit a v4, please change.

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 01/11] Ext4Pkg: Fix memory leak in Ext4RetrieveDirent

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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 456916453952..f80b1aacd459 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -113,8 +113,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;
> @@ -128,7 +127,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) {
> @@ -137,8 +137,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; ) {
> @@ -146,19 +145,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
> @@ -193,8 +192,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;
> @@ -203,8 +202,11 @@ Ext4RetrieveDirent (
>      Off += Partition->BlockSize;
>    }
>
> +  Status = EFI_NOT_FOUND;
> +
> +Out:
>    FreePool (Buf);
> -  return EFI_NOT_FOUND;
> +  return Status;
>  }
>
>  /**
> --
> 2.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
-- 
Pedro

Re: [edk2-platforms][PATCH v3 03/11] Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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 35dcf3c007c8..be3527e4d618 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.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Marvin Häuser]

> On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
>>> 
>>> The directory entry name conventions forbid having null-terminator
>>> symbols in its body and can lead to undefined behavior conditions
>>> and crashes
>>> 
>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>>> ---
>>> Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>> index 0753a20b5377..465749c9b51d 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>> @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
>>> {
>>>  CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
>>>  UINT16      *Str;
>>> +  UINTN       Index;
>> 
>> I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.
> Considering this is a size (length of name) and it gets explicitly

Well, being a "size" is not sufficient, I explicitly referred to buffer sizes. Say, you get an arbitrarily long buffer from a caller, its size parameter should probably be UINTN, to be able to optimally leverage the platform memory and be flexible. This is not a buffer size, this is a bounded index of an architecture-agnostic data structure. I simply do not want behaviour that depends on the host architecture in architecture-agnostic code for no reason. We actually had multiple prior examples of bugs due to this, though this is obviously fine (thus not demanding it's changed).

> bounded by name_len (a UINT8) I'm okay with it. But if for some reason
> you need to submit a v4, please change.
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 01/11] Ext4Pkg: Fix memory leak in Ext4RetrieveDirent

[Marvin Häuser]

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

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>

> On 27. Jan 2023, at 15:12, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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 456916453952..f80b1aacd459 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> @@ -113,8 +113,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;
>> @@ -128,7 +127,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) {
>> @@ -137,8 +137,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; ) {
>> @@ -146,19 +145,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
>> @@ -193,8 +192,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;
>> @@ -203,8 +202,11 @@ Ext4RetrieveDirent (
>>     Off += Partition->BlockSize;
>>   }
>> 
>> +  Status = EFI_NOT_FOUND;
>> +
>> +Out:
>>   FreePool (Buf);
>> -  return EFI_NOT_FOUND;
>> +  return Status;
>> }
>> 
>> /**
>> --
>> 2.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> -- 
> Pedro


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

Re: [edk2-platforms][PATCH v3 03/11] Ext4Pkg: Fix division by zero by adding check for s_inodes_per_group

[Marvin Häuser]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:13, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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 35dcf3c007c8..be3527e4d618 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.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 04/11] Ext4Pkg: Add inode number validity check

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 13 ++++++++--
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 25 ++++++++++++++++++++
>  Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++++
>  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 10 ++++++++
>  4 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index d0a455d0e572..70cb6c3209dd 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -484,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
> -#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 f608def7c9eb..2e489ce4dd86 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;
> +  }
> +

I don't know how to feel about this patch.
I do not understand why we need this here (and below). Given
Ext4OpenDirent, how is this deref'ing a NULL pointer without this
check?
Has this been handled by the UTF8 patches and your \0 patch?

-- 
Pedro

Re: [edk2-platforms][PATCH v3 05/11] Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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 2e489ce4dd86..a23323319a59 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.
Nit: s/page size/large page size/
I can change this for you when pushing, no need for a v4 on this one.
> +// 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 be3527e4d618..3f56de93c105 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.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 05/11] Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock

[Marvin Häuser]

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

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:22, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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 2e489ce4dd86..a23323319a59 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.
> Nit: s/page size/large page size/
> I can change this for you when pushing, no need for a v4 on this one.
>> +// 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 be3527e4d618..3f56de93c105 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.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com <mailto:pedro.falcato@gmail.com>>
> 
> -- 
> Pedro


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

Re: [edk2-platforms][PATCH v3 06/11] Ext4Pkg: Corrects integer overflow check logic in DiskUtil

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> Corrects multiplication overflow check code
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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
> -
> +
Why this whitespace change?
>    #
>    # 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.39.0
>

I don't really like open coding this, but it does fix my buggy checks.

In the interest of not adding a new overengineered safe int library,

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>


-- 
Pedro

Re: [edk2-platforms][PATCH v3 07/11] Ext4Pkg: Check that source file is directory in Ext4OpenInternal

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> 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 8dfe324255f4..9dde4a5d1a2d 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.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-devel] [edk2-platforms][PATCH v3 08/11] Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: cfbbae595eec ("Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().")
> 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 9dde4a5d1a2d..677caf88fbdc 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -719,7 +719,11 @@ Ext4GetVolumeName (
>
>
>      VolNameLength = StrLen (VolumeName);
>
>    } else {
>
> -    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
>
> +    VolumeName = AllocateZeroPool (sizeof (CHAR16));
>
> +    if (VolumeName == NULL) {
>
> +      return EFI_OUT_OF_RESOURCES;
>
> +    }
>
> +
>
>      VolNameLength = 0;
>
>    }
>
>
>
> @@ -786,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);
>
>
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index e44b5638599f..90e3eb88f523 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.39.0
>

Embarrassing...
Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 09/11] Ext4Pkg: Add missing exit Status in Ext4OpenDirent

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> 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>
> Fixes: 21b1853880d5 ("Ext4Pkg: Add a directory entry tree.")
> 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 2e9a58a7e329..0753a20b5377 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -267,7 +267,8 @@ Ext4OpenDirent (
>    } else {
>      File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
>
> -    if (!File->Dentry) {
> +    if (File->Dentry == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
>        goto Error;
>      }
>    }
> --
> 2.39.0
>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 02/11] Ext4Pkg: Fix incorrect checksum metadata feature check

[Pedro Falcato]

On Fri, Jan 27, 2023 at 10:02 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> >
> > Missing comparison != 0 leads to broken logic condition.
>
> The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.

+1, the actual issue comes from using the wrong feature mask and not the != 0.
If we get a v4, please update, else I can rewrite it myself.

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

Re: [edk2-platforms][PATCH v3 07/11] Ext4Pkg: Check that source file is directory in Ext4OpenInternal

[Marvin Häuser]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:26, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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 8dfe324255f4..9dde4a5d1a2d 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.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC

[Pedro Falcato]

On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> Accessing array using index of uint64 type makes MSVC compiler to
> include `__allmul` function in NOOPT which is not referenced in IA32.
> So we null-terminates string using ReadSize, which should be equal to
> SymlinkSizeTmp after correct reading. Also adds missing MultU64x32
> in Ext4Read.
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: 7c46116b0e18 ("Ext4Pkg: Add ext2/3 support")
> Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links support")
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Inode.c   |  4 ++--
>  Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 90e3eb88f523..8db051d3c444 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -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);
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
> index 19b357ac6ba0..8b1511a38b55 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c
> +++ b/Features/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,11 +155,6 @@ Ext4ReadSlowSymlink (
>      return Status;
>    }
>
> -  //
> -  // Add null-terminator
> -  //
> -  SymlinkTmp[SymlinkSizeTmp] = '\0';
> -
>    if (SymlinkSizeTmp != ReadSize) {
>      DEBUG ((
>        DEBUG_FS,
> @@ -168,6 +163,11 @@ Ext4ReadSlowSymlink (
>      return EFI_VOLUME_CORRUPTED;
>    }
>
> +  //
> +  // Add null-terminator
> +  //
Sidenote: please don't use this comment style, I know it's prevalent
in EDK2 and EDK2 platforms but Ext4Pkg just does:
// Comment

Also, why are you bringing this null-termination down here?
> +  SymlinkTmp[ReadSize] = '\0';
> +
>    *AsciiSymlinkSize = SymlinkAllocateSize;
>    *AsciiSymlink     = SymlinkTmp;
>
> --
> 2.39.0
>


-- 
Pedro

Re: [edk2-devel] [edk2-platforms][PATCH v3 08/11] Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName

[Marvin Häuser]

This code makes me wish for an in-place conversion lib, there really is no reason for dynamic allocation here...

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:27, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: cfbbae595eec ("Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().")
>> 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 9dde4a5d1a2d..677caf88fbdc 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
>> @@ -719,7 +719,11 @@ Ext4GetVolumeName (
>> 
>> 
>>     VolNameLength = StrLen (VolumeName);
>> 
>>   } else {
>> 
>> -    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
>> 
>> +    VolumeName = AllocateZeroPool (sizeof (CHAR16));
>> 
>> +    if (VolumeName == NULL) {
>> 
>> +      return EFI_OUT_OF_RESOURCES;
>> 
>> +    }
>> 
>> +
>> 
>>     VolNameLength = 0;
>> 
>>   }
>> 
>> 
>> 
>> @@ -786,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);
>> 
>> 
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index e44b5638599f..90e3eb88f523 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.39.0
>> 
> 
> Embarrassing...
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 09/11] Ext4Pkg: Add missing exit Status in Ext4OpenDirent

[Marvin Häuser]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:28, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: 21b1853880d5 ("Ext4Pkg: Add a directory entry tree.")
>> 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 2e9a58a7e329..0753a20b5377 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> @@ -267,7 +267,8 @@ Ext4OpenDirent (
>>   } else {
>>     File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
>> 
>> -    if (!File->Dentry) {
>> +    if (File->Dentry == NULL) {
>> +      Status = EFI_OUT_OF_RESOURCES;
>>       goto Error;
>>     }
>>   }
>> --
>> 2.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC

[Marvin Häuser]

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

On 27. Jan 2023, at 15:33, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
>> 
>> Accessing array using index of uint64 type makes MSVC compiler to
>> include `__allmul` function in NOOPT which is not referenced in IA32.
>> So we null-terminates string using ReadSize, which should be equal to
>> SymlinkSizeTmp after correct reading. Also adds missing MultU64x32
>> in Ext4Read.
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: 7c46116b0e18 ("Ext4Pkg: Add ext2/3 support")
>> Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links support")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Inode.c   |  4 ++--
>> Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 ++++++------
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index 90e3eb88f523..8db051d3c444 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> @@ -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);
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
>> index 19b357ac6ba0..8b1511a38b55 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c
>> +++ b/Features/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,11 +155,6 @@ Ext4ReadSlowSymlink (
>>     return Status;
>>   }
>> 
>> -  //
>> -  // Add null-terminator
>> -  //
>> -  SymlinkTmp[SymlinkSizeTmp] = '\0';
>> -
>>   if (SymlinkSizeTmp != ReadSize) {
>>     DEBUG ((
>>       DEBUG_FS,
>> @@ -168,6 +163,11 @@ Ext4ReadSlowSymlink (
>>     return EFI_VOLUME_CORRUPTED;
>>   }
>> 
>> +  //
>> +  // Add null-terminator
>> +  //
> Sidenote: please don't use this comment style, I know it's prevalent
> in EDK2 and EDK2 platforms but Ext4Pkg just does:
> // Comment
> 
> Also, why are you bringing this null-termination down here?

I don't think there's a strong functional reason, but it adheres to the principle of not touching malformed data if at all possible. Not sure it needs to be part of this particular commit, but why not, it causes less noise this way.

>> +  SymlinkTmp[ReadSize] = '\0';
>> +
>>   *AsciiSymlinkSize = SymlinkAllocateSize;
>>   *AsciiSymlink     = SymlinkTmp;
>> 
>> --
>> 2.39.0
>> 
> 
> 
> -- 
> Pedro


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

Re: [edk2-platforms][PATCH v3 06/11] Ext4Pkg: Corrects integer overflow check logic in DiskUtil

[Savva Mitrofanov]

> Why this whitespace change?

Seems code formatter just removed trailing space. If you want so, I can drop this change in v4.

> On 27 Jan 2023, at 20:24, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> Corrects multiplication overflow check code
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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
>> -
>> +
> Why this whitespace change?
>>   #
>>   # 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.39.0
>> 
> 
> I don't really like open coding this, but it does fix my buggy checks.
> 
> In the interest of not adding a new overengineered safe int library,
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 05/11] Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock

[Savva Mitrofanov]

Thanks, I corrected this in the referenced repository fork.
Will be included in v4.

> On 27 Jan 2023, at 20:22, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> 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 2e489ce4dd86..a23323319a59 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.
> Nit: s/page size/large page size/
> I can change this for you when pushing, no need for a v4 on this one.
>> +// 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 be3527e4d618..3f56de93c105 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.39.0
>> 
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 06/11] Ext4Pkg: Corrects integer overflow check logic in DiskUtil

[Pedro Falcato]

On Fri, Jan 27, 2023 at 4:10 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>
> > Why this whitespace change?
>
> Seems code formatter just removed trailing space. If you want so, I can drop this change in v4.

No need, I just couldn't see what changed here :) No trailing space = good.

-- 
Pedro

Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Savva Mitrofanov]

It is not so important from my point of view, however, I corrected this in the referenced repository fork.

> On 27 Jan 2023, at 16:04, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> The directory entry name conventions forbid having null-terminator
>> symbols in its body and can lead to undefined behavior conditions
>> and crashes
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> index 0753a20b5377..465749c9b51d 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
>> {
>>  CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
>>  UINT16      *Str;
>> +  UINTN       Index;
> 
> I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.
> 
>>  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';
>> 
>> -- 
>> 2.39.0
>> 
> 

Re: [edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC

[Marvin Häuser]

[-- Attachment #1: Type: text/html, Size: 12420 bytes --]

Re: [edk2-platforms][PATCH v3 02/11] Ext4Pkg: Fix incorrect checksum metadata feature check

[Marvin Häuser]

With the commit message changed,

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:29, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 10:02 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
>>> 
>>> Missing comparison != 0 leads to broken logic condition.
>> 
>> The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.
> 
> +1, the actual issue comes from using the wrong feature mask and not the != 0.
> If we get a v4, please update, else I can rewrite it myself.
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro

Re: [edk2-platforms][PATCH v3 02/11] Ext4Pkg: Fix incorrect checksum metadata feature check

[Savva Mitrofanov]

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

Thanks for pointing this, yes, this change actually replaces structure field from FeaturesCompat to FeaturesRoCompat. The commit message was already corrected in referenced repository fork.

> On 27 Jan 2023, at 16:02, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.


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

Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid

[Marvin Häuser]

As Pedro agreed, forgot to add

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

@Pedro feel free to change to UINT8 on push nevertheless. :)

> On 27. Jan 2023, at 15:14, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> 
> 
>> On 27. Jan 2023, at 15:09, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>> 
>>> On Fri, Jan 27, 2023 at 10:04 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>>> 
>>> On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@gmail.com> wrote:
>>>> 
>>>> The directory entry name conventions forbid having null-terminator
>>>> symbols in its body and can lead to undefined behavior conditions
>>>> and crashes
>>>> 
>>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>>>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-utf8 dir entries")
>>>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>>>> ---
>>>> Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++-
>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>>> index 0753a20b5377..465749c9b51d 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>>>> @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName (
>>>> {
>>>> CHAR8       Utf8NameBuf[EXT4_NAME_MAX + 1];
>>>> UINT16      *Str;
>>>> +  UINTN       Index;
>>> 
>>> I *really* do not like UINTN in code that does not deal with buffer addresses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro.
>> Considering this is a size (length of name) and it gets explicitly
> 
> Well, being a "size" is not sufficient, I explicitly referred to buffer sizes. Say, you get an arbitrarily long buffer from a caller, its size parameter should probably be UINTN, to be able to optimally leverage the platform memory and be flexible. This is not a buffer size, this is a bounded index of an architecture-agnostic data structure. I simply do not want behaviour that depends on the host architecture in architecture-agnostic code for no reason. We actually had multiple prior examples of bugs due to this, though this is obviously fine (thus not demanding it's changed).
> 
>> bounded by name_len (a UINT8) I'm okay with it. But if for some reason
>> you need to submit a v4, please change.
>> 
>> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
>> 
>> -- 
>> Pedro
> 

Re: [edk2-platforms][PATCH v3 04/11] Ext4Pkg: Add inode number validity check

[Savva Mitrofanov]

Yes, I checked this out. Your UTF8 patches and latest patch
which redirects '..' folder to proper '/' with my directory entry '\0' patch solves the problem.
However, we need to perform inode number validation at least in Ext4ReadInode.
As we discussed, we can do this in a simplified way. These changes will be in v4

Thanks!

> On 27 Jan 2023, at 20:19, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> 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>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   | 13 ++++++++--
>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 25 ++++++++++++++++++++
>> Features/Ext4Pkg/Ext4Dxe/BlockGroup.c |  5 ++++
>> Features/Ext4Pkg/Ext4Dxe/Directory.c  | 10 ++++++++
>> 4 files changed, 51 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> index d0a455d0e572..70cb6c3209dd 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> @@ -484,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
>> -#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 f608def7c9eb..2e489ce4dd86 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;
>> +  }
>> +
> 
> I don't know how to feel about this patch.
> I do not understand why we need this here (and below). Given
> Ext4OpenDirent, how is this deref'ing a NULL pointer without this
> check?
> Has this been handled by the UTF8 patches and your \0 patch?
> 
> -- 
> Pedro