* [edk2-platforms PATCH v2 1/5] Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
@ 2021-08-21 14:47 ` Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 2/5] Ext4Pkg: Hide "." and ".." entries from Read() callers Pedro Falcato
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2021-08-21 14:47 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Leif Lindholm, Michael D Kinney, Bret Barkelew
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [edk2-platforms PATCH v2 2/5] Ext4Pkg: Hide "." and ".." entries from Read() callers.
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 1/5] Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap Pedro Falcato
@ 2021-08-21 14:47 ` Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 3/5] Ext4Pkg: Add a directory entry tree Pedro Falcato
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2021-08-21 14:47 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Leif Lindholm, Michael D Kinney, Bret Barkelew
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [edk2-platforms PATCH v2 3/5] Ext4Pkg: Add a directory entry tree.
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 1/5] Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 2/5] Ext4Pkg: Hide "." and ".." entries from Read() callers Pedro Falcato
@ 2021-08-21 14:47 ` Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 4/5] Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo() Pedro Falcato
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2021-08-21 14:47 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Leif Lindholm, Michael D Kinney, Bret Barkelew
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [edk2-platforms PATCH v2 4/5] Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
` (2 preceding siblings ...)
2021-08-21 14:47 ` [edk2-platforms PATCH v2 3/5] Ext4Pkg: Add a directory entry tree Pedro Falcato
@ 2021-08-21 14:47 ` Pedro Falcato
2021-08-21 14:47 ` [edk2-platforms PATCH v2 5/5] Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values Pedro Falcato
2021-08-24 1:41 ` [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Michael D Kinney
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2021-08-21 14:47 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Leif Lindholm, Michael D Kinney, Bret Barkelew
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [edk2-platforms PATCH v2 5/5] Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
` (3 preceding siblings ...)
2021-08-21 14:47 ` [edk2-platforms PATCH v2 4/5] Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo() Pedro Falcato
@ 2021-08-21 14:47 ` Pedro Falcato
2021-08-24 1:41 ` [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Michael D Kinney
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2021-08-21 14:47 UTC (permalink / raw)
To: devel; +Cc: Pedro Falcato, Leif Lindholm, Michael D Kinney, Bret Barkelew
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
2021-08-21 14:47 [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Pedro Falcato
` (4 preceding siblings ...)
2021-08-21 14:47 ` [edk2-platforms PATCH v2 5/5] Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values Pedro Falcato
@ 2021-08-24 1:41 ` Michael D Kinney
2021-08-24 1:58 ` Michael D Kinney
5 siblings, 1 reply; 9+ messages in thread
From: Michael D Kinney @ 2021-08-24 1:41 UTC (permalink / raw)
To: Pedro Falcato, devel@edk2.groups.io, Kinney, Michael D
Cc: Leif Lindholm, Bret Barkelew
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
2021-08-24 1:41 ` [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs Michael D Kinney
@ 2021-08-24 1:58 ` Michael D Kinney
2021-08-24 1:59 ` Michael D Kinney
0 siblings, 1 reply; 9+ messages in thread
From: Michael D Kinney @ 2021-08-24 1:58 UTC (permalink / raw)
To: Pedro Falcato, devel@edk2.groups.io, Kinney, Michael D
Cc: Leif Lindholm, Bret Barkelew
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-platforms PATCH v2 0/5] Ext4Pkg: Fix bugs
2021-08-24 1:58 ` Michael D Kinney
@ 2021-08-24 1:59 ` Michael D Kinney
0 siblings, 0 replies; 9+ messages in thread
From: Michael D Kinney @ 2021-08-24 1:59 UTC (permalink / raw)
To: Pedro Falcato, devel@edk2.groups.io, Kinney, Michael D
Cc: Leif Lindholm, Bret Barkelew
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
^ permalink raw reply [flat|nested] 9+ messages in thread