[edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs

[edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs

[Pedro Falcato]

This patch-series addresses bugs found when testing the filesystem with
more complex usage of the file protocol, particularly through the shell
itself.

This is version 2 of the patch series and addresses feedback received
from the community. This version also adds two new patches to further
improve Ext4Dxe and make it more resilient and ready to be used.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Pedro Falcato (5):
  Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
  Ext4Pkg: Hide "." and ".." entries from Read() callers.
  Ext4Pkg: Add a directory entry tree.
  Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().
  Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.

 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 343 ++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    |   1 -
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  88 ++++++-
 Features/Ext4Pkg/Ext4Dxe/File.c       | 202 +++++++++++----
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |   3 +-
 Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
 Features/Ext4Pkg/Ext4Dxe/Superblock.c |  23 +-
 8 files changed, 534 insertions(+), 136 deletions(-)

-- 
2.33.0

[edk2-platforms PATCH v2 1/5] Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.

[Pedro Falcato]

Fixes bug triggered by ShellPkg code, in usage of EFI_FILE_PROTOCOL's
Open().

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/File.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index 0f5fa6f73fb6..a3eff2b48a07 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -207,6 +207,11 @@ Ext4Open (
 
     FileName += Length;
 
+    if (StrCmp(PathSegment, L".") == 0) {
+      // Opens of "." are a no-op
+      continue;
+    }
+
     DEBUG ((DEBUG_FS, "[ext4] Opening %s\n", PathSegment));
 
     if (!Ext4FileIsDir (Current)) {
@@ -512,12 +517,20 @@ Ext4GetFileInfo (
   IN EXT4_FILE *File, OUT EFI_FILE_INFO *Info, IN OUT UINTN *BufferSize
   )
 {
-  UINTN  FileNameLen;
-  UINTN  FileNameSize;
-  UINTN  NeededLength;
+  UINTN         FileNameLen;
+  UINTN         FileNameSize;
+  UINTN         NeededLength;
+  CONST CHAR16  *FileName;
 
-  FileNameLen  = StrLen (File->FileName);
-  FileNameSize = StrSize (File->FileName);
+  if (File->InodeNum == 2) {
+    // Root inode gets a filename of "", regardless of how it was opened.
+    FileName = L"";
+  } else {
+    FileName = File->FileName;
+  }
+
+  FileNameLen  = StrLen (FileName);
+  FileNameSize = StrSize (FileName);
 
   NeededLength = SIZE_OF_EFI_FILE_INFO + FileNameSize;
 
@@ -540,7 +553,7 @@ Ext4GetFileInfo (
 
   *BufferSize = NeededLength;
 
-  return StrCpyS (Info->FileName, FileNameLen + 1, File->FileName);
+  return StrCpyS (Info->FileName, FileNameLen + 1, FileName);
 }
 
 /**
@@ -687,6 +700,7 @@ Ext4DuplicateFile (
 {
   EXT4_PARTITION  *Partition;
   EXT4_FILE       *File;
+  EFI_STATUS      Status;
 
   Partition = Original->Partition;
   File = AllocateZeroPool (sizeof (EXT4_FILE));
@@ -717,7 +731,8 @@ Ext4DuplicateFile (
   File->InodeNum = Original->InodeNum;
   File->OpenMode = 0; // Will be filled by other code
 
-  if (!Ext4InitExtentsMap (File)) {
+  Status = Ext4InitExtentsMap (File);
+  if (EFI_ERROR (Status)) {
     FreePool (File->FileName);
     FreePool (File->Inode);
     FreePool (File);
-- 
2.33.0

[edk2-platforms PATCH v2 2/5] Ext4Pkg: Hide "." and ".." entries from Read() callers.

[Pedro Falcato]

This makes it so callers that may expect FAT32 filesystems (most do)
have more normal looking ReadDir() results.
This commit also presents a better filename for files opened through
Open(".").

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 45 +++++++++++++++++++++++-----
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   |  4 ++-
 Features/Ext4Pkg/Ext4Dxe/Inode.c     |  2 +-
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 081c6bf0f435..c85c4df6d5c5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -180,6 +180,7 @@ Ext4RetrieveDirent (
    @param[in]      OpenMode    Mode in which the file is supposed to be open.
    @param[out]     OutFile     Pointer to the newly opened file.
    @param[in]      Entry       Directory entry to be used.
+   @param[in]      Directory   Pointer to the opened directory.
 
    @retval EFI_STATUS          Result of the operation
 **/
@@ -188,12 +189,18 @@ Ext4OpenDirent (
   IN  EXT4_PARTITION  *Partition,
   IN  UINT64          OpenMode,
   OUT EXT4_FILE       **OutFile,
-  IN  EXT4_DIR_ENTRY  *Entry
+  IN  EXT4_DIR_ENTRY  *Entry,
+  IN  EXT4_FILE       *Directory
   )
 {
   EFI_STATUS  Status;
-  CHAR16      FileName[EXT4_NAME_MAX + 1];
+  CHAR16      FileNameBuf[EXT4_NAME_MAX + 1];
   EXT4_FILE   *File;
+  CHAR16      *FileName;
+  UINTN       DestMax;
+
+  FileName = FileNameBuf;
+  DestMax  = ARRAY_SIZE (FileNameBuf);
 
   File = AllocateZeroPool (sizeof (EXT4_FILE));
 
@@ -202,12 +209,18 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  Status = Ext4GetUcs2DirentName (Entry, FileName);
+  Status = Ext4GetUcs2DirentName (Entry, FileNameBuf);
 
   if (EFI_ERROR (Status)) {
     goto Error;
   }
 
+  if (StrCmp (FileNameBuf, L".") == 0) {
+    // We're using the parent directory's name
+    FileName = Directory->FileName;
+    DestMax  = StrLen (FileName) + 1;
+  }
+
   File->FileName = AllocateZeroPool (StrSize (FileName));
 
   if (!File->FileName) {
@@ -222,7 +235,7 @@ Ext4OpenDirent (
   }
 
   // This should not fail.
-  StrCpyS (File->FileName, ARRAY_SIZE (FileName), FileName);
+  StrCpyS (File->FileName, DestMax, FileName);
 
   File->InodeNum = Entry->inode;
 
@@ -290,7 +303,7 @@ Ext4OpenFile (
     return EFI_NOT_FOUND;
   }
 
-  return Ext4OpenDirent (Partition, OpenMode, OutFile, &Entry);
+  return Ext4OpenDirent (Partition, OpenMode, OutFile, &Entry, Directory);
 }
 
 /**
@@ -429,6 +442,8 @@ Ext4ReadDir (
   UINT32          BlockRemainder;
   EXT4_DIR_ENTRY  Entry;
   EXT4_FILE       *TempFile;
+  BOOLEAN         ShouldSkip;
+  BOOLEAN         IsDotOrDotDot;
 
   DirIno     = File->Inode;
   Status     = EFI_SUCCESS;
@@ -470,13 +485,27 @@ Ext4ReadDir (
       goto Out;
     }
 
-    if (Entry.inode == 0) {
-      // When inode = 0, it's unused
+    // We don't care about passing . or .. entries to the caller of ReadDir(),
+    // since they're generally useless entries *and* may break things if too
+    // many callers assume FAT32.
+
+    // Entry.name_len may be 0 if it's a nameless entry, like an unused entry
+    // or a checksum at the end of the directory block.
+    // memcmp (and CompareMem) return 0 when the passed length is 0.
+
+    IsDotOrDotDot = Entry.name_len != 0 &&
+                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
+                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
+
+    // When inode = 0, it's unused.
+    ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
+
+    if (ShouldSkip) {
       Offset += Entry.rec_len;
       continue;
     }
 
-    Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry);
+    Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
 
     if (EFI_ERROR (Status)) {
       goto Out;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 93f0a8a04add..1aafc60ab57d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -353,6 +353,7 @@ Ext4OpenFile (
    @param[in]      OpenMode    Mode in which the file is supposed to be open.
    @param[out]     OutFile     Pointer to the newly opened file.
    @param[in]      Entry       Directory entry to be used.
+   @param[in]      Directory   Pointer to the opened directory.
 
    @retval EFI_STATUS          Result of the operation
 **/
@@ -361,7 +362,8 @@ Ext4OpenDirent (
   IN  EXT4_PARTITION  *Partition,
   IN  UINT64          OpenMode,
   OUT EXT4_FILE       **OutFile,
-  IN  EXT4_DIR_ENTRY  *Entry
+  IN  EXT4_DIR_ENTRY  *Entry,
+  IN  EXT4_FILE       *Directory
   );
 
 /**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 1bbff9e69f4c..982b19c763d0 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -89,7 +89,7 @@ Ext4Read (
   IN OUT UINTN           *Length
   )
 {
-  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(Offset %lu, Length %lu)\n", Offset, *Length));
+  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->FileName, Offset, *Length));
   EXT4_INODE   *Inode;
   UINT64       InodeSize;
   UINT64       CurrentSeek;
-- 
2.33.0

[edk2-platforms PATCH v2 3/5] Ext4Pkg: Add a directory entry tree.

[Pedro Falcato]

This helps us track directories and directory entries,
which helps us getting a general idea of how the filesystem
looks. In the future, it might serve as a directory cache.
Right now, it only lets us know which name "." and ".." may refer
to, which fixes a EFI_FILE_PROTOCOL::GetInfo() bug that got triggered
by ShellPkg, in some MdePkg code.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 238 +++++++++++++++++++++-----
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  67 +++++++-
 Features/Ext4Pkg/Ext4Dxe/File.c       |  24 +--
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |   3 +-
 Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
 Features/Ext4Pkg/Ext4Dxe/Superblock.c |  23 ++-
 7 files changed, 301 insertions(+), 64 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index c85c4df6d5c5..7d1b2dcfe524 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -194,13 +194,8 @@ Ext4OpenDirent (
   )
 {
   EFI_STATUS  Status;
-  CHAR16      FileNameBuf[EXT4_NAME_MAX + 1];
+  CHAR16      FileName[EXT4_NAME_MAX + 1];
   EXT4_FILE   *File;
-  CHAR16      *FileName;
-  UINTN       DestMax;
-
-  FileName = FileNameBuf;
-  DestMax  = ARRAY_SIZE (FileNameBuf);
 
   File = AllocateZeroPool (sizeof (EXT4_FILE));
 
@@ -209,23 +204,32 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  Status = Ext4GetUcs2DirentName (Entry, FileNameBuf);
+  Status = Ext4GetUcs2DirentName (Entry, FileName);
 
   if (EFI_ERROR (Status)) {
     goto Error;
   }
 
-  if (StrCmp (FileNameBuf, L".") == 0) {
-    // We're using the parent directory's name
-    FileName = Directory->FileName;
-    DestMax  = StrLen (FileName) + 1;
-  }
+  if (StrCmp (FileName, L".") == 0) {
+    // We're using the parent directory's dentry
+    File->Dentry = Directory->Dentry;
 
-  File->FileName = AllocateZeroPool (StrSize (FileName));
+    ASSERT (File->Dentry != NULL);
 
-  if (!File->FileName) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Error;
+    Ext4RefDentry (File->Dentry);
+  } else if (StrCmp (FileName, L"..") == 0) {
+    // Using the parent's parent's dentry
+    File->Dentry = Directory->Dentry->Parent;
+
+    ASSERT (File->Dentry != NULL);
+
+    Ext4RefDentry (File->Dentry);
+  } else {
+    File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
+
+    if (!File->Dentry) {
+      goto Error;
+    }
   }
 
   Status = Ext4InitExtentsMap (File);
@@ -234,9 +238,6 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  // This should not fail.
-  StrCpyS (File->FileName, DestMax, FileName);
-
   File->InodeNum = Entry->inode;
 
   Ext4SetupFile (File, Partition);
@@ -255,8 +256,8 @@ Ext4OpenDirent (
 
 Error:
   if (File != NULL) {
-    if (File->FileName != NULL) {
-      FreePool (File->FileName);
+    if (File->Dentry != NULL) {
+      Ext4UnrefDentry (File->Dentry);
     }
 
     if (File->ExtentsMap != NULL) {
@@ -333,52 +334,47 @@ Ext4OpenVolume (
   OUT EFI_FILE_PROTOCOL               **Root
   )
 {
-  EXT4_INODE  *RootInode;
-  EFI_STATUS  Status;
-  EXT4_FILE   *RootDir;
+  EXT4_INODE      *RootInode;
+  EFI_STATUS      Status;
+  EXT4_FILE       *RootDir;
+  EXT4_PARTITION  *Partition;
 
-  // 2 is always the root inode number in ext4
-  Status = Ext4ReadInode ((EXT4_PARTITION *)This, 2, &RootInode);
+  Partition = (EXT4_PARTITION *)This;
+
+  Status = Ext4ReadInode (Partition, EXT4_ROOT_INODE_NR, &RootInode);
 
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "[ext4] Could not open root inode - status %x\n", Status));
+    DEBUG ((DEBUG_ERROR, "[ext4] Could not open root inode - error %r\n", Status));
     return Status;
   }
 
   RootDir = AllocateZeroPool (sizeof (EXT4_FILE));
 
-  if (!RootDir) {
+  if (RootDir == NULL) {
     FreePool (RootInode);
     return EFI_OUT_OF_RESOURCES;
   }
 
-  // The filename will be "\"(null terminated of course)
-  RootDir->FileName = AllocateZeroPool (2 * sizeof (CHAR16));
-
-  if (!RootDir->FileName) {
-    FreePool (RootDir);
-    FreePool (RootInode);
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  RootDir->FileName[0] = L'\\';
-
   RootDir->Inode    = RootInode;
-  RootDir->InodeNum = 2;
+  RootDir->InodeNum = EXT4_ROOT_INODE_NR;
 
   Status = Ext4InitExtentsMap (RootDir);
 
   if (EFI_ERROR (Status)) {
-    FreePool (RootDir->FileName);
     FreePool (RootInode);
     FreePool (RootDir);
     return EFI_OUT_OF_RESOURCES;
   }
 
-  Ext4SetupFile (RootDir, (EXT4_PARTITION *)This);
+  Ext4SetupFile (RootDir, Partition);
   *Root = &RootDir->Protocol;
 
-  InsertTailList (&((EXT4_PARTITION *)This)->OpenFiles, &RootDir->OpenFilesListNode);
+  InsertTailList (&Partition->OpenFiles, &RootDir->OpenFilesListNode);
+
+  ASSERT (Partition->RootDentry != NULL);
+  RootDir->Dentry = Partition->RootDentry;
+
+  Ext4RefDentry (RootDir->Dentry);
 
   return EFI_SUCCESS;
 }
@@ -525,3 +521,159 @@ Ext4ReadDir (
 Out:
   return Status;
 }
+
+/**
+   Removes a dentry from the other's list.
+
+   @param[in out]            Parent       Pointer to the parent EXT4_DENTRY.
+   @param[in out]            ToBeRemoved  Pointer to the child EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4RemoveDentry (
+  IN OUT EXT4_DENTRY  *Parent,
+  IN OUT EXT4_DENTRY  *ToBeRemoved
+  )
+{
+  EXT4_DENTRY  *D;
+  LIST_ENTRY   *Entry;
+  LIST_ENTRY   *NextEntry;
+
+  BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {
+    D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
+
+    if (D == ToBeRemoved) {
+      RemoveEntryList (Entry);
+      return;
+    }
+  }
+
+  DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for dentry\n"));
+}
+
+/**
+   Adds a dentry to the other's list.
+
+   The dentry that is added to the other one's list gets ->Parent set to Parent,
+   and the parent gets its reference count incremented.
+
+   @param[in out]            Parent       Pointer to the parent EXT4_DENTRY.
+   @param[in out]            ToBeAdded    Pointer to the child EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4AddDentry (
+  IN OUT EXT4_DENTRY  *Parent,
+  IN OUT EXT4_DENTRY  *ToBeAdded
+  )
+{
+  ToBeAdded->Parent = Parent;
+  InsertTailList (&Parent->Children, &ToBeAdded->ListNode);
+  Ext4RefDentry (Parent);
+}
+
+/**
+   Creates a new dentry object.
+
+   @param[in]              Name        Name of the dentry.
+   @param[in out opt]      Parent      Parent dentry, if it's not NULL.
+
+   @return The new allocated and initialised dentry.
+           The ref count will be set to 1.
+**/
+EXT4_DENTRY *
+Ext4CreateDentry (
+  IN CONST CHAR16              *Name,
+  IN OUT EXT4_DENTRY  *Parent  OPTIONAL
+  )
+{
+  EXT4_DENTRY  *Dentry;
+  EFI_STATUS   Status;
+
+  Dentry = AllocateZeroPool (sizeof (EXT4_DENTRY));
+
+  if (Dentry == NULL) {
+    return NULL;
+  }
+
+  Dentry->RefCount = 1;
+
+  // This StrCpyS should not fail.
+  Status = StrCpyS (Dentry->Name, ARRAY_SIZE (Dentry->Name), Name);
+
+  ASSERT_EFI_ERROR (Status);
+
+  InitializeListHead (&Dentry->Children);
+
+  if (Parent != NULL) {
+    Ext4AddDentry (Parent, Dentry);
+  }
+
+  DEBUG ((DEBUG_FS, "[ext4] Created dentry %s\n", Name));
+
+  return Dentry;
+}
+
+/**
+   Increments the ref count of the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+VOID
+Ext4RefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  UINTN  OldRef;
+
+  OldRef = Dentry->RefCount;
+
+  Dentry->RefCount++;
+
+  // I'm not sure if this (Refcount overflow) is a valid concern,
+  // but it's better to be safe than sorry.
+  ASSERT (OldRef < Dentry->RefCount);
+}
+
+/**
+   Deletes the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4DeleteDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  if (Dentry->Parent) {
+    Ext4RemoveDentry (Dentry->Parent, Dentry);
+    Ext4UnrefDentry (Dentry->Parent);
+  }
+
+  DEBUG ((DEBUG_FS, "[ext4] Deleted dentry %s\n", Dentry->Name));
+  FreePool (Dentry);
+}
+
+/**
+   Decrements the ref count of the dentry.
+   If the ref count is 0, it's destroyed.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+
+   @retval True if it was destroyed, false if it's alive.
+**/
+BOOLEAN
+Ext4UnrefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  Dentry->RefCount--;
+
+  if (Dentry->RefCount == 0) {
+    Ext4DeleteDentry (Dentry);
+    return TRUE;
+  }
+
+  return FALSE;
+}
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index b387ebcd36a6..070eb5a9c827 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -451,4 +451,7 @@ typedef struct {
 typedef UINT64  EXT4_BLOCK_NR;
 typedef UINT32  EXT4_INO_NR;
 
+// 2 is always the root inode number in ext4
+#define EXT4_ROOT_INODE_NR  2
+
 #endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 1aafc60ab57d..db938c25244d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -55,6 +55,7 @@ Ext4OpenPartition (
   );
 
 typedef struct _Ext4File EXT4_FILE;
+typedef struct _Ext4_Dentry EXT4_DENTRY;
 
 typedef struct _Ext4_PARTITION {
   EFI_SIMPLE_FILE_SYSTEM_PROTOCOL    Interface;
@@ -81,8 +82,68 @@ typedef struct _Ext4_PARTITION {
   UINT32                             InitialSeed;
 
   LIST_ENTRY                         OpenFiles;
+
+  EXT4_DENTRY                        *RootDentry;
 } EXT4_PARTITION;
 
+/**
+   This structure represents a directory entry inside our directory entry tree.
+   For now, it will be used as a way to track file names inside our opening code,
+   but it may very well be used as a directory cache in the future.
+   Because it's not being used as a directory cache right now,
+   an EXT4_DENTRY structure is not necessarily unique name-wise in the list of
+   children. Therefore, the dentry tree does not accurately reflect the filesystem
+   structure.
+ */
+struct _Ext4_Dentry {
+  UINTN                  RefCount;
+  CHAR16                 Name[EXT4_NAME_MAX + 1];
+  EXT4_INO_NR            Inode;
+  struct _Ext4_Dentry    *Parent;
+  LIST_ENTRY             Children;
+  LIST_ENTRY             ListNode;
+};
+
+#define EXT4_DENTRY_FROM_DENTRY_LIST(Node)  BASE_CR (Node, EXT4_DENTRY, ListNode)
+
+/**
+   Creates a new dentry object.
+
+   @param[in]              Name        Name of the dentry.
+   @param[in out opt]      Parent      Parent dentry, if it's not NULL.
+
+   @return The new allocated and initialised dentry.
+           The ref count will be set to 1.
+**/
+EXT4_DENTRY *
+Ext4CreateDentry (
+  IN CONST CHAR16     *Name,
+  IN OUT EXT4_DENTRY  *Parent  OPTIONAL
+  );
+
+/**
+   Increments the ref count of the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+VOID
+Ext4RefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  );
+
+/**
+   Decrements the ref count of the dentry.
+   If the ref count is 0, it's destroyed.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+
+   @retval True if it was destroyed, false if it's alive.
+**/
+BOOLEAN
+Ext4UnrefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  );
+
 /**
    Opens and parses the superblock.
 
@@ -126,7 +187,7 @@ Ext4OpenSuperblock (
    @param[in]     Partition  Pointer to the opened ext4 partition.
    @return The media ID associated with the partition.
 **/
-#define EXT4_MEDIA_ID(Partition) Partition->BlockIo->Media->MediaId
+#define EXT4_MEDIA_ID(Partition)  Partition->BlockIo->Media->MediaId
 
 /**
    Reads from the partition's disk using the DISK_IO protocol.
@@ -299,11 +360,13 @@ struct _Ext4File {
   UINT64                Position;
 
   EXT4_PARTITION        *Partition;
-  CHAR16                *FileName;
 
   ORDERED_COLLECTION    *ExtentsMap;
 
   LIST_ENTRY            OpenFilesListNode;
+
+  // Owning reference to this file's directory entry.
+  EXT4_DENTRY           *Dentry;
 };
 
 #define EXT4_FILE_FROM_OPEN_FILES_NODE(Node)  BASE_CR (Node, EXT4_FILE, OpenFilesListNode)
diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index a3eff2b48a07..021d10b1edfb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -207,7 +207,7 @@ Ext4Open (
 
     FileName += Length;
 
-    if (StrCmp(PathSegment, L".") == 0) {
+    if (StrCmp (PathSegment, L".") == 0) {
       // Opens of "." are a no-op
       continue;
     }
@@ -272,7 +272,7 @@ Ext4Open (
 
   *NewHandle = &Current->Protocol;
 
-  DEBUG ((DEBUG_FS, "Opened filename %s\n", Current->FileName));
+  DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
   return EFI_SUCCESS;
 }
 
@@ -312,9 +312,9 @@ Ext4CloseInternal (
 
   DEBUG ((DEBUG_FS, "[ext4] Closed file %p (inode %lu)\n", File, File->InodeNum));
   RemoveEntryList (&File->OpenFilesListNode);
-  FreePool (File->FileName);
   FreePool (File->Inode);
   Ext4FreeExtentsMap (File);
+  Ext4UnrefDentry (File->Dentry);
   FreePool (File);
   return EFI_SUCCESS;
 }
@@ -522,11 +522,11 @@ Ext4GetFileInfo (
   UINTN         NeededLength;
   CONST CHAR16  *FileName;
 
-  if (File->InodeNum == 2) {
+  if (File->InodeNum == EXT4_ROOT_INODE_NR) {
     // Root inode gets a filename of "", regardless of how it was opened.
     FileName = L"";
   } else {
-    FileName = File->FileName;
+    FileName = File->Dentry->Name;
   }
 
   FileNameLen  = StrLen (FileName);
@@ -717,15 +717,6 @@ Ext4DuplicateFile (
 
   CopyMem (File->Inode, Original->Inode, Partition->InodeSize);
 
-  File->FileName = AllocateZeroPool (StrSize (Original->FileName));
-  if (File->FileName == NULL) {
-    FreePool (File->Inode);
-    FreePool (File);
-    return NULL;
-  }
-
-  StrCpyS (File->FileName, StrLen (Original->FileName) + 1, Original->FileName);
-
   File->Position = 0;
   Ext4SetupFile (File, Partition);
   File->InodeNum = Original->InodeNum;
@@ -733,12 +724,15 @@ Ext4DuplicateFile (
 
   Status = Ext4InitExtentsMap (File);
   if (EFI_ERROR (Status)) {
-    FreePool (File->FileName);
     FreePool (File->Inode);
     FreePool (File);
     return NULL;
   }
 
+  File->Dentry = Original->Dentry;
+
+  Ext4RefDentry (File->Dentry);
+
   InsertTailList (&Partition->OpenFiles, &File->OpenFilesListNode);
 
   return File;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 982b19c763d0..63cecec1f7cf 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -89,7 +89,6 @@ Ext4Read (
   IN OUT UINTN           *Length
   )
 {
-  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->FileName, Offset, *Length));
   EXT4_INODE   *Inode;
   UINT64       InodeSize;
   UINT64       CurrentSeek;
@@ -116,6 +115,8 @@ Ext4Read (
   RemainingRead = *Length;
   BeenRead      = 0;
 
+  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->Dentry->Name, Offset, *Length));
+
   if (Offset > InodeSize) {
     return EFI_DEVICE_ERROR;
   }
diff --git a/Features/Ext4Pkg/Ext4Dxe/Partition.c b/Features/Ext4Pkg/Ext4Dxe/Partition.c
index 2258bac76a4f..afa0392024ec 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Partition.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Partition.c
@@ -108,6 +108,7 @@ Ext4UnmountAndFreePartition (
   LIST_ENTRY  *Entry;
   LIST_ENTRY  *NextEntry;
   EXT4_FILE   *File;
+  BOOLEAN     DeletedRootDentry;
 
   Partition->Unmounting = TRUE;
   Ext4CloseInternal (Partition->Root);
@@ -118,6 +119,12 @@ Ext4UnmountAndFreePartition (
     Ext4CloseInternal (File);
   }
 
+  DeletedRootDentry = Ext4UnrefDentry (Partition->RootDentry);
+
+  if (!DeletedRootDentry) {
+    DEBUG ((DEBUG_ERROR, "[ext4] Failed to delete root dentry - resource leak present.\n"));
+  }
+
   FreePool (Partition->BlockGroups);
   FreePool (Partition);
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 8231831115fa..c321d8c3d86d 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -167,8 +167,11 @@ Ext4OpenSuperblock (
   // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disasterous.
 
   if (Partition->FeaturesIncompat & ~gSupportedIncompatFeat) {
-    DEBUG ((DEBUG_ERROR, "[ext4] Unsupported features %lx\n",
-            Partition->FeaturesIncompat & ~gSupportedIncompatFeat));
+    DEBUG ((
+      DEBUG_ERROR,
+      "[ext4] Unsupported features %lx\n",
+      Partition->FeaturesIncompat & ~gSupportedIncompatFeat
+      ));
     return EFI_UNSUPPORTED;
   }
 
@@ -247,7 +250,7 @@ Ext4OpenSuperblock (
 
   Partition->BlockGroups = Ext4AllocAndReadBlocks (Partition, NrBlocks, Partition->BlockSize == 1024 ? 2 : 1);
 
-  if (!Partition->BlockGroups) {
+  if (Partition->BlockGroups == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -255,13 +258,27 @@ Ext4OpenSuperblock (
     Desc = Ext4GetBlockGroupDesc (Partition, Index);
     if (!Ext4VerifyBlockGroupDescChecksum (Partition, Desc, Index)) {
       DEBUG ((DEBUG_ERROR, "[ext4] Block group descriptor %u has an invalid checksum\n", Index));
+      FreePool (Partition->BlockGroups);
       return EFI_VOLUME_CORRUPTED;
     }
   }
 
+  // RootDentry will serve as the basis of our directory entry tree.
+  Partition->RootDentry = Ext4CreateDentry (L"\\", NULL);
+
+  if (Partition->RootDentry == NULL) {
+    FreePool (Partition->BlockGroups);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   // Note that the cast below is completely safe, because EXT4_FILE is a specialisation of EFI_FILE_PROTOCOL
   Status = Ext4OpenVolume (&Partition->Interface, (EFI_FILE_PROTOCOL **)&Partition->Root);
 
+  if (EFI_ERROR (Status)) {
+    Ext4UnrefDentry (Partition->RootDentry);
+    FreePool (Partition->BlockGroups);
+  }
+
   return Status;
 }
 
-- 
2.33.0

[edk2-platforms PATCH v2 4/5] Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().

[Pedro Falcato]

This commit adds support for EFI_FILE_SYSTEM_VOLUME_LABEL requests
in GetInfo().

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c |   1 -
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h |  17 ++++
 Features/Ext4Pkg/Ext4Dxe/File.c    | 155 ++++++++++++++++++++++-------
 3 files changed, 138 insertions(+), 35 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index 71360ea894d2..ea2e048d7762 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -6,7 +6,6 @@
 **/
 
 #include "Ext4Dxe.h"
-#include "Uefi/UefiBaseType.h"
 
 GLOBAL_REMOVE_IF_UNREFERENCED EFI_UNICODE_STRING_TABLE  mExt4DriverNameTable[] = {
   {
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index db938c25244d..64eab455db4a 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1100,4 +1100,21 @@ Ext4CalculateBlockGroupDescChecksum (
 #define EXT4_HAS_GDT_CSUM(Partition) \
   EXT4_HAS_RO_COMPAT (Partition, EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)
 
+/**
+   Retrieves the volume name.
+
+   @param[in]      Part           Pointer to the opened partition.
+   @param[out]     Info           Pointer to a CHAR16*.
+   @param[out]     BufferSize     Pointer to a UINTN, where the string length
+                                  of the name will be put.
+
+   @return Status of the volume name request.
+**/
+EFI_STATUS
+Ext4GetVolumeName (
+  IN EXT4_PARTITION  *Partition,
+  OUT CHAR16         **OutVolName,
+  OUT UINTN          *VolNameLen
+  );
+
 #endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index 021d10b1edfb..4ad7cad8dcf5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -35,7 +35,9 @@ Ext4DuplicateFile (
 STATIC
 EFI_STATUS
 GetPathSegment (
-  IN CONST CHAR16 *Path, OUT CHAR16 *PathSegment, OUT UINTN *Length
+  IN CONST CHAR16  *Path,
+  OUT CHAR16       *PathSegment,
+  OUT UINTN        *Length
   )
 {
   CONST CHAR16  *Start;
@@ -514,7 +516,9 @@ Ext4SetPosition (
 **/
 EFI_STATUS
 Ext4GetFileInfo (
-  IN EXT4_FILE *File, OUT EFI_FILE_INFO *Info, IN OUT UINTN *BufferSize
+  IN EXT4_FILE       *File,
+  OUT EFI_FILE_INFO  *Info,
+  IN OUT UINTN       *BufferSize
   )
 {
   UINTN         FileNameLen;
@@ -557,35 +561,33 @@ Ext4GetFileInfo (
 }
 
 /**
-   Retrieves information about the filesystem and stores it in the EFI_FILE_SYSTEM_INFO format.
+   Retrieves the volume name.
 
    @param[in]      Part           Pointer to the opened partition.
-   @param[out]     Info           Pointer to a EFI_FILE_SYSTEM_INFO.
-   @param[in out]  BufferSize     Pointer to the buffer size
+   @param[out]     Info           Pointer to a CHAR16*.
+   @param[out]     BufferSize     Pointer to a UINTN, where the string length
+                                  of the name will be put.
 
-   @return Status of the file information request.
+   @return Status of the volume name request.
 **/
-STATIC
 EFI_STATUS
-Ext4GetFilesystemInfo (
-  IN EXT4_PARTITION *Part, OUT EFI_FILE_SYSTEM_INFO *Info, IN OUT UINTN *BufferSize
+Ext4GetVolumeName (
+  IN EXT4_PARTITION  *Partition,
+  OUT CHAR16         **OutVolName,
+  OUT UINTN          *VolNameLen
   )
 {
-  // Length of s_volume_name + null terminator
-  CHAR8          TempVolName[16 + 1];
-  CHAR16         *VolumeName;
-  UINTN          VolNameLength;
-  EFI_STATUS     Status;
-  UINTN          NeededLength;
-  EXT4_BLOCK_NR  TotalBlocks;
-  EXT4_BLOCK_NR  FreeBlocks;
+  CHAR8       TempVolName[16 + 1];
+  CHAR16      *VolumeName;
+  UINTN       VolNameLength;
+  EFI_STATUS  Status;
 
   VolNameLength = 0;
   VolumeName    = NULL;
 
   // s_volume_name is only valid on dynamic revision; old filesystems don't support this
-  if (Part->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
-    CopyMem (TempVolName, (CONST CHAR8 *)Part->SuperBlock.s_volume_name, 16);
+  if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
+    CopyMem (TempVolName, (CONST CHAR8 *)Partition->SuperBlock.s_volume_name, 16);
     TempVolName[16] = '\0';
 
     Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
@@ -595,23 +597,56 @@ Ext4GetFilesystemInfo (
     }
 
     VolNameLength = StrLen (VolumeName);
+  } else {
+    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
+    VolNameLength = 0;
+  }
+
+  *OutVolName = VolumeName;
+  *VolNameLen = VolNameLength;
+
+  return EFI_SUCCESS;
+}
+
+/**
+   Retrieves information about the filesystem and stores it in the EFI_FILE_SYSTEM_INFO format.
+
+   @param[in]      Part           Pointer to the opened partition.
+   @param[out]     Info           Pointer to a EFI_FILE_SYSTEM_INFO.
+   @param[in out]  BufferSize     Pointer to the buffer size
+
+   @return Status of the file information request.
+**/
+STATIC
+EFI_STATUS
+Ext4GetFilesystemInfo (
+  IN EXT4_PARTITION         *Part,
+  OUT EFI_FILE_SYSTEM_INFO  *Info,
+  IN OUT UINTN              *BufferSize
+  )
+{
+  // Length of s_volume_name + null terminator
+  EFI_STATUS     Status;
+  UINTN          NeededLength;
+  EXT4_BLOCK_NR  TotalBlocks;
+  EXT4_BLOCK_NR  FreeBlocks;
+  CHAR16         *VolumeName;
+  UINTN          VolNameLength;
+
+  Status = Ext4GetVolumeName (Part, &VolumeName, &VolNameLength);
+
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
   NeededLength = SIZE_OF_EFI_FILE_SYSTEM_INFO;
 
-  if (VolumeName != NULL) {
-    NeededLength += StrSize (VolumeName);
-  } else {
-    // If we don't have a volume name, we set VolumeLabel to a single null terminator
-    NeededLength += sizeof (CHAR16);
-  }
+  NeededLength += StrSize (VolumeName);
 
   if (*BufferSize < NeededLength) {
     *BufferSize = NeededLength;
 
-    if (VolumeName != NULL) {
-      FreePool (VolumeName);
-    }
+    FreePool (VolumeName);
 
     return EFI_BUFFER_TOO_SMALL;
   }
@@ -630,16 +665,60 @@ Ext4GetFilesystemInfo (
   Info->VolumeSize = MultU64x32 (TotalBlocks, Part->BlockSize);
   Info->FreeSpace  = MultU64x32 (FreeBlocks, Part->BlockSize);
 
-  if (VolumeName != NULL) {
-    StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
-  } else {
-    Info->VolumeLabel[0] = L'\0';
+  StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
+
+  FreePool (VolumeName);
+
+  *BufferSize = NeededLength;
+
+  return EFI_SUCCESS;
+}
+
+/**
+   Retrieves the volume label and stores it in the EFI_FILE_SYSTEM_VOLUME_LABEL format.
+
+   @param[in]      Part           Pointer to the opened partition.
+   @param[out]     Info           Pointer to a EFI_FILE_SYSTEM_VOLUME_LABEL.
+   @param[in out]  BufferSize     Pointer to the buffer size
+
+   @return Status of the file information request.
+**/
+STATIC
+EFI_STATUS
+Ext4GetVolumeLabelInfo (
+  IN EXT4_PARTITION                 *Part,
+  OUT EFI_FILE_SYSTEM_VOLUME_LABEL  *Info,
+  IN OUT UINTN                      *BufferSize
+  )
+{
+  // Length of s_volume_name + null terminator
+  CHAR16      *VolumeName;
+  UINTN       VolNameLength;
+  EFI_STATUS  Status;
+  UINTN       NeededLength;
+
+  Status = Ext4GetVolumeName (Part, &VolumeName, &VolNameLength);
+
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
-  if (VolumeName != NULL) {
+  NeededLength = (VolNameLength + 1) * sizeof (CHAR16);
+
+  if (NeededLength > *BufferSize) {
+    *BufferSize = NeededLength;
+
     FreePool (VolumeName);
+
+    return EFI_BUFFER_TOO_SMALL;
   }
 
+  Status = StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
+
+  ASSERT_EFI_ERROR (Status);
+
+  FreePool (VolumeName);
+
   *BufferSize = NeededLength;
 
   return EFI_SUCCESS;
@@ -674,12 +753,20 @@ Ext4GetInfo (
   OUT VOID              *Buffer
   )
 {
+  EXT4_PARTITION  *Partition;
+
+  Partition = ((EXT4_FILE *)This)->Partition;
+
   if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
     return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
   }
 
   if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
-    return Ext4GetFilesystemInfo (((EXT4_FILE *)This)->Partition, Buffer, BufferSize);
+    return Ext4GetFilesystemInfo (Partition, Buffer, BufferSize);
+  }
+
+  if (CompareGuid (InformationType, &gEfiFileSystemVolumeLabelInfoIdGuid)) {
+    return Ext4GetVolumeLabelInfo (Partition, Buffer, BufferSize);
   }
 
   return EFI_UNSUPPORTED;
-- 
2.33.0

[edk2-platforms PATCH v2 5/5] Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.

[Pedro Falcato]

This should close up some possible exploits using crafted
filesystem images.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 90 ++++++++++++++++------------
 1 file changed, 51 insertions(+), 39 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 7d1b2dcfe524..102c82f05da0 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -50,6 +50,37 @@ Ext4GetUcs2DirentName (
   return Status;
 }
 
+/**
+   Validates a directory entry.
+
+   @param[in]      Dirent      Pointer to the directory entry.
+
+   @retval TRUE          Valid directory entry.
+           FALSE         Invalid directory entry.
+**/
+STATIC
+BOOLEAN
+Ext4ValidDirent (
+  IN CONST EXT4_DIR_ENTRY  *Dirent
+  )
+{
+  UINTN  RequiredSize;
+
+  RequiredSize = Dirent->name_len + EXT4_MIN_DIR_ENTRY_LEN;
+
+  if (Dirent->rec_len < RequiredSize) {
+    DEBUG ((DEBUG_ERROR, "[ext4] dirent size %lu too small (compared to %lu)\n", Dirent->rec_len, RequiredSize));
+    return FALSE;
+  }
+
+  // Dirent sizes need to be 4 byte aligned
+  if (Dirent->rec_len % 4) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 /**
    Retrieves a directory entry.
 
@@ -75,11 +106,11 @@ Ext4RetrieveDirent (
   UINT64          DirInoSize;
   UINT32          BlockRemainder;
   UINTN           Length;
-  CHAR8           *BufPtr;
   EXT4_DIR_ENTRY  *Entry;
   UINTN           RemainingBlock;
   CHAR16          DirentUcs2Name[EXT4_NAME_MAX + 1];
   UINTN           ToCopy;
+  UINTN           BlockOffset;
 
   Status = EFI_NOT_FOUND;
   Buf    = AllocatePool (Partition->BlockSize);
@@ -109,14 +140,19 @@ Ext4RetrieveDirent (
       return Status;
     }
 
-    for (BufPtr = Buf; BufPtr < Buf + Partition->BlockSize; ) {
-      Entry = (EXT4_DIR_ENTRY *)BufPtr;
-      if (Entry->rec_len == 0) {
+    for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) {
+      Entry = (EXT4_DIR_ENTRY *)(Buf + BlockOffset);
+      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;
       }
 
-      RemainingBlock = Partition->BlockSize - (BufPtr - Buf);
+      if (!Ext4ValidDirent (Entry)) {
+        FreePool (Buf);
+        return EFI_VOLUME_CORRUPTED;
+      }
 
       if (Entry->name_len > RemainingBlock || Entry->rec_len > RemainingBlock) {
         // Corrupted filesystem
@@ -131,12 +167,13 @@ Ext4RetrieveDirent (
         2) Linux and a number of BSDs also have a filename limit of 255.
       */
       if (Entry->name_len > EXT4_NAME_MAX) {
+        BlockOffset += Entry->rec_len;
         continue;
       }
 
       // Unused entry
       if (Entry->inode == 0) {
-        BufPtr += Entry->rec_len;
+        BlockOffset += Entry->rec_len;
         continue;
       }
 
@@ -150,7 +187,7 @@ Ext4RetrieveDirent (
       if (EFI_ERROR (Status)) {
         // If we error out, skip this entry
         // I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
-        BufPtr += Entry->rec_len;
+        BlockOffset += Entry->rec_len;
         continue;
       }
 
@@ -163,7 +200,7 @@ Ext4RetrieveDirent (
         return EFI_SUCCESS;
       }
 
-      BufPtr += Entry->rec_len;
+      BlockOffset += Entry->rec_len;
     }
 
     Off += Partition->BlockSize;
@@ -379,37 +416,6 @@ Ext4OpenVolume (
   return EFI_SUCCESS;
 }
 
-/**
-   Validates a directory entry.
-
-   @param[in]      Dirent      Pointer to the directory entry.
-
-   @retval TRUE          Valid directory entry.
-           FALSE         Invalid directory entry.
-**/
-STATIC
-BOOLEAN
-Ext4ValidDirent (
-  IN CONST EXT4_DIR_ENTRY  *Dirent
-  )
-{
-  UINTN  RequiredSize;
-
-  RequiredSize = Dirent->name_len + EXT4_MIN_DIR_ENTRY_LEN;
-
-  if (Dirent->rec_len < RequiredSize) {
-    DEBUG ((DEBUG_ERROR, "[ext4] dirent size %lu too small (compared to %lu)\n", Dirent->rec_len, RequiredSize));
-    return FALSE;
-  }
-
-  // Dirent sizes need to be 4 byte aligned
-  if (Dirent->rec_len % 4) {
-    return FALSE;
-  }
-
-  return TRUE;
-}
-
 /**
    Reads a directory entry.
 
@@ -481,6 +487,12 @@ Ext4ReadDir (
       goto Out;
     }
 
+    // Check if the entire dir entry length fits in Len
+    if (Len < EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len) {
+      Status = EFI_VOLUME_CORRUPTED;
+      goto Out;
+    }
+
     // We don't care about passing . or .. entries to the caller of ReadDir(),
     // since they're generally useless entries *and* may break things if too
     // many callers assume FAT32.
-- 
2.33.0

Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs

[Michael D Kinney]

Series

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Saturday, August 21, 2021 7:47 AM
> To: devel@edk2.groups.io
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> 
> This patch-series addresses bugs found when testing the filesystem with
> more complex usage of the file protocol, particularly through the shell
> itself.
> 
> This is version 2 of the patch series and addresses feedback received
> from the community. This version also adds two new patches to further
> improve Ext4Dxe and make it more resilient and ready to be used.
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> Pedro Falcato (5):
>   Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
>   Ext4Pkg: Hide "." and ".." entries from Read() callers.
>   Ext4Pkg: Add a directory entry tree.
>   Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().
>   Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.
> 
>  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 343 ++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    |   1 -
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  88 ++++++-
>  Features/Ext4Pkg/Ext4Dxe/File.c       | 202 +++++++++++----
>  Features/Ext4Pkg/Ext4Dxe/Inode.c      |   3 +-
>  Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c |  23 +-
>  8 files changed, 534 insertions(+), 136 deletions(-)
> 
> --
> 2.33.0

Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs

[Michael D Kinney]

Hi Pedro,

Found one VS compat issue with signed/unsigned comparison in last patch.

It was a very simple fix for force an unsigned compare.

Change:
    if (Len < EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len) {

To:
    if (Len < (UINTN)(EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len)) {


With this one change:  Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

I will do push with this change

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, August 23, 2021 6:42 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <leif@nuviainc.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: RE: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> 
> Series
> 
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Mike
> 
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Saturday, August 21, 2021 7:47 AM
> > To: devel@edk2.groups.io
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> >
> > This patch-series addresses bugs found when testing the filesystem with
> > more complex usage of the file protocol, particularly through the shell
> > itself.
> >
> > This is version 2 of the patch series and addresses feedback received
> > from the community. This version also adds two new patches to further
> > improve Ext4Dxe and make it more resilient and ready to be used.
> >
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >
> > Pedro Falcato (5):
> >   Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
> >   Ext4Pkg: Hide "." and ".." entries from Read() callers.
> >   Ext4Pkg: Add a directory entry tree.
> >   Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().
> >   Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.
> >
> >  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 343 ++++++++++++++++++++------
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    |   1 -
> >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  88 ++++++-
> >  Features/Ext4Pkg/Ext4Dxe/File.c       | 202 +++++++++++----
> >  Features/Ext4Pkg/Ext4Dxe/Inode.c      |   3 +-
> >  Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
> >  Features/Ext4Pkg/Ext4Dxe/Superblock.c |  23 +-
> >  8 files changed, 534 insertions(+), 136 deletions(-)
> >
> > --
> > 2.33.0

Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs

[Michael D Kinney]

Pushed as ff31f8f683..75899d2a8f

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, August 23, 2021 6:58 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <leif@nuviainc.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: RE: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> 
> Hi Pedro,
> 
> Found one VS compat issue with signed/unsigned comparison in last patch.
> 
> It was a very simple fix for force an unsigned compare.
> 
> Change:
>     if (Len < EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len) {
> 
> To:
>     if (Len < (UINTN)(EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len)) {
> 
> 
> With this one change:  Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> I will do push with this change
> 
> Mike
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, August 23, 2021 6:42 PM
> > To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: RE: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> >
> > Series
> >
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Pedro Falcato <pedro.falcato@gmail.com>
> > > Sent: Saturday, August 21, 2021 7:47 AM
> > > To: devel@edk2.groups.io
> > > Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Subject: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
> > >
> > > This patch-series addresses bugs found when testing the filesystem with
> > > more complex usage of the file protocol, particularly through the shell
> > > itself.
> > >
> > > This is version 2 of the patch series and addresses feedback received
> > > from the community. This version also adds two new patches to further
> > > improve Ext4Dxe and make it more resilient and ready to be used.
> > >
> > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > >
> > > Pedro Falcato (5):
> > >   Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
> > >   Ext4Pkg: Hide "." and ".." entries from Read() callers.
> > >   Ext4Pkg: Add a directory entry tree.
> > >   Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().
> > >   Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.
> > >
> > >  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 343 ++++++++++++++++++++------
> > >  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
> > >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    |   1 -
> > >  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  88 ++++++-
> > >  Features/Ext4Pkg/Ext4Dxe/File.c       | 202 +++++++++++----
> > >  Features/Ext4Pkg/Ext4Dxe/Inode.c      |   3 +-
> > >  Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
> > >  Features/Ext4Pkg/Ext4Dxe/Superblock.c |  23 +-
> > >  8 files changed, 534 insertions(+), 136 deletions(-)
> > >
> > > --
> > > 2.33.0