[edk2-platforms][PATCH v4 0/2] Ext4Pkg: Add Symbolic Links support

[edk2-platforms][PATCH v4 0/2] Ext4Pkg: Add Symbolic Links support

[Savva Mitrofanov]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677

Hi all,

In the fourth version I corrected SymlinkSize selection logic in 
Ext4ReadFastSymlink(), previously fast-symlink's EXT4_INODE_SIZE is not necessarily 
validated when we checked it in Ext4SymlinkIsFastSymlink(), so we should truncate
if necessary. Also I corrected MSVC compiler warning by assigning ExtAttrBlocks to
UINT32 type.

This patchset adds symbolic links support with simple recursion protection based
on symbolic link nest level limitation, also I included patch which adds BASE_CR
to extract EXT4_FILE private structure to prevent possible code corruption caused
by structure changes and rearrangements in future.

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

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

Savva Mitrofanov (2):
  Ext4Pkg: Add symbolic links support
  Ext4Pkg: Add base containing record macro for EXT4_FILE

 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  | 100 +++++-
 Features/Ext4Pkg/Ext4Dxe/File.c     | 369 ++++++++++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
 4 files changed, 492 insertions(+), 43 deletions(-)

-- 
2.37.1

[edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support

[Savva Mitrofanov]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677

Provided support for symlink file type. Added routine which allows
reading and following them through recursive open() call. As a security
meausure implemented simple symlink loop check with nest level limit
equal 8. Also this patch moves Ext4Open functionality to internal
routine.

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
 Features/Ext4Pkg/Ext4Dxe/File.c     | 359 ++++++++++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
 4 files changed, 485 insertions(+), 38 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index a55cd2fa68ad..a73e3f8622f1 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -171,7 +171,7 @@
 #define EXT4_DIRTY_FL         0x00000100
 #define EXT4_COMPRBLK_FL      0x00000200
 #define EXT4_NOCOMPR_FL       0x00000400
-#define EXT4_ECOMPR_FL        0x00000800
+#define EXT4_ENCRYPT_FL       0x00000800
 #define EXT4_BTREE_FL         0x00001000
 #define EXT4_INDEX_FL         0x00002000
 #define EXT4_JOURNAL_DATA_FL  0x00004000
@@ -332,11 +332,12 @@ STATIC_ASSERT (
   "ext4 block group descriptor struct has incorrect size"
   );
 
-#define EXT4_DBLOCKS     12
-#define EXT4_IND_BLOCK   12
-#define EXT4_DIND_BLOCK  13
-#define EXT4_TIND_BLOCK  14
-#define EXT4_NR_BLOCKS   15
+#define EXT4_DBLOCKS                12
+#define EXT4_IND_BLOCK              12
+#define EXT4_DIND_BLOCK             13
+#define EXT4_TIND_BLOCK             14
+#define EXT4_NR_BLOCKS              15
+#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * sizeof(UINT32)
 
 #define EXT4_GOOD_OLD_INODE_SIZE  128
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index b1508482b0a7..c1df9d1149e4 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -31,7 +31,9 @@
 
 #include "Ext4Disk.h"
 
+#define SYMLOOP_MAX    8
 #define EXT4_NAME_MAX  255
+#define EFI_PATH_MAX   4096
 
 #define EXT4_DRIVER_VERSION  0x0000
 
@@ -324,11 +326,11 @@ number of read bytes.
 **/
 EFI_STATUS
 Ext4Read (
-  IN EXT4_PARTITION  *Partition,
-  IN EXT4_FILE       *File,
-  OUT VOID           *Buffer,
-  IN UINT64          Offset,
-  IN OUT UINTN       *Length
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE       *File,
+  OUT    VOID            *Buffer,
+  IN     UINT64          Offset,
+  IN OUT UINTN           *Length
   );
 
 /**
@@ -368,6 +370,7 @@ struct _Ext4File {
 
   UINT64                OpenMode;
   UINT64                Position;
+  UINT32                SymLoops;
 
   EXT4_PARTITION        *Partition;
 
@@ -497,6 +500,45 @@ Ext4SetupFile (
   IN EXT4_PARTITION  *Partition
   );
 
+/**
+  Opens a new file relative to the source file's location.
+
+  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
+                         file.
+  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
+                         handle to the source location. This would typically be an open
+                         handle to a directory.
+  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
+                         The file name may contain the following path modifiers: "\", ".",
+                         and "..".
+  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
+                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
+  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
+                         attribute bits for the newly created file.
+
+  @retval EFI_SUCCESS          The file was opened.
+  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
+  @retval EFI_NO_MEDIA         The device has no medium.
+  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
+                               longer supported.
+  @retval EFI_DEVICE_ERROR     The device reported an error.
+  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
+                               when the media is write-protected.
+  @retval EFI_ACCESS_DENIED    The service denied access to the file.
+  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
+  @retval EFI_VOLUME_FULL      The volume is full.
+
+**/
+EFI_STATUS
+Ext4OpenInternal (
+  OUT EXT4_FILE  **FoundFile,
+  IN  EXT4_FILE  *Source,
+  IN  CHAR16     *FileName,
+  IN  UINT64     OpenMode,
+  IN  UINT64     Attributes
+  );
+
 /**
    Closes a file.
 
@@ -774,6 +816,30 @@ Ext4FileIsDir (
   IN CONST EXT4_FILE  *File
   );
 
+/**
+   Checks if a file is a symlink.
+
+   @param[in]      File          Pointer to the opened file.
+
+   @return BOOLEAN         Whether file is a symlink
+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE  *File
+  );
+
+/**
+   Detects if a symlink is a fast symlink.
+
+   @param[in]      File          Pointer to the opened file.
+
+   @return BOOLEAN         Whether symlink is a fast symlink
+**/
+BOOLEAN
+Ext4SymlinkIsFastSymlink (
+  IN CONST EXT4_FILE  *File
+  );
+
 /**
    Checks if a file is a regular file.
    @param[in]      File          Pointer to the opened file.
@@ -797,7 +863,7 @@ Ext4FileIsReg (
            it's a regular file or a directory, since most other file types
            don't make sense under UEFI.
 **/
-#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) || Ext4FileIsDir(File))
+#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File))
 
 #define EXT4_INODE_HAS_FIELD(Inode, Field)                                     \
   (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=                          \
@@ -935,6 +1001,26 @@ Ext4ReadDir (
   IN OUT UINTN       *OutLength
   );
 
+/**
+  Reads a symlink file.
+
+  @param[in]      Partition   Pointer to the ext4 partition.
+  @param[in]      File        Pointer to the open symlink file.
+  @param[out]     Symlink     Pointer to the output unicode symlink string.
+
+  @retval EFI_SUCCESS           Symlink was read.
+  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
+  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
+  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
+  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
+**/
+EFI_STATUS
+Ext4ReadSymlink (
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE       *File,
+  OUT    CHAR16          **Symlink
+  );
+
 /**
    Initialises the (empty) extents map, that will work as a cache of extents.
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index ff1746d5640a..ae9230d6422b 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -134,14 +134,224 @@ Ext4DirCanLookup (
   return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) == EXT4_INO_PERM_EXEC_OWNER;
 }
 
+/**
+  Reads a fast symlink file.
+
+  @param[in]      Partition   Pointer to the ext4 partition.
+  @param[in]      File        Pointer to the open symlink file.
+  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
+  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
+
+  @retval EFI_SUCCESS            Fast symlink was read.
+  @retval EFI_OUT_OF_RESOURCES   Memory allocation error.
+**/
+STATIC
+EFI_STATUS
+Ext4ReadFastSymlink (
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE       *File,
+  OUT    CHAR8           **AsciiSymlink,
+  OUT    UINT32          *AsciiSymlinkSize
+  )
+{
+  UINT32  SymlinkSize;
+  CHAR8   *AsciiSymlinkTmp;
+  //
+  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we checked it in
+  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
+  //
+  SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), EXT4_FAST_SYMLINK_MAX_SIZE);
+
+  AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
+  if (AsciiSymlinkTmp == NULL) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
+
+  //
+  // Add null-terminator
+  //
+  AsciiSymlinkTmp[SymlinkSize] = '\0';
+
+  *AsciiSymlink     = AsciiSymlinkTmp;
+  *AsciiSymlinkSize = SymlinkSize + 1;
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Reads a slow symlink file.
+
+  @param[in]      Partition        Pointer to the ext4 partition.
+  @param[in]      File             Pointer to the open symlink file.
+  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
+  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
+
+  @retval EFI_SUCCESS           Slow symlink was read.
+  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
+  @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
+  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
+**/
+STATIC
+EFI_STATUS
+Ext4ReadSlowSymlink (
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE       *File,
+  OUT    CHAR8           **AsciiSymlink,
+  OUT    UINT32          *AsciiSymlinkSize
+  )
+{
+  EFI_STATUS  Status;
+  CHAR8       *SymlinkTmp;
+  UINT64      SymlinkSizeTmp;
+  UINT32      SymlinkAllocateSize;
+  UINTN       ReadSize;
+
+  SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
+
+  //
+  // Allocate EXT4_INODE_SIZE + 1
+  //
+  if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
+    DEBUG ((
+      DEBUG_FS,
+      "[ext4] Error! Symlink path maximum length was hit!\n"
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
+
+  SymlinkTmp = AllocatePool (SymlinkAllocateSize);
+  if (SymlinkTmp == NULL) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  ReadSize = (UINTN)SymlinkSizeTmp;
+  Status   = Ext4Read (Partition, File, SymlinkTmp, File->Position, &ReadSize);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with Status %r\n", Status));
+    FreePool (SymlinkTmp);
+    return Status;
+  }
+
+  File->Position += ReadSize;
+
+  //
+  // Add null-terminator
+  //
+  SymlinkTmp[SymlinkSizeTmp] = '\0';
+
+  if (SymlinkSizeTmp != ReadSize) {
+    DEBUG ((
+      DEBUG_FS,
+      "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n"
+      ));
+    return EFI_VOLUME_CORRUPTED;
+  }
+
+  *AsciiSymlinkSize = SymlinkAllocateSize;
+  *AsciiSymlink     = SymlinkTmp;
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Reads a symlink file.
+
+  @param[in]      Partition   Pointer to the ext4 partition.
+  @param[in]      File        Pointer to the open symlink file.
+  @param[out]     Symlink     Pointer to the output unicode symlink string.
+
+  @retval EFI_SUCCESS           Symlink was read.
+  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
+  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
+  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
+  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
+**/
+EFI_STATUS
+Ext4ReadSymlink (
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE       *File,
+  OUT    CHAR16          **Symlink
+  )
+{
+  EFI_STATUS  Status;
+  CHAR8       *SymlinkTmp;
+  UINT32      SymlinkSize;
+  CHAR16      *Symlink16Tmp;
+  CHAR16      *Needle;
+
+  //
+  // Assume that we alread read Inode via Ext4ReadInode
+  // Skip reading, just check encryption flag
+  //
+  if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
+    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
+    return EFI_ACCESS_DENIED;
+  }
+
+  if (Ext4SymlinkIsFastSymlink (File)) {
+    Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
+  } else {
+    Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
+  }
+
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n", Status));
+    return Status;
+  }
+
+  Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));
+  if (Symlink16Tmp == NULL) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n"));
+    FreePool (SymlinkTmp);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = AsciiStrToUnicodeStrS (
+             SymlinkTmp,
+             Symlink16Tmp,
+             SymlinkSize
+             );
+
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_FS,
+      "[ext4] Failed to convert ascii symlink to unicode with Status %r\n",
+      Status
+      ));
+    FreePool (Symlink16Tmp);
+    FreePool (SymlinkTmp);
+    return Status;
+  }
+
+  //
+  // Convert to UEFI slashes
+  //
+  for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
+    if (*Needle == L'/') {
+      *Needle = L'\\';
+    }
+  }
+
+  *Symlink = Symlink16Tmp;
+
+  FreePool (SymlinkTmp);
+  return Status;
+}
+
 /**
   Opens a new file relative to the source file's location.
 
-  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
+  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
+                         file.
+  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
                          handle to the source location. This would typically be an open
                          handle to a directory.
-  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
-                         file.
   @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
                          The file name may contain the following path modifiers: "\", ".",
                          and "..".
@@ -165,13 +375,12 @@ Ext4DirCanLookup (
 
 **/
 EFI_STATUS
-EFIAPI
-Ext4Open (
-  IN EFI_FILE_PROTOCOL   *This,
-  OUT EFI_FILE_PROTOCOL  **NewHandle,
-  IN CHAR16              *FileName,
-  IN UINT64              OpenMode,
-  IN UINT64              Attributes
+Ext4OpenInternal (
+  OUT EXT4_FILE  **FoundFile,
+  IN  EXT4_FILE  *Source,
+  IN  CHAR16     *FileName,
+  IN  UINT64     OpenMode,
+  IN  UINT64     Attributes
   )
 {
   EXT4_FILE       *Current;
@@ -180,13 +389,14 @@ Ext4Open (
   CHAR16          PathSegment[EXT4_NAME_MAX + 1];
   UINTN           Length;
   EXT4_FILE       *File;
+  CHAR16          *Symlink;
   EFI_STATUS      Status;
 
-  Current   = (EXT4_FILE *)This;
+  Current   = Source;
   Partition = Current->Partition;
   Level     = 0;
 
-  DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
+  DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
   // If the path starts with a backslash, we treat the root directory as the base directory
   if (FileName[0] == L'\\') {
     FileName++;
@@ -194,6 +404,11 @@ Ext4Open (
   }
 
   while (FileName[0] != L'\0') {
+    if (Partition->Root->SymLoops > SYMLOOP_MAX) {
+      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
+      return EFI_ACCESS_DENIED;
+    }
+
     // Discard leading path separators
     while (FileName[0] == L'\\') {
       FileName++;
@@ -238,18 +453,45 @@ Ext4Open (
     }
 
     // Check if this is a valid file to open in EFI
-
-    // What to do with symlinks? They're nonsense when absolute but may
-    // be useful when they're relative. Right now, they're ignored, since they
-    // bring a lot of trouble for something that's not as useful in our case.
-    // If you want to link, use hard links.
-
     if (!Ext4FileIsOpenable (File)) {
       Ext4CloseInternal (File);
       // This looks like an /okay/ status to return.
       return EFI_ACCESS_DENIED;
     }
 
+    //
+    // Reading symlink and then trying to follow it
+    //
+    if (Ext4FileIsSymlink (File)) {
+      Partition->Root->SymLoops++;
+      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n", PathSegment));
+      Status = Ext4ReadSymlink (Partition, File, &Symlink);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n", PathSegment));
+        return Status;
+      }
+
+      DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment, Symlink));
+      //
+      // Close symlink file
+      //
+      Ext4CloseInternal (File);
+      //
+      // Open linked file by recursive call of Ext4OpenFile
+      //
+      Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode, Attributes);
+      FreePool (Symlink);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n", Symlink));
+        return Status;
+      }
+
+      //
+      // Set File to newly opened
+      //
+      File = *FoundFile;
+    }
+
     if (Level != 0) {
       // Careful not to close the base directory
       Ext4CloseInternal (Current);
@@ -273,12 +515,75 @@ Ext4Open (
     return EFI_ACCESS_DENIED;
   }
 
-  *NewHandle = &Current->Protocol;
+  *FoundFile = Current;
 
   DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
   return EFI_SUCCESS;
 }
 
+/**
+  Opens a new file relative to the source file's location.
+  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
+                         handle to the source location. This would typically be an open
+                         handle to a directory.
+  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
+                         file.
+  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
+                         The file name may contain the following path modifiers: "\", ".",
+                         and "..".
+  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
+                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
+  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
+                         attribute bits for the newly created file.
+  @retval EFI_SUCCESS          The file was opened.
+  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
+  @retval EFI_NO_MEDIA         The device has no medium.
+  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
+                               longer supported.
+  @retval EFI_DEVICE_ERROR     The device reported an error.
+  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
+                               when the media is write-protected.
+  @retval EFI_ACCESS_DENIED    The service denied access to the file.
+  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
+  @retval EFI_VOLUME_FULL      The volume is full.
+**/
+EFI_STATUS
+EFIAPI
+Ext4Open (
+  IN EFI_FILE_PROTOCOL   *This,
+  OUT EFI_FILE_PROTOCOL  **NewHandle,
+  IN CHAR16              *FileName,
+  IN UINT64              OpenMode,
+  IN UINT64              Attributes
+  )
+{
+  EFI_STATUS  Status;
+  EXT4_FILE   *FoundFile;
+  EXT4_FILE   *Source;
+
+  Source = (EXT4_FILE *)This;
+
+  //
+  // Reset SymLoops counter
+  //
+  Source->Partition->Root->SymLoops = 0;
+
+  Status = Ext4OpenInternal (
+             &FoundFile,
+             Source,
+             FileName,
+             OpenMode,
+             Attributes
+             );
+
+  if (!EFI_ERROR (Status)) {
+    *NewHandle = &FoundFile->Protocol;
+  }
+
+  return Status;
+}
+
 /**
   Closes a specified file handle.
 
@@ -588,7 +893,7 @@ Ext4GetVolumeName (
 
   // s_volume_name is only valid on dynamic revision; old filesystems don't support this
   if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
-    CopyMem (TempVolName, (CONST CHAR8 *)Partition->SuperBlock.s_volume_name, 16);
+    CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
     TempVolName[16] = '\0';
 
     Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
@@ -754,12 +1059,14 @@ Ext4GetInfo (
   OUT VOID              *Buffer
   )
 {
+  EXT4_FILE       *File;
   EXT4_PARTITION  *Partition;
 
-  Partition = ((EXT4_FILE *)This)->Partition;
+  File      = (EXT4_FILE *)This;
+  Partition = File->Partition;
 
   if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
-    return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
+    return Ext4GetFileInfo (File, Buffer, BufferSize);
   }
 
   if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
@@ -870,12 +1177,12 @@ Ext4SetInfo (
   )
 {
   EXT4_FILE       *File;
-  EXT4_PARTITION  *Part;
+  EXT4_PARTITION  *Partition;
 
-  File = (EXT4_FILE *)This;
-  Part = File->Partition;
+  File      = (EXT4_FILE *)This;
+  Partition = File->Partition;
 
-  if (Part->ReadOnly) {
+  if (Partition->ReadOnly) {
     return EFI_WRITE_PROTECTED;
   }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 831f5946e870..e7a6b3225709 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -255,6 +255,59 @@ Ext4FileIsDir (
   return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
 }
 
+/**
+   Checks if a file is a symlink.
+
+   @param[in]      File          Pointer to the opened file.
+
+   @return BOOLEAN         Whether file is a symlink
+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE  *File
+  )
+{
+  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) == EXT4_INO_TYPE_SYMLINK;
+}
+
+/**
+   Detects if a symlink is a fast symlink.
+
+   @param[in]      File          Pointer to the opened file.
+
+   @return BOOLEAN         Whether symlink is a fast symlink
+**/
+BOOLEAN
+Ext4SymlinkIsFastSymlink (
+  IN CONST EXT4_FILE  *File
+  )
+{
+  //
+  // Detection logic of the fast-symlink splits into two behaviors - old and new.
+  // The old behavior is based on comparing the extended attribute blocks
+  // with the inode's i_blocks, and if it's zero we know the inode isn't storing
+  // the link in filesystem blocks, so we look to the inode->i_data.
+  // The new behavior is apparently needed only with the large EA inode feature.
+  // In this case we check that inode size less than maximum fast symlink size.
+  // So, we revert to the old behavior if the large EA inode feature is not set.
+  //
+  UINT32  FileAcl;
+  UINT32  ExtAttrBlocks;
+
+  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
+    FileAcl = File->Inode->i_file_acl;
+    if (EXT4_IS_64_BIT (File->Partition)) {
+      FileAcl |= LShiftU64 (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);
+    }
+
+    ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
+
+    return File->Inode->i_blocks == ExtAttrBlocks;
+  }
+
+  return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
+}
+
 /**
    Checks if a file is a regular file.
    @param[in]      File          Pointer to the opened file.
-- 
2.37.1

Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support

[Marvin Häuser]

Looks very nice, tyvm. I did add a few more comments, but nothing critical at all.

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de> as-is or with my comments addressed, either works.

> On 28. Jul 2022, at 17:26, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677
> 
> Provided support for symlink file type. Added routine which allows
> reading and following them through recursive open() call. As a security
> meausure implemented simple symlink loop check with nest level limit
> equal 8. Also this patch moves Ext4Open functionality to internal
> routine.
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
> Features/Ext4Pkg/Ext4Dxe/File.c     | 359 ++++++++++++++++++--
> Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
> 4 files changed, 485 insertions(+), 38 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index a55cd2fa68ad..a73e3f8622f1 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -171,7 +171,7 @@
> #define EXT4_DIRTY_FL         0x00000100
> #define EXT4_COMPRBLK_FL      0x00000200
> #define EXT4_NOCOMPR_FL       0x00000400
> -#define EXT4_ECOMPR_FL        0x00000800
> +#define EXT4_ENCRYPT_FL       0x00000800
> #define EXT4_BTREE_FL         0x00001000
> #define EXT4_INDEX_FL         0x00002000
> #define EXT4_JOURNAL_DATA_FL  0x00004000
> @@ -332,11 +332,12 @@ STATIC_ASSERT (
>   "ext4 block group descriptor struct has incorrect size"
>   );
> 
> -#define EXT4_DBLOCKS     12
> -#define EXT4_IND_BLOCK   12
> -#define EXT4_DIND_BLOCK  13
> -#define EXT4_TIND_BLOCK  14
> -#define EXT4_NR_BLOCKS   15
> +#define EXT4_DBLOCKS                12
> +#define EXT4_IND_BLOCK              12
> +#define EXT4_DIND_BLOCK             13
> +#define EXT4_TIND_BLOCK             14
> +#define EXT4_NR_BLOCKS              15
> +#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * sizeof(UINT32)
> 
> #define EXT4_GOOD_OLD_INODE_SIZE  128
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index b1508482b0a7..c1df9d1149e4 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -31,7 +31,9 @@
> 
> #include "Ext4Disk.h"
> 
> +#define SYMLOOP_MAX    8
> #define EXT4_NAME_MAX  255
> +#define EFI_PATH_MAX   4096
> 
> #define EXT4_DRIVER_VERSION  0x0000
> 
> @@ -324,11 +326,11 @@ number of read bytes.
> **/
> EFI_STATUS
> Ext4Read (
> -  IN EXT4_PARTITION  *Partition,
> -  IN EXT4_FILE       *File,
> -  OUT VOID           *Buffer,
> -  IN UINT64          Offset,
> -  IN OUT UINTN       *Length
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    VOID            *Buffer,
> +  IN     UINT64          Offset,
> +  IN OUT UINTN           *Length
>   );
> 
> /**
> @@ -368,6 +370,7 @@ struct _Ext4File {
> 
>   UINT64                OpenMode;
>   UINT64                Position;
> +  UINT32                SymLoops;
> 
>   EXT4_PARTITION        *Partition;
> 
> @@ -497,6 +500,45 @@ Ext4SetupFile (
>   IN EXT4_PARTITION  *Partition
>   );
> 
> +/**
> +  Opens a new file relative to the source file's location.
> +
> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
> +                         handle to the source location. This would typically be an open
> +                         handle to a directory.
> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
> +                         The file name may contain the following path modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
> +                         attribute bits for the newly created file.
> +
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +
> +**/
> +EFI_STATUS
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
> +  );
> +
> /**
>    Closes a file.
> 
> @@ -774,6 +816,30 @@ Ext4FileIsDir (
>   IN CONST EXT4_FILE  *File
>   );
> 
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
> +
> /**
>    Checks if a file is a regular file.
>    @param[in]      File          Pointer to the opened file.
> @@ -797,7 +863,7 @@ Ext4FileIsReg (
>            it's a regular file or a directory, since most other file types
>            don't make sense under UEFI.
> **/
> -#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) || Ext4FileIsDir(File))
> +#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File))
> 
> #define EXT4_INODE_HAS_FIELD(Inode, Field)                                     \
>   (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=                          \
> @@ -935,6 +1001,26 @@ Ext4ReadDir (
>   IN OUT UINTN       *OutLength
>   );
> 
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  );
> +
> /**
>    Initialises the (empty) extents map, that will work as a cache of extents.
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
> index ff1746d5640a..ae9230d6422b 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -134,14 +134,224 @@ Ext4DirCanLookup (
>   return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) == EXT4_INO_PERM_EXEC_OWNER;
> }
> 
> +/**
> +  Reads a fast symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
> +
> +  @retval EFI_SUCCESS            Fast symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES   Memory allocation error.
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadFastSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  UINT32  SymlinkSize;
> +  CHAR8   *AsciiSymlinkTmp;
> +  //
> +  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we checked it in
> +  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
> +  //
> +  SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), EXT4_FAST_SYMLINK_MAX_SIZE);
> +
> +  AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
> +  if (AsciiSymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
> +
> +  //
> +  // Add null-terminator
> +  //
> +  AsciiSymlinkTmp[SymlinkSize] = '\0';
> +
> +  *AsciiSymlink     = AsciiSymlinkTmp;
> +  *AsciiSymlinkSize = SymlinkSize + 1;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a slow symlink file.
> +
> +  @param[in]      Partition        Pointer to the ext4 partition.
> +  @param[in]      File             Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
> +
> +  @retval EFI_SUCCESS           Slow symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadSlowSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT64      SymlinkSizeTmp;
> +  UINT32      SymlinkAllocateSize;
> +  UINTN       ReadSize;
> +
> +  SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
> +
> +  //
> +  // Allocate EXT4_INODE_SIZE + 1
> +  //
> +  if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Error! Symlink path maximum length was hit!\n"
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
> +
> +  SymlinkTmp = AllocatePool (SymlinkAllocateSize);
> +  if (SymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  ReadSize = (UINTN)SymlinkSizeTmp;
> +  Status   = Ext4Read (Partition, File, SymlinkTmp, File->Position, &ReadSize);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with Status %r\n", Status));
> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  File->Position += ReadSize;
> +
> +  //
> +  // Add null-terminator
> +  //
> +  SymlinkTmp[SymlinkSizeTmp] = '\0';
> +
> +  if (SymlinkSizeTmp != ReadSize) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n"
> +      ));
> +    return EFI_VOLUME_CORRUPTED;
> +  }
> +
> +  *AsciiSymlinkSize = SymlinkAllocateSize;
> +  *AsciiSymlink     = SymlinkTmp;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT32      SymlinkSize;
> +  CHAR16      *Symlink16Tmp;
> +  CHAR16      *Needle;
> +
> +  //
> +  // Assume that we alread read Inode via Ext4ReadInode
> +  // Skip reading, just check encryption flag
> +  //
> +  if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
> +    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  if (Ext4SymlinkIsFastSymlink (File)) {
> +    Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
> +  } else {
> +    Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n", Status));
> +    return Status;
> +  }
> +
> +  Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));

Any specific reason to use ZeroPool here, when the rest of the code doesn’t?

> +  if (Symlink16Tmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n"));
> +    FreePool (SymlinkTmp);
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = AsciiStrToUnicodeStrS (
> +             SymlinkTmp,
> +             Symlink16Tmp,
> +             SymlinkSize
> +             );

Can’t SymlinkTmp be free’d here?

> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Failed to convert ascii symlink to unicode with Status %r\n",
> +      Status
> +      ));
> +    FreePool (Symlink16Tmp);
> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  //
> +  // Convert to UEFI slashes
> +  //
> +  for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
> +    if (*Needle == L'/') {
> +      *Needle = L'\\';
> +    }
> +  }
> +
> +  *Symlink = Symlink16Tmp;
> +
> +  FreePool (SymlinkTmp);
> +  return Status;
> +}
> +
> /**
>   Opens a new file relative to the source file's location.
> 
> -  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
>                          handle to the source location. This would typically be an open
>                          handle to a directory.
> -  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
> -                         file.
>   @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
>                          The file name may contain the following path modifiers: "\", ".",
>                          and "..".
> @@ -165,13 +375,12 @@ Ext4DirCanLookup (
> 
> **/
> EFI_STATUS
> -EFIAPI
> -Ext4Open (
> -  IN EFI_FILE_PROTOCOL   *This,
> -  OUT EFI_FILE_PROTOCOL  **NewHandle,
> -  IN CHAR16              *FileName,
> -  IN UINT64              OpenMode,
> -  IN UINT64              Attributes
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
>   )
> {
>   EXT4_FILE       *Current;
> @@ -180,13 +389,14 @@ Ext4Open (
>   CHAR16          PathSegment[EXT4_NAME_MAX + 1];
>   UINTN           Length;
>   EXT4_FILE       *File;
> +  CHAR16          *Symlink;
>   EFI_STATUS      Status;
> 
> -  Current   = (EXT4_FILE *)This;
> +  Current   = Source;
>   Partition = Current->Partition;
>   Level     = 0;
> 
> -  DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
> +  DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
>   // If the path starts with a backslash, we treat the root directory as the base directory
>   if (FileName[0] == L'\\') {
>     FileName++;
> @@ -194,6 +404,11 @@ Ext4Open (
>   }
> 
>   while (FileName[0] != L'\0') {
> +    if (Partition->Root->SymLoops > SYMLOOP_MAX) {
> +      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
> +      return EFI_ACCESS_DENIED;
> +    }
> +
>     // Discard leading path separators
>     while (FileName[0] == L'\\') {
>       FileName++;
> @@ -238,18 +453,45 @@ Ext4Open (
>     }
> 
>     // Check if this is a valid file to open in EFI
> -
> -    // What to do with symlinks? They're nonsense when absolute but may
> -    // be useful when they're relative. Right now, they're ignored, since they
> -    // bring a lot of trouble for something that's not as useful in our case.
> -    // If you want to link, use hard links.
> -
>     if (!Ext4FileIsOpenable (File)) {
>       Ext4CloseInternal (File);
>       // This looks like an /okay/ status to return.
>       return EFI_ACCESS_DENIED;
>     }
> 
> +    //
> +    // Reading symlink and then trying to follow it
> +    //
> +    if (Ext4FileIsSymlink (File)) {
> +      Partition->Root->SymLoops++;
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n", PathSegment));
> +      Status = Ext4ReadSymlink (Partition, File, &Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n", PathSegment));
> +        return Status;
> +      }
> +
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment, Symlink));
> +      //
> +      // Close symlink file
> +      //
> +      Ext4CloseInternal (File);
> +      //
> +      // Open linked file by recursive call of Ext4OpenFile
> +      //
> +      Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode, Attributes);
> +      FreePool (Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n", Symlink));
> +        return Status;
> +      }
> +
> +      //
> +      // Set File to newly opened
> +      //
> +      File = *FoundFile;
> +    }
> +
>     if (Level != 0) {
>       // Careful not to close the base directory
>       Ext4CloseInternal (Current);
> @@ -273,12 +515,75 @@ Ext4Open (
>     return EFI_ACCESS_DENIED;
>   }
> 
> -  *NewHandle = &Current->Protocol;
> +  *FoundFile = Current;
> 
>   DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
>   return EFI_SUCCESS;
> }
> 
> +/**
> +  Opens a new file relative to the source file's location.
> +  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
> +                         handle to the source location. This would typically be an open
> +                         handle to a directory.
> +  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
> +                         The file name may contain the following path modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
> +                         attribute bits for the newly created file.
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +**/
> +EFI_STATUS
> +EFIAPI
> +Ext4Open (
> +  IN EFI_FILE_PROTOCOL   *This,
> +  OUT EFI_FILE_PROTOCOL  **NewHandle,
> +  IN CHAR16              *FileName,
> +  IN UINT64              OpenMode,
> +  IN UINT64              Attributes
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EXT4_FILE   *FoundFile;
> +  EXT4_FILE   *Source;
> +
> +  Source = (EXT4_FILE *)This;
> +
> +  //
> +  // Reset SymLoops counter
> +  //
> +  Source->Partition->Root->SymLoops = 0;
> +
> +  Status = Ext4OpenInternal (
> +             &FoundFile,
> +             Source,
> +             FileName,
> +             OpenMode,
> +             Attributes
> +             );
> +
> +  if (!EFI_ERROR (Status)) {
> +    *NewHandle = &FoundFile->Protocol;
> +  }
> +
> +  return Status;
> +}
> +
> /**
>   Closes a specified file handle.
> 
> @@ -588,7 +893,7 @@ Ext4GetVolumeName (
> 
>   // s_volume_name is only valid on dynamic revision; old filesystems don't support this
>   if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
> -    CopyMem (TempVolName, (CONST CHAR8 *)Partition->SuperBlock.s_volume_name, 16);
> +    CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
>     TempVolName[16] = '\0';
> 
>     Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
> @@ -754,12 +1059,14 @@ Ext4GetInfo (
>   OUT VOID              *Buffer
>   )
> {
> +  EXT4_FILE       *File;
>   EXT4_PARTITION  *Partition;
> 
> -  Partition = ((EXT4_FILE *)This)->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
> 
>   if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
> -    return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
> +    return Ext4GetFileInfo (File, Buffer, BufferSize);
>   }
> 
>   if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
> @@ -870,12 +1177,12 @@ Ext4SetInfo (
>   )
> {
>   EXT4_FILE       *File;
> -  EXT4_PARTITION  *Part;
> +  EXT4_PARTITION  *Partition;
> 
> -  File = (EXT4_FILE *)This;
> -  Part = File->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
> 
> -  if (Part->ReadOnly) {
> +  if (Partition->ReadOnly) {
>     return EFI_WRITE_PROTECTED;
>   }
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 831f5946e870..e7a6b3225709 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -255,6 +255,59 @@ Ext4FileIsDir (
>   return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
> }
> 
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) == EXT4_INO_TYPE_SYMLINK;
> +}
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  //
> +  // Detection logic of the fast-symlink splits into two behaviors - old and new.
> +  // The old behavior is based on comparing the extended attribute blocks
> +  // with the inode's i_blocks, and if it's zero we know the inode isn't storing
> +  // the link in filesystem blocks, so we look to the inode->i_data.
> +  // The new behavior is apparently needed only with the large EA inode feature.
> +  // In this case we check that inode size less than maximum fast symlink size.
> +  // So, we revert to the old behavior if the large EA inode feature is not set.
> +  //
> +  UINT32  FileAcl;
> +  UINT32  ExtAttrBlocks;
> +
> +  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
> +    FileAcl = File->Inode->i_file_acl;
> +    if (EXT4_IS_64_BIT (File->Partition)) {
> +      FileAcl |= LShiftU64 (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);

*If* you happen to do a new revision anyway, you could drop the shift (with a comment!) if you want, because we don’t care about the actual value, just whether any one bit is set. Don’t bother if there won’t be a new revision anyway, though. :)

> +    }
> +
> +    ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
> +
> +    return File->Inode->i_blocks == ExtAttrBlocks;
> +  }
> +
> +  return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
> +}
> +
> /**
>    Checks if a file is a regular file.
>    @param[in]      File          Pointer to the opened file.
> -- 
> 2.37.1
> 

Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support

[Savva Mitrofanov]

Hello Marvin!

That seems there will be another revision based on Pedro Falcato's comments,
so in the next version I solved all the remarks from you.

Best regards,
Savva Mitrofanov

> On 28 Jul 2022, at 22:43, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> Looks very nice, tyvm. I did add a few more comments, but nothing critical at all.
> 
> Reviewed-by: Marvin Häuser <mhaeuser@posteo.de> as-is or with my comments addressed, either works.
> 
>> On 28. Jul 2022, at 17:26, Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677
>> 
>> Provided support for symlink file type. Added routine which allows
>> reading and following them through recursive open() call. As a security
>> meausure implemented simple symlink loop check with nest level limit
>> equal 8. Also this patch moves Ext4Open functionality to internal
>> routine.
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
>> Features/Ext4Pkg/Ext4Dxe/File.c     | 359 ++++++++++++++++++--
>> Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
>> 4 files changed, 485 insertions(+), 38 deletions(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> index a55cd2fa68ad..a73e3f8622f1 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> @@ -171,7 +171,7 @@
>> #define EXT4_DIRTY_FL         0x00000100
>> #define EXT4_COMPRBLK_FL      0x00000200
>> #define EXT4_NOCOMPR_FL       0x00000400
>> -#define EXT4_ECOMPR_FL        0x00000800
>> +#define EXT4_ENCRYPT_FL       0x00000800
>> #define EXT4_BTREE_FL         0x00001000
>> #define EXT4_INDEX_FL         0x00002000
>> #define EXT4_JOURNAL_DATA_FL  0x00004000
>> @@ -332,11 +332,12 @@ STATIC_ASSERT (
>>  "ext4 block group descriptor struct has incorrect size"
>>  );
>> 
>> -#define EXT4_DBLOCKS     12
>> -#define EXT4_IND_BLOCK   12
>> -#define EXT4_DIND_BLOCK  13
>> -#define EXT4_TIND_BLOCK  14
>> -#define EXT4_NR_BLOCKS   15
>> +#define EXT4_DBLOCKS                12
>> +#define EXT4_IND_BLOCK              12
>> +#define EXT4_DIND_BLOCK             13
>> +#define EXT4_TIND_BLOCK             14
>> +#define EXT4_NR_BLOCKS              15
>> +#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * sizeof(UINT32)
>> 
>> #define EXT4_GOOD_OLD_INODE_SIZE  128
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> index b1508482b0a7..c1df9d1149e4 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> @@ -31,7 +31,9 @@
>> 
>> #include "Ext4Disk.h"
>> 
>> +#define SYMLOOP_MAX    8
>> #define EXT4_NAME_MAX  255
>> +#define EFI_PATH_MAX   4096
>> 
>> #define EXT4_DRIVER_VERSION  0x0000
>> 
>> @@ -324,11 +326,11 @@ number of read bytes.
>> **/
>> EFI_STATUS
>> Ext4Read (
>> -  IN EXT4_PARTITION  *Partition,
>> -  IN EXT4_FILE       *File,
>> -  OUT VOID           *Buffer,
>> -  IN UINT64          Offset,
>> -  IN OUT UINTN       *Length
>> +  IN     EXT4_PARTITION  *Partition,
>> +  IN     EXT4_FILE       *File,
>> +  OUT    VOID            *Buffer,
>> +  IN     UINT64          Offset,
>> +  IN OUT UINTN           *Length
>>  );
>> 
>> /**
>> @@ -368,6 +370,7 @@ struct _Ext4File {
>> 
>>  UINT64                OpenMode;
>>  UINT64                Position;
>> +  UINT32                SymLoops;
>> 
>>  EXT4_PARTITION        *Partition;
>> 
>> @@ -497,6 +500,45 @@ Ext4SetupFile (
>>  IN EXT4_PARTITION  *Partition
>>  );
>> 
>> +/**
>> +  Opens a new file relative to the source file's location.
>> +
>> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
>> +                         file.
>> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
>> +                         handle to the source location. This would typically be an open
>> +                         handle to a directory.
>> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
>> +                         The file name may contain the following path modifiers: "\", ".",
>> +                         and "..".
>> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
>> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
>> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
>> +                         attribute bits for the newly created file.
>> +
>> +  @retval EFI_SUCCESS          The file was opened.
>> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
>> +  @retval EFI_NO_MEDIA         The device has no medium.
>> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
>> +                               longer supported.
>> +  @retval EFI_DEVICE_ERROR     The device reported an error.
>> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
>> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
>> +                               when the media is write-protected.
>> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
>> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
>> +  @retval EFI_VOLUME_FULL      The volume is full.
>> +
>> +**/
>> +EFI_STATUS
>> +Ext4OpenInternal (
>> +  OUT EXT4_FILE  **FoundFile,
>> +  IN  EXT4_FILE  *Source,
>> +  IN  CHAR16     *FileName,
>> +  IN  UINT64     OpenMode,
>> +  IN  UINT64     Attributes
>> +  );
>> +
>> /**
>>   Closes a file.
>> 
>> @@ -774,6 +816,30 @@ Ext4FileIsDir (
>>  IN CONST EXT4_FILE  *File
>>  );
>> 
>> +/**
>> +   Checks if a file is a symlink.
>> +
>> +   @param[in]      File          Pointer to the opened file.
>> +
>> +   @return BOOLEAN         Whether file is a symlink
>> +**/
>> +BOOLEAN
>> +Ext4FileIsSymlink (
>> +  IN CONST EXT4_FILE  *File
>> +  );
>> +
>> +/**
>> +   Detects if a symlink is a fast symlink.
>> +
>> +   @param[in]      File          Pointer to the opened file.
>> +
>> +   @return BOOLEAN         Whether symlink is a fast symlink
>> +**/
>> +BOOLEAN
>> +Ext4SymlinkIsFastSymlink (
>> +  IN CONST EXT4_FILE  *File
>> +  );
>> +
>> /**
>>   Checks if a file is a regular file.
>>   @param[in]      File          Pointer to the opened file.
>> @@ -797,7 +863,7 @@ Ext4FileIsReg (
>>           it's a regular file or a directory, since most other file types
>>           don't make sense under UEFI.
>> **/
>> -#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) || Ext4FileIsDir(File))
>> +#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File))
>> 
>> #define EXT4_INODE_HAS_FIELD(Inode, Field)                                     \
>>  (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=                          \
>> @@ -935,6 +1001,26 @@ Ext4ReadDir (
>>  IN OUT UINTN       *OutLength
>>  );
>> 
>> +/**
>> +  Reads a symlink file.
>> +
>> +  @param[in]      Partition   Pointer to the ext4 partition.
>> +  @param[in]      File        Pointer to the open symlink file.
>> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
>> +
>> +  @retval EFI_SUCCESS           Symlink was read.
>> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
>> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
>> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
>> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
>> +**/
>> +EFI_STATUS
>> +Ext4ReadSymlink (
>> +  IN     EXT4_PARTITION  *Partition,
>> +  IN     EXT4_FILE       *File,
>> +  OUT    CHAR16          **Symlink
>> +  );
>> +
>> /**
>>   Initialises the (empty) extents map, that will work as a cache of extents.
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
>> index ff1746d5640a..ae9230d6422b 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
>> @@ -134,14 +134,224 @@ Ext4DirCanLookup (
>>  return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) == EXT4_INO_PERM_EXEC_OWNER;
>> }
>> 
>> +/**
>> +  Reads a fast symlink file.
>> +
>> +  @param[in]      Partition   Pointer to the ext4 partition.
>> +  @param[in]      File        Pointer to the open symlink file.
>> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
>> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
>> +
>> +  @retval EFI_SUCCESS            Fast symlink was read.
>> +  @retval EFI_OUT_OF_RESOURCES   Memory allocation error.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +Ext4ReadFastSymlink (
>> +  IN     EXT4_PARTITION  *Partition,
>> +  IN     EXT4_FILE       *File,
>> +  OUT    CHAR8           **AsciiSymlink,
>> +  OUT    UINT32          *AsciiSymlinkSize
>> +  )
>> +{
>> +  UINT32  SymlinkSize;
>> +  CHAR8   *AsciiSymlinkTmp;
>> +  //
>> +  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we checked it in
>> +  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
>> +  //
>> +  SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), EXT4_FAST_SYMLINK_MAX_SIZE);
>> +
>> +  AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
>> +  if (AsciiSymlinkTmp == NULL) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
>> +
>> +  //
>> +  // Add null-terminator
>> +  //
>> +  AsciiSymlinkTmp[SymlinkSize] = '\0';
>> +
>> +  *AsciiSymlink     = AsciiSymlinkTmp;
>> +  *AsciiSymlinkSize = SymlinkSize + 1;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Reads a slow symlink file.
>> +
>> +  @param[in]      Partition        Pointer to the ext4 partition.
>> +  @param[in]      File             Pointer to the open symlink file.
>> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
>> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
>> +
>> +  @retval EFI_SUCCESS           Slow symlink was read.
>> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
>> +  @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
>> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +Ext4ReadSlowSymlink (
>> +  IN     EXT4_PARTITION  *Partition,
>> +  IN     EXT4_FILE       *File,
>> +  OUT    CHAR8           **AsciiSymlink,
>> +  OUT    UINT32          *AsciiSymlinkSize
>> +  )
>> +{
>> +  EFI_STATUS  Status;
>> +  CHAR8       *SymlinkTmp;
>> +  UINT64      SymlinkSizeTmp;
>> +  UINT32      SymlinkAllocateSize;
>> +  UINTN       ReadSize;
>> +
>> +  SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
>> +
>> +  //
>> +  // Allocate EXT4_INODE_SIZE + 1
>> +  //
>> +  if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
>> +    DEBUG ((
>> +      DEBUG_FS,
>> +      "[ext4] Error! Symlink path maximum length was hit!\n"
>> +      ));
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
>> +
>> +  SymlinkTmp = AllocatePool (SymlinkAllocateSize);
>> +  if (SymlinkTmp == NULL) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  ReadSize = (UINTN)SymlinkSizeTmp;
>> +  Status   = Ext4Read (Partition, File, SymlinkTmp, File->Position, &ReadSize);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with Status %r\n", Status));
>> +    FreePool (SymlinkTmp);
>> +    return Status;
>> +  }
>> +
>> +  File->Position += ReadSize;
>> +
>> +  //
>> +  // Add null-terminator
>> +  //
>> +  SymlinkTmp[SymlinkSizeTmp] = '\0';
>> +
>> +  if (SymlinkSizeTmp != ReadSize) {
>> +    DEBUG ((
>> +      DEBUG_FS,
>> +      "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n"
>> +      ));
>> +    return EFI_VOLUME_CORRUPTED;
>> +  }
>> +
>> +  *AsciiSymlinkSize = SymlinkAllocateSize;
>> +  *AsciiSymlink     = SymlinkTmp;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Reads a symlink file.
>> +
>> +  @param[in]      Partition   Pointer to the ext4 partition.
>> +  @param[in]      File        Pointer to the open symlink file.
>> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
>> +
>> +  @retval EFI_SUCCESS           Symlink was read.
>> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
>> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
>> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
>> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
>> +**/
>> +EFI_STATUS
>> +Ext4ReadSymlink (
>> +  IN     EXT4_PARTITION  *Partition,
>> +  IN     EXT4_FILE       *File,
>> +  OUT    CHAR16          **Symlink
>> +  )
>> +{
>> +  EFI_STATUS  Status;
>> +  CHAR8       *SymlinkTmp;
>> +  UINT32      SymlinkSize;
>> +  CHAR16      *Symlink16Tmp;
>> +  CHAR16      *Needle;
>> +
>> +  //
>> +  // Assume that we alread read Inode via Ext4ReadInode
>> +  // Skip reading, just check encryption flag
>> +  //
>> +  if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
>> +    return EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if (Ext4SymlinkIsFastSymlink (File)) {
>> +    Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
>> +  } else {
>> +    Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
>> +  }
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n", Status));
>> +    return Status;
>> +  }
>> +
>> +  Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));
> 
> Any specific reason to use ZeroPool here, when the rest of the code doesn’t?
> 
>> +  if (Symlink16Tmp == NULL) {
>> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n"));
>> +    FreePool (SymlinkTmp);
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Status = AsciiStrToUnicodeStrS (
>> +             SymlinkTmp,
>> +             Symlink16Tmp,
>> +             SymlinkSize
>> +             );
> 
> Can’t SymlinkTmp be free’d here?
> 
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((
>> +      DEBUG_FS,
>> +      "[ext4] Failed to convert ascii symlink to unicode with Status %r\n",
>> +      Status
>> +      ));
>> +    FreePool (Symlink16Tmp);
>> +    FreePool (SymlinkTmp);
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // Convert to UEFI slashes
>> +  //
>> +  for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
>> +    if (*Needle == L'/') {
>> +      *Needle = L'\\';
>> +    }
>> +  }
>> +
>> +  *Symlink = Symlink16Tmp;
>> +
>> +  FreePool (SymlinkTmp);
>> +  return Status;
>> +}
>> +
>> /**
>>  Opens a new file relative to the source file's location.
>> 
>> -  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
>> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
>> +                         file.
>> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
>>                         handle to the source location. This would typically be an open
>>                         handle to a directory.
>> -  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
>> -                         file.
>>  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
>>                         The file name may contain the following path modifiers: "\", ".",
>>                         and "..".
>> @@ -165,13 +375,12 @@ Ext4DirCanLookup (
>> 
>> **/
>> EFI_STATUS
>> -EFIAPI
>> -Ext4Open (
>> -  IN EFI_FILE_PROTOCOL   *This,
>> -  OUT EFI_FILE_PROTOCOL  **NewHandle,
>> -  IN CHAR16              *FileName,
>> -  IN UINT64              OpenMode,
>> -  IN UINT64              Attributes
>> +Ext4OpenInternal (
>> +  OUT EXT4_FILE  **FoundFile,
>> +  IN  EXT4_FILE  *Source,
>> +  IN  CHAR16     *FileName,
>> +  IN  UINT64     OpenMode,
>> +  IN  UINT64     Attributes
>>  )
>> {
>>  EXT4_FILE       *Current;
>> @@ -180,13 +389,14 @@ Ext4Open (
>>  CHAR16          PathSegment[EXT4_NAME_MAX + 1];
>>  UINTN           Length;
>>  EXT4_FILE       *File;
>> +  CHAR16          *Symlink;
>>  EFI_STATUS      Status;
>> 
>> -  Current   = (EXT4_FILE *)This;
>> +  Current   = Source;
>>  Partition = Current->Partition;
>>  Level     = 0;
>> 
>> -  DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
>> +  DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
>>  // If the path starts with a backslash, we treat the root directory as the base directory
>>  if (FileName[0] == L'\\') {
>>    FileName++;
>> @@ -194,6 +404,11 @@ Ext4Open (
>>  }
>> 
>>  while (FileName[0] != L'\0') {
>> +    if (Partition->Root->SymLoops > SYMLOOP_MAX) {
>> +      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
>> +      return EFI_ACCESS_DENIED;
>> +    }
>> +
>>    // Discard leading path separators
>>    while (FileName[0] == L'\\') {
>>      FileName++;
>> @@ -238,18 +453,45 @@ Ext4Open (
>>    }
>> 
>>    // Check if this is a valid file to open in EFI
>> -
>> -    // What to do with symlinks? They're nonsense when absolute but may
>> -    // be useful when they're relative. Right now, they're ignored, since they
>> -    // bring a lot of trouble for something that's not as useful in our case.
>> -    // If you want to link, use hard links.
>> -
>>    if (!Ext4FileIsOpenable (File)) {
>>      Ext4CloseInternal (File);
>>      // This looks like an /okay/ status to return.
>>      return EFI_ACCESS_DENIED;
>>    }
>> 
>> +    //
>> +    // Reading symlink and then trying to follow it
>> +    //
>> +    if (Ext4FileIsSymlink (File)) {
>> +      Partition->Root->SymLoops++;
>> +      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n", PathSegment));
>> +      Status = Ext4ReadSymlink (Partition, File, &Symlink);
>> +      if (EFI_ERROR (Status)) {
>> +        DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n", PathSegment));
>> +        return Status;
>> +      }
>> +
>> +      DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment, Symlink));
>> +      //
>> +      // Close symlink file
>> +      //
>> +      Ext4CloseInternal (File);
>> +      //
>> +      // Open linked file by recursive call of Ext4OpenFile
>> +      //
>> +      Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode, Attributes);
>> +      FreePool (Symlink);
>> +      if (EFI_ERROR (Status)) {
>> +        DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n", Symlink));
>> +        return Status;
>> +      }
>> +
>> +      //
>> +      // Set File to newly opened
>> +      //
>> +      File = *FoundFile;
>> +    }
>> +
>>    if (Level != 0) {
>>      // Careful not to close the base directory
>>      Ext4CloseInternal (Current);
>> @@ -273,12 +515,75 @@ Ext4Open (
>>    return EFI_ACCESS_DENIED;
>>  }
>> 
>> -  *NewHandle = &Current->Protocol;
>> +  *FoundFile = Current;
>> 
>>  DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
>>  return EFI_SUCCESS;
>> }
>> 
>> +/**
>> +  Opens a new file relative to the source file's location.
>> +  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
>> +                         handle to the source location. This would typically be an open
>> +                         handle to a directory.
>> +  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
>> +                         file.
>> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
>> +                         The file name may contain the following path modifiers: "\", ".",
>> +                         and "..".
>> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
>> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
>> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
>> +                         attribute bits for the newly created file.
>> +  @retval EFI_SUCCESS          The file was opened.
>> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
>> +  @retval EFI_NO_MEDIA         The device has no medium.
>> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
>> +                               longer supported.
>> +  @retval EFI_DEVICE_ERROR     The device reported an error.
>> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
>> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
>> +                               when the media is write-protected.
>> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
>> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
>> +  @retval EFI_VOLUME_FULL      The volume is full.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +Ext4Open (
>> +  IN EFI_FILE_PROTOCOL   *This,
>> +  OUT EFI_FILE_PROTOCOL  **NewHandle,
>> +  IN CHAR16              *FileName,
>> +  IN UINT64              OpenMode,
>> +  IN UINT64              Attributes
>> +  )
>> +{
>> +  EFI_STATUS  Status;
>> +  EXT4_FILE   *FoundFile;
>> +  EXT4_FILE   *Source;
>> +
>> +  Source = (EXT4_FILE *)This;
>> +
>> +  //
>> +  // Reset SymLoops counter
>> +  //
>> +  Source->Partition->Root->SymLoops = 0;
>> +
>> +  Status = Ext4OpenInternal (
>> +             &FoundFile,
>> +             Source,
>> +             FileName,
>> +             OpenMode,
>> +             Attributes
>> +             );
>> +
>> +  if (!EFI_ERROR (Status)) {
>> +    *NewHandle = &FoundFile->Protocol;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> /**
>>  Closes a specified file handle.
>> 
>> @@ -588,7 +893,7 @@ Ext4GetVolumeName (
>> 
>>  // s_volume_name is only valid on dynamic revision; old filesystems don't support this
>>  if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
>> -    CopyMem (TempVolName, (CONST CHAR8 *)Partition->SuperBlock.s_volume_name, 16);
>> +    CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
>>    TempVolName[16] = '\0';
>> 
>>    Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
>> @@ -754,12 +1059,14 @@ Ext4GetInfo (
>>  OUT VOID              *Buffer
>>  )
>> {
>> +  EXT4_FILE       *File;
>>  EXT4_PARTITION  *Partition;
>> 
>> -  Partition = ((EXT4_FILE *)This)->Partition;
>> +  File      = (EXT4_FILE *)This;
>> +  Partition = File->Partition;
>> 
>>  if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
>> -    return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
>> +    return Ext4GetFileInfo (File, Buffer, BufferSize);
>>  }
>> 
>>  if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
>> @@ -870,12 +1177,12 @@ Ext4SetInfo (
>>  )
>> {
>>  EXT4_FILE       *File;
>> -  EXT4_PARTITION  *Part;
>> +  EXT4_PARTITION  *Partition;
>> 
>> -  File = (EXT4_FILE *)This;
>> -  Part = File->Partition;
>> +  File      = (EXT4_FILE *)This;
>> +  Partition = File->Partition;
>> 
>> -  if (Part->ReadOnly) {
>> +  if (Partition->ReadOnly) {
>>    return EFI_WRITE_PROTECTED;
>>  }
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index 831f5946e870..e7a6b3225709 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> @@ -255,6 +255,59 @@ Ext4FileIsDir (
>>  return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
>> }
>> 
>> +/**
>> +   Checks if a file is a symlink.
>> +
>> +   @param[in]      File          Pointer to the opened file.
>> +
>> +   @return BOOLEAN         Whether file is a symlink
>> +**/
>> +BOOLEAN
>> +Ext4FileIsSymlink (
>> +  IN CONST EXT4_FILE  *File
>> +  )
>> +{
>> +  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) == EXT4_INO_TYPE_SYMLINK;
>> +}
>> +
>> +/**
>> +   Detects if a symlink is a fast symlink.
>> +
>> +   @param[in]      File          Pointer to the opened file.
>> +
>> +   @return BOOLEAN         Whether symlink is a fast symlink
>> +**/
>> +BOOLEAN
>> +Ext4SymlinkIsFastSymlink (
>> +  IN CONST EXT4_FILE  *File
>> +  )
>> +{
>> +  //
>> +  // Detection logic of the fast-symlink splits into two behaviors - old and new.
>> +  // The old behavior is based on comparing the extended attribute blocks
>> +  // with the inode's i_blocks, and if it's zero we know the inode isn't storing
>> +  // the link in filesystem blocks, so we look to the inode->i_data.
>> +  // The new behavior is apparently needed only with the large EA inode feature.
>> +  // In this case we check that inode size less than maximum fast symlink size.
>> +  // So, we revert to the old behavior if the large EA inode feature is not set.
>> +  //
>> +  UINT32  FileAcl;
>> +  UINT32  ExtAttrBlocks;
>> +
>> +  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
>> +    FileAcl = File->Inode->i_file_acl;
>> +    if (EXT4_IS_64_BIT (File->Partition)) {
>> +      FileAcl |= LShiftU64 (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);
> 
> *If* you happen to do a new revision anyway, you could drop the shift (with a comment!) if you want, because we don’t care about the actual value, just whether any one bit is set. Don’t bother if there won’t be a new revision anyway, though. :)
> 
>> +    }
>> +
>> +    ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
>> +
>> +    return File->Inode->i_blocks == ExtAttrBlocks;
>> +  }
>> +
>> +  return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
>> +}
>> +
>> /**
>>   Checks if a file is a regular file.
>>   @param[in]      File          Pointer to the opened file.
>> -- 
>> 2.37.1
>> 
> 

Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support

[Pedro Falcato]

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

Hi Savva,

Sorry for the huge delay. Comments inline.

On Thu, Jul 28, 2022 at 4:26 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:

> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677
>
> Provided support for symlink file type. Added routine which allows
> reading and following them through recursive open() call. As a security
> meausure implemented simple symlink loop check with nest level limit
> equal 8. Also this patch moves Ext4Open functionality to internal
> routine.
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
>  Features/Ext4Pkg/Ext4Dxe/File.c     | 359 ++++++++++++++++++--
>  Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
>  4 files changed, 485 insertions(+), 38 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index a55cd2fa68ad..a73e3f8622f1 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -171,7 +171,7 @@
>  #define EXT4_DIRTY_FL         0x00000100
>  #define EXT4_COMPRBLK_FL      0x00000200
>  #define EXT4_NOCOMPR_FL       0x00000400
> -#define EXT4_ECOMPR_FL        0x00000800
> +#define EXT4_ENCRYPT_FL       0x00000800
>  #define EXT4_BTREE_FL         0x00001000
>  #define EXT4_INDEX_FL         0x00002000
>  #define EXT4_JOURNAL_DATA_FL  0x00004000
> @@ -332,11 +332,12 @@ STATIC_ASSERT (
>    "ext4 block group descriptor struct has incorrect size"
>    );
>
> -#define EXT4_DBLOCKS     12
> -#define EXT4_IND_BLOCK   12
> -#define EXT4_DIND_BLOCK  13
> -#define EXT4_TIND_BLOCK  14
> -#define EXT4_NR_BLOCKS   15
> +#define EXT4_DBLOCKS                12
> +#define EXT4_IND_BLOCK              12
> +#define EXT4_DIND_BLOCK             13
> +#define EXT4_TIND_BLOCK             14
> +#define EXT4_NR_BLOCKS              15
> +#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * sizeof(UINT32)
>
>  #define EXT4_GOOD_OLD_INODE_SIZE  128
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index b1508482b0a7..c1df9d1149e4 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -31,7 +31,9 @@
>
>  #include "Ext4Disk.h"
>
> +#define SYMLOOP_MAX    8
>
I'm scared that this may not be enough. Could we use 40 here as Linux does?

>  #define EXT4_NAME_MAX  255
> +#define EFI_PATH_MAX   4096
>
Don't use EFI_* in Ext4Dxe code, so prefix this with EXT4_ instead maybe?

>
>  #define EXT4_DRIVER_VERSION  0x0000
>
> @@ -324,11 +326,11 @@ number of read bytes.
>  **/
>  EFI_STATUS
>  Ext4Read (
> -  IN EXT4_PARTITION  *Partition,
> -  IN EXT4_FILE       *File,
> -  OUT VOID           *Buffer,
> -  IN UINT64          Offset,
> -  IN OUT UINTN       *Length
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    VOID            *Buffer,
> +  IN     UINT64          Offset,
> +  IN OUT UINTN           *Length
>    );
>
>  /**
> @@ -368,6 +370,7 @@ struct _Ext4File {
>
>    UINT64                OpenMode;
>    UINT64                Position;
> +  UINT32                SymLoops;
>
>    EXT4_PARTITION        *Partition;
>
> @@ -497,6 +500,45 @@ Ext4SetupFile (
>    IN EXT4_PARTITION  *Partition
>    );
>
> +/**
> +  Opens a new file relative to the source file's location.
> +
> +  @param[out] FoundFile  A pointer to the location to return the opened
> handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the
> file
> +                         handle to the source location. This would
> typically be an open
> +                         handle to a directory.
> +  @param[in]  FileName   The Null-terminated string of the name of the
> file to be opened.
> +                         The file name may contain the following path
> modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid
> combinations that the
> +                         file may be opened with are: Read, Read/Write,
> or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which
> case these are the
> +                         attribute bits for the newly created file.
> +
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on
> the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or
> the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or
> open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to
> open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +
> +**/
> +EFI_STATUS
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
>
First IN parameters, then OUT parameters.

> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
> +  );
> +
>  /**
>     Closes a file.
>
> @@ -774,6 +816,30 @@ Ext4FileIsDir (
>    IN CONST EXT4_FILE  *File
>    );
>
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
>
Does this need to be in Ext4Dxe.h? This is pretty symlink specific.

> +
>  /**
>     Checks if a file is a regular file.
>     @param[in]      File          Pointer to the opened file.
> @@ -797,7 +863,7 @@ Ext4FileIsReg (
>             it's a regular file or a directory, since most other file types
>             don't make sense under UEFI.
>  **/
> -#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) ||
> Ext4FileIsDir(File))
> +#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || Ext4FileIsDir
> (File) || Ext4FileIsSymlink (File))
>
>  #define EXT4_INODE_HAS_FIELD(Inode, Field)
>      \
>    (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=
>       \
> @@ -935,6 +1001,26 @@ Ext4ReadDir (
>    IN OUT UINTN       *OutLength
>    );
>
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink
> string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode
> value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  );
> +
>  /**
>     Initialises the (empty) extents map, that will work as a cache of
> extents.
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c
> b/Features/Ext4Pkg/Ext4Dxe/File.c
> index ff1746d5640a..ae9230d6422b 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -134,14 +134,224 @@ Ext4DirCanLookup (
>    return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) ==
> EXT4_INO_PERM_EXEC_OWNER;
>  }
>
> +/**
> +  Reads a fast symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink
> string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink
> string length.
> +
> +  @retval EFI_SUCCESS            Fast symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES   Memory allocation error.
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadFastSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  UINT32  SymlinkSize;
> +  CHAR8   *AsciiSymlinkTmp;
> +  //
> +  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we
> checked it in
> +  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
> +  //
>
I think the correct solution here is to check if the inode size fits in
EXT4_FAST_SYMLINK_MAX_SIZE in Ext4SymlinkIsFastSymlink. Or possibly check
it here and error-out (blatant corruption).
Truncating doesn't seem correct.

> +  SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode),
> EXT4_FAST_SYMLINK_MAX_SIZE);
> +
> +  AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
> +  if (AsciiSymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string
> buffer\n"));
>
Nit: DEBUG_ERROR?

> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
> +
> +  //
> +  // Add null-terminator
> +  //
> +  AsciiSymlinkTmp[SymlinkSize] = '\0';
> +
> +  *AsciiSymlink     = AsciiSymlinkTmp;
> +  *AsciiSymlinkSize = SymlinkSize + 1;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a slow symlink file.
> +
> +  @param[in]      Partition        Pointer to the ext4 partition.
> +  @param[in]      File             Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink
> string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink
> string length.
> +
> +  @retval EFI_SUCCESS           Slow symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode
> value
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadSlowSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT64      SymlinkSizeTmp;
> +  UINT32      SymlinkAllocateSize;
> +  UINTN       ReadSize;
> +
> +  SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
> +
> +  //
> +  // Allocate EXT4_INODE_SIZE + 1
> +  //
> +  if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
>
Why is this > EFI_PATH_MAX - 1 and not >= EFI_PATH_MAX? Also, why does the
inode size need to be strictly smaller than EFI_PATH_MAX?

> +    DEBUG ((
> +      DEBUG_FS,
>
Nit: DEBUG_ERROR?

> +      "[ext4] Error! Symlink path maximum length was hit!\n"
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
> +
> +  SymlinkTmp = AllocatePool (SymlinkAllocateSize);
> +  if (SymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string
> buffer\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +

+  ReadSize = (UINTN)SymlinkSizeTmp;
> +  Status   = Ext4Read (Partition, File, SymlinkTmp, File->Position,
> &ReadSize);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with
> Status %r\n", Status));
>
Nit: lowercase "status" in the DEBUG.

> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  File->Position += ReadSize;
>
Taking Position into account (here and in the Ext4Read call) seems wrong.
Why would the file seek affect the readlink() operation?

> +
> +  //
> +  // Add null-terminator
> +  //
> +  SymlinkTmp[SymlinkSizeTmp] = '\0';
> +
> +  if (SymlinkSizeTmp != ReadSize) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Error! The sz of the read block doesn't match the value
> from the inode!\n"
>
Nit: size

> +      ));
> +    return EFI_VOLUME_CORRUPTED;
> +  }
> +
> +  *AsciiSymlinkSize = SymlinkAllocateSize;
> +  *AsciiSymlink     = SymlinkTmp;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink
> string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode
> value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT32      SymlinkSize;
> +  CHAR16      *Symlink16Tmp;
> +  CHAR16      *Needle;
> +
> +  //
> +  // Assume that we alread read Inode via Ext4ReadInode
> +  // Skip reading, just check encryption flag
> +  //
> +  if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
> +    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
>
Nit: DEBUG_ERROR and "Error: ".

> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  if (Ext4SymlinkIsFastSymlink (File)) {
> +    Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp,
> &SymlinkSize);
> +  } else {
> +    Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp,
> &SymlinkSize);
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n",
> Status));
> +    return Status;
> +  }
> +
> +  Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));
> +  if (Symlink16Tmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string
> buffer\n"));
> +    FreePool (SymlinkTmp);
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = AsciiStrToUnicodeStrS (
> +             SymlinkTmp,
> +             Symlink16Tmp,
> +             SymlinkSize
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Failed to convert ascii symlink to unicode with Status
> %r\n",
> +      Status
> +      ));
> +    FreePool (Symlink16Tmp);
> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  //
> +  // Convert to UEFI slashes
> +  //
> +  for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
> +    if (*Needle == L'/') {
> +      *Needle = L'\\';
> +    }
> +  }
> +
> +  *Symlink = Symlink16Tmp;
> +
> +  FreePool (SymlinkTmp);
> +  return Status;
> +}
> +
>  /**
>    Opens a new file relative to the source file's location.
>
> -  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that
> is the file
> +  @param[out] FoundFile  A pointer to the location to return the opened
> handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the
> file
>                           handle to the source location. This would
> typically be an open
>                           handle to a directory.
> -  @param[out] NewHandle  A pointer to the location to return the opened
> handle for the new
> -                         file.
>    @param[in]  FileName   The Null-terminated string of the name of the
> file to be opened.
>                           The file name may contain the following path
> modifiers: "\", ".",
>                           and "..".
> @@ -165,13 +375,12 @@ Ext4DirCanLookup (
>
>  **/
>  EFI_STATUS
> -EFIAPI
> -Ext4Open (
> -  IN EFI_FILE_PROTOCOL   *This,
> -  OUT EFI_FILE_PROTOCOL  **NewHandle,
> -  IN CHAR16              *FileName,
> -  IN UINT64              OpenMode,
> -  IN UINT64              Attributes
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
>    )
>  {
>    EXT4_FILE       *Current;
> @@ -180,13 +389,14 @@ Ext4Open (
>    CHAR16          PathSegment[EXT4_NAME_MAX + 1];
>    UINTN           Length;
>    EXT4_FILE       *File;
> +  CHAR16          *Symlink;
>    EFI_STATUS      Status;
>
> -  Current   = (EXT4_FILE *)This;
> +  Current   = Source;
>    Partition = Current->Partition;
>    Level     = 0;
>
> -  DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
> +  DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
>    // If the path starts with a backslash, we treat the root directory as
> the base directory
>    if (FileName[0] == L'\\') {
>      FileName++;
> @@ -194,6 +404,11 @@ Ext4Open (
>    }
>
>    while (FileName[0] != L'\0') {
> +    if (Partition->Root->



> > SYMLOOP_MAX) {
> +      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
> +      return EFI_ACCESS_DENIED;
> +    }
> +
>      // Discard leading path separators
>      while (FileName[0] == L'\\') {
>        FileName++;
> @@ -238,18 +453,45 @@ Ext4Open (
>      }
>
>      // Check if this is a valid file to open in EFI
> -
> -    // What to do with symlinks? They're nonsense when absolute but may
> -    // be useful when they're relative. Right now, they're ignored, since
> they
> -    // bring a lot of trouble for something that's not as useful in our
> case.
> -    // If you want to link, use hard links.
> -
>      if (!Ext4FileIsOpenable (File)) {
>        Ext4CloseInternal (File);
>        // This looks like an /okay/ status to return.
>        return EFI_ACCESS_DENIED;
>      }
>
> +    //
> +    // Reading symlink and then trying to follow it
> +    //
> +    if (Ext4FileIsSymlink (File)) {
> +      Partition->Root->SymLoops++;
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n",
> PathSegment));
> +      Status = Ext4ReadSymlink (Partition, File, &Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n",
> PathSegment));
> +        return Status;
> +      }
> +
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment,
> Symlink));
> +      //
> +      // Close symlink file
> +      //
> +      Ext4CloseInternal (File);
> +      //
> +      // Open linked file by recursive call of Ext4OpenFile
> +      //
> +      Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode,
> Attributes);
> +      FreePool (Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n",
> Symlink));
> +        return Status;
> +      }
> +
> +      //
> +      // Set File to newly opened
> +      //
> +      File = *FoundFile;
> +    }
> +
>      if (Level != 0) {
>        // Careful not to close the base directory
>        Ext4CloseInternal (Current);
> @@ -273,12 +515,75 @@ Ext4Open (
>      return EFI_ACCESS_DENIED;
>    }
>
> -  *NewHandle = &Current->Protocol;
> +  *FoundFile = Current;
>
>    DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n",
> Current->Dentry->Name));
>    return EFI_SUCCESS;
>  }
>
> +/**
> +  Opens a new file relative to the source file's location.
> +  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that
> is the file
> +                         handle to the source location. This would
> typically be an open
> +                         handle to a directory.
> +  @param[out] NewHandle  A pointer to the location to return the opened
> handle for the new
> +                         file.
> +  @param[in]  FileName   The Null-terminated string of the name of the
> file to be opened.
> +                         The file name may contain the following path
> modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid
> combinations that the
> +                         file may be opened with are: Read, Read/Write,
> or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which
> case these are the
> +                         attribute bits for the newly created file.
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on
> the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or
> the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or
> open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to
> open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +**/
> +EFI_STATUS
> +EFIAPI
> +Ext4Open (
> +  IN EFI_FILE_PROTOCOL   *This,
> +  OUT EFI_FILE_PROTOCOL  **NewHandle,
> +  IN CHAR16              *FileName,
> +  IN UINT64              OpenMode,
> +  IN UINT64              Attributes
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EXT4_FILE   *FoundFile;
> +  EXT4_FILE   *Source;
> +
> +  Source = (EXT4_FILE *)This;
> +
> +  //
> +  // Reset SymLoops counter
> +  //
> +  Source->Partition->Root->SymLoops = 0;
>
Keeping SymLoops in the root FILE seems kind of out of place. My idea here
(and this was going to be chained with a bunch of improvements to the path
walking logic, which is why I never got around to adding symlink support)
was to add a nameidata(see Onyx code, or NetBSD's namei(9) manpage)-like
structure, as you would in typical UNIX fashion. You would then do your
whole path traversal on that structure, which would keep your path walking
state, walked_symlinks, etc.

> +
> +  Status = Ext4OpenInternal (
> +             &FoundFile,
> +             Source,
> +             FileName,
> +             OpenMode,
> +             Attributes
> +             );
> +
> +  if (!EFI_ERROR (Status)) {
> +    *NewHandle = &FoundFile->Protocol;
> +  }
> +
> +  return Status;
> +}
> +
>  /**
>    Closes a specified file handle.
>
> @@ -588,7 +893,7 @@ Ext4GetVolumeName (
>
>    // s_volume_name is only valid on dynamic revision; old filesystems
> don't support this
>    if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
> -    CopyMem (TempVolName, (CONST CHAR8
> *)Partition->SuperBlock.s_volume_name, 16);
> +    CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
>      TempVolName[16] = '\0';
>
>      Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
> @@ -754,12 +1059,14 @@ Ext4GetInfo (
>    OUT VOID              *Buffer
>    )
>  {
> +  EXT4_FILE       *File;
>    EXT4_PARTITION  *Partition;
>
> -  Partition = ((EXT4_FILE *)This)->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
>
>    if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
> -    return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
> +    return Ext4GetFileInfo (File, Buffer, BufferSize);
>    }
>
>    if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
> @@ -870,12 +1177,12 @@ Ext4SetInfo (
>    )
>  {
>    EXT4_FILE       *File;
> -  EXT4_PARTITION  *Part;
> +  EXT4_PARTITION  *Partition;
>
> -  File = (EXT4_FILE *)This;
> -  Part = File->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
>
> -  if (Part->ReadOnly) {
> +  if (Partition->ReadOnly) {
>      return EFI_WRITE_PROTECTED;
>    }
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 831f5946e870..e7a6b3225709 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -255,6 +255,59 @@ Ext4FileIsDir (
>    return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
>  }
>
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) ==
> EXT4_INO_TYPE_SYMLINK;
> +}
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  //
> +  // Detection logic of the fast-symlink splits into two behaviors - old
> and new.
> +  // The old behavior is based on comparing the extended attribute blocks
> +  // with the inode's i_blocks, and if it's zero we know the inode isn't
> storing
> +  // the link in filesystem blocks, so we look to the inode->i_data.
> +  // The new behavior is apparently needed only with the large EA inode
> feature.
> +  // In this case we check that inode size less than maximum fast symlink
> size.
> +  // So, we revert to the old behavior if the large EA inode feature is
> not set.
> +  //
> +  UINT32  FileAcl;
> +  UINT32  ExtAttrBlocks;
> +
> +  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
> +    FileAcl = File->Inode->i_file_acl;
> +    if (EXT4_IS_64_BIT (File->Partition)) {
> +      FileAcl |= LShiftU64
> (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);
> +    }
> +
> +    ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
> +
> +    return File->Inode->i_blocks == ExtAttrBlocks;
> +  }
> +
> +  return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
> +}
> +
>  /**
>     Checks if a file is a regular file.
>     @param[in]      File          Pointer to the opened file.
> --
> 2.37.1
>
>

-- 
Pedro Falcato

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

Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support

[Savva Mitrofanov]

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

Hi Pedro,

Thank you for your code-review. I answer to comments inline too.

> On 30 Aug 2022, at 04:12, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Hi Savva,
> 
> Sorry for the huge delay. Comments inline.
> 
> On Thu, Jul 28, 2022 at 4:26 PM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677 <https://bugzilla.tianocore.org/show_bug.cgi?id=3677>
> 
> Provided support for symlink file type. Added routine which allows
> reading and following them through recursive open() call. As a security
> meausure implemented simple symlink loop check with nest level limit
> equal 8. Also this patch moves Ext4Open functionality to internal
> routine.
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
> Cc: Pedro Falcato <pedro.falcato@gmail.com <mailto:pedro.falcato@gmail.com>>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h |  13 +-
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
>  Features/Ext4Pkg/Ext4Dxe/File.c     | 359 ++++++++++++++++++--
>  Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
>  4 files changed, 485 insertions(+), 38 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index a55cd2fa68ad..a73e3f8622f1 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -171,7 +171,7 @@
>  #define EXT4_DIRTY_FL         0x00000100
>  #define EXT4_COMPRBLK_FL      0x00000200
>  #define EXT4_NOCOMPR_FL       0x00000400
> -#define EXT4_ECOMPR_FL        0x00000800
> +#define EXT4_ENCRYPT_FL       0x00000800
>  #define EXT4_BTREE_FL         0x00001000
>  #define EXT4_INDEX_FL         0x00002000
>  #define EXT4_JOURNAL_DATA_FL  0x00004000
> @@ -332,11 +332,12 @@ STATIC_ASSERT (
>    "ext4 block group descriptor struct has incorrect size"
>    );
> 
> -#define EXT4_DBLOCKS     12
> -#define EXT4_IND_BLOCK   12
> -#define EXT4_DIND_BLOCK  13
> -#define EXT4_TIND_BLOCK  14
> -#define EXT4_NR_BLOCKS   15
> +#define EXT4_DBLOCKS                12
> +#define EXT4_IND_BLOCK              12
> +#define EXT4_DIND_BLOCK             13
> +#define EXT4_TIND_BLOCK             14
> +#define EXT4_NR_BLOCKS              15
> +#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * sizeof(UINT32)
> 
>  #define EXT4_GOOD_OLD_INODE_SIZE  128
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index b1508482b0a7..c1df9d1149e4 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -31,7 +31,9 @@
> 
>  #include "Ext4Disk.h"
> 
> +#define SYMLOOP_MAX    8
> I'm scared that this may not be enough. Could we use 40 here as Linux does?

As we discussed, the limit equal 8 should be compatible for booting all modern operation systems using this fs driver.
In this recursion-based symlink implementation it is a bit dangerous due to possible exceed of stack size. So we
decided keep this limit until we are do the lookup logic like namei.

>  #define EXT4_NAME_MAX  255
> +#define EFI_PATH_MAX   4096
> Don't use EFI_* in Ext4Dxe code, so prefix this with EXT4_ instead maybe?

As a result of consensus, it was decided to use EXT4_EFI_PATH_MAX.

> 
>  #define EXT4_DRIVER_VERSION  0x0000
> 
> @@ -324,11 +326,11 @@ number of read bytes.
>  **/
>  EFI_STATUS
>  Ext4Read (
> -  IN EXT4_PARTITION  *Partition,
> -  IN EXT4_FILE       *File,
> -  OUT VOID           *Buffer,
> -  IN UINT64          Offset,
> -  IN OUT UINTN       *Length
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    VOID            *Buffer,
> +  IN     UINT64          Offset,
> +  IN OUT UINTN           *Length
>    );
> 
>  /**
> @@ -368,6 +370,7 @@ struct _Ext4File {
> 
>    UINT64                OpenMode;
>    UINT64                Position;
> +  UINT32                SymLoops;
> 
>    EXT4_PARTITION        *Partition;
> 
> @@ -497,6 +500,45 @@ Ext4SetupFile (
>    IN EXT4_PARTITION  *Partition
>    );
> 
> +/**
> +  Opens a new file relative to the source file's location.
> +
> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
> +                         handle to the source location. This would typically be an open
> +                         handle to a directory.
> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
> +                         The file name may contain the following path modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
> +                         attribute bits for the newly created file.
> +
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +
> +**/
> +EFI_STATUS
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
> First IN parameters, then OUT parameters.

Due to lack of strict order conventions on procedure parameters, I consider to keep it unchanged, because all important C APIs have
destination first.

> 
> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
> +  );
> +
>  /**
>     Closes a file.
> 
> @@ -774,6 +816,30 @@ Ext4FileIsDir (
>    IN CONST EXT4_FILE  *File
>    );
> 
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  );
> Does this need to be in Ext4Dxe.h? This is pretty symlink specific. 

I agree with you, in the next patch I'll separate symlink-related functions into independent file.

> +
>  /**
>     Checks if a file is a regular file.
>     @param[in]      File          Pointer to the opened file.
> @@ -797,7 +863,7 @@ Ext4FileIsReg (
>             it's a regular file or a directory, since most other file types
>             don't make sense under UEFI.
>  **/
> -#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) || Ext4FileIsDir(File))
> +#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File))
> 
>  #define EXT4_INODE_HAS_FIELD(Inode, Field)                                     \
>    (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=                          \
> @@ -935,6 +1001,26 @@ Ext4ReadDir (
>    IN OUT UINTN       *OutLength
>    );
> 
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  );
> +
>  /**
>     Initialises the (empty) extents map, that will work as a cache of extents.
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
> index ff1746d5640a..ae9230d6422b 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -134,14 +134,224 @@ Ext4DirCanLookup (
>    return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) == EXT4_INO_PERM_EXEC_OWNER;
>  }
> 
> +/**
> +  Reads a fast symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
> +
> +  @retval EFI_SUCCESS            Fast symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES   Memory allocation error.
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadFastSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  UINT32  SymlinkSize;
> +  CHAR8   *AsciiSymlinkTmp;
> +  //
> +  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we checked it in
> +  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
> +  //
> I think the correct solution here is to check if the inode size fits in EXT4_FAST_SYMLINK_MAX_SIZE in Ext4SymlinkIsFastSymlink. Or possibly check it here and error-out (blatant corruption).
> Truncating doesn't seem correct.

The origin problem comes from old filesystems (ext2,ext3) which can have wrong inode size for symlinks. There are two behaviours on detection logic, the old one, 
which is suitable for old filesystems is based on checking presence of extended attribute and doesnt have inode size check. So in such case we just additionally truncate it by EXT4_FAST_SYMLINK_MAX_SIZE using MIN macro.

> +  SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), EXT4_FAST_SYMLINK_MAX_SIZE);
> +
> +  AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
> +  if (AsciiSymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
> Nit: DEBUG_ERROR? 
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
> +
> +  //
> +  // Add null-terminator
> +  //
> +  AsciiSymlinkTmp[SymlinkSize] = '\0';
> +
> +  *AsciiSymlink     = AsciiSymlinkTmp;
> +  *AsciiSymlinkSize = SymlinkSize + 1;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a slow symlink file.
> +
> +  @param[in]      Partition        Pointer to the ext4 partition.
> +  @param[in]      File             Pointer to the open symlink file.
> +  @param[out]     AsciiSymlink     Pointer to the output ascii symlink string.
> +  @param[out]     AsciiSymlinkSize Pointer to the output ascii symlink string length.
> +
> +  @retval EFI_SUCCESS           Slow symlink was read.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadSlowSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR8           **AsciiSymlink,
> +  OUT    UINT32          *AsciiSymlinkSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT64      SymlinkSizeTmp;
> +  UINT32      SymlinkAllocateSize;
> +  UINTN       ReadSize;
> +
> +  SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
> +
> +  //
> +  // Allocate EXT4_INODE_SIZE + 1
> +  //
> +  if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
> Why is this > EFI_PATH_MAX - 1 and not >= EFI_PATH_MAX? Also, why does the inode size need to be strictly smaller than EFI_PATH_MAX? 

Yes, my mistake, I'll correct it in the next patch.

> +    DEBUG ((
> +      DEBUG_FS,
> Nit: DEBUG_ERROR? 

I'm not agree that there we should use DEBUG_ERROR, because it will threats like fatal error, and it can breaks boot process in some configurations, that why I kept DEBUG_FS in this condition. The logic is simple - if something wrong with symlink then just skip it. So, as a result of the discussion, we came to a consensus to replace such places with DEBUG_WARN macros.


> +      "[ext4] Error! Symlink path maximum length was hit!\n"
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
> +
> +  SymlinkTmp = AllocatePool (SymlinkAllocateSize);
> +  if (SymlinkTmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> + 
> +  ReadSize = (UINTN)SymlinkSizeTmp;
> +  Status   = Ext4Read (Partition, File, SymlinkTmp, File->Position, &ReadSize);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with Status %r\n", Status));
> Nit: lowercase "status" in the DEBUG.
> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  File->Position += ReadSize;
> Taking Position into account (here and in the Ext4Read call) seems wrong. Why would the file seek affect the readlink() operation?

Yes, it's pointless there, I'll  remove it in the next revision.

> +
> +  //
> +  // Add null-terminator
> +  //
> +  SymlinkTmp[SymlinkSizeTmp] = '\0';
> +
> +  if (SymlinkSizeTmp != ReadSize) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n"
> Nit: size 
> +      ));
> +    return EFI_VOLUME_CORRUPTED;
> +  }
> +
> +  *AsciiSymlinkSize = SymlinkAllocateSize;
> +  *AsciiSymlink     = SymlinkTmp;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Reads a symlink file.
> +
> +  @param[in]      Partition   Pointer to the ext4 partition.
> +  @param[in]      File        Pointer to the open symlink file.
> +  @param[out]     Symlink     Pointer to the output unicode symlink string.
> +
> +  @retval EFI_SUCCESS           Symlink was read.
> +  @retval EFI_ACCESS_DENIED     Symlink is encrypted.
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
> +  @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> +  @retval EFI_VOLUME_CORRUPTED  Symlink read block size differ from inode value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> +  IN     EXT4_PARTITION  *Partition,
> +  IN     EXT4_FILE       *File,
> +  OUT    CHAR16          **Symlink
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CHAR8       *SymlinkTmp;
> +  UINT32      SymlinkSize;
> +  CHAR16      *Symlink16Tmp;
> +  CHAR16      *Needle;
> +
> +  //
> +  // Assume that we alread read Inode via Ext4ReadInode
> +  // Skip reading, just check encryption flag
> +  //
> +  if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
> +    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
> Nit: DEBUG_ERROR and "Error: ".
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  if (Ext4SymlinkIsFastSymlink (File)) {
> +    Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
> +  } else {
> +    Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp, &SymlinkSize);
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n", Status));
> +    return Status;
> +  }
> +
> +  Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));
> +  if (Symlink16Tmp == NULL) {
> +    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n"));
> +    FreePool (SymlinkTmp);
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = AsciiStrToUnicodeStrS (
> +             SymlinkTmp,
> +             Symlink16Tmp,
> +             SymlinkSize
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_FS,
> +      "[ext4] Failed to convert ascii symlink to unicode with Status %r\n",
> +      Status
> +      ));
> +    FreePool (Symlink16Tmp);
> +    FreePool (SymlinkTmp);
> +    return Status;
> +  }
> +
> +  //
> +  // Convert to UEFI slashes
> +  //
> +  for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
> +    if (*Needle == L'/') {
> +      *Needle = L'\\';
> +    }
> +  }
> +
> +  *Symlink = Symlink16Tmp;
> +
> +  FreePool (SymlinkTmp);
> +  return Status;
> +}
> +
>  /**
>    Opens a new file relative to the source file's location.
> 
> -  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
> +  @param[out] FoundFile  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  Source     A pointer to the EXT4_FILE instance that is the file
>                           handle to the source location. This would typically be an open
>                           handle to a directory.
> -  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
> -                         file.
>    @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
>                           The file name may contain the following path modifiers: "\", ".",
>                           and "..".
> @@ -165,13 +375,12 @@ Ext4DirCanLookup (
> 
>  **/
>  EFI_STATUS
> -EFIAPI
> -Ext4Open (
> -  IN EFI_FILE_PROTOCOL   *This,
> -  OUT EFI_FILE_PROTOCOL  **NewHandle,
> -  IN CHAR16              *FileName,
> -  IN UINT64              OpenMode,
> -  IN UINT64              Attributes
> +Ext4OpenInternal (
> +  OUT EXT4_FILE  **FoundFile,
> +  IN  EXT4_FILE  *Source,
> +  IN  CHAR16     *FileName,
> +  IN  UINT64     OpenMode,
> +  IN  UINT64     Attributes
>    )
>  {
>    EXT4_FILE       *Current;
> @@ -180,13 +389,14 @@ Ext4Open (
>    CHAR16          PathSegment[EXT4_NAME_MAX + 1];
>    UINTN           Length;
>    EXT4_FILE       *File;
> +  CHAR16          *Symlink;
>    EFI_STATUS      Status;
> 
> -  Current   = (EXT4_FILE *)This;
> +  Current   = Source;
>    Partition = Current->Partition;
>    Level     = 0;
> 
> -  DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
> +  DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
>    // If the path starts with a backslash, we treat the root directory as the base directory
>    if (FileName[0] == L'\\') {
>      FileName++;
> @@ -194,6 +404,11 @@ Ext4Open (
>    }
> 
>    while (FileName[0] != L'\0') {
> +    if (Partition->Root->
>  
> > SYMLOOP_MAX) {
> +      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
> +      return EFI_ACCESS_DENIED;
> +    }
> +
>      // Discard leading path separators
>      while (FileName[0] == L'\\') {
>        FileName++;
> @@ -238,18 +453,45 @@ Ext4Open (
>      }
> 
>      // Check if this is a valid file to open in EFI
> -
> -    // What to do with symlinks? They're nonsense when absolute but may
> -    // be useful when they're relative. Right now, they're ignored, since they
> -    // bring a lot of trouble for something that's not as useful in our case.
> -    // If you want to link, use hard links.
> -
>      if (!Ext4FileIsOpenable (File)) {
>        Ext4CloseInternal (File);
>        // This looks like an /okay/ status to return.
>        return EFI_ACCESS_DENIED;
>      }
> 
> +    //
> +    // Reading symlink and then trying to follow it
> +    //
> +    if (Ext4FileIsSymlink (File)) {
> +      Partition->Root->SymLoops++;
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n", PathSegment));
> +      Status = Ext4ReadSymlink (Partition, File, &Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n", PathSegment));
> +        return Status;
> +      }
> +
> +      DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment, Symlink));
> +      //
> +      // Close symlink file
> +      //
> +      Ext4CloseInternal (File);
> +      //
> +      // Open linked file by recursive call of Ext4OpenFile
> +      //
> +      Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode, Attributes);
> +      FreePool (Symlink);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n", Symlink));
> +        return Status;
> +      }
> +
> +      //
> +      // Set File to newly opened
> +      //
> +      File = *FoundFile;
> +    }
> +
>      if (Level != 0) {
>        // Careful not to close the base directory
>        Ext4CloseInternal (Current);
> @@ -273,12 +515,75 @@ Ext4Open (
>      return EFI_ACCESS_DENIED;
>    }
> 
> -  *NewHandle = &Current->Protocol;
> +  *FoundFile = Current;
> 
>    DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Opens a new file relative to the source file's location.
> +  @param[in]  This       A pointer to the EFI_FILE_PROTOCOL instance that is the file
> +                         handle to the source location. This would typically be an open
> +                         handle to a directory.
> +  @param[out] NewHandle  A pointer to the location to return the opened handle for the new
> +                         file.
> +  @param[in]  FileName   The Null-terminated string of the name of the file to be opened.
> +                         The file name may contain the following path modifiers: "\", ".",
> +                         and "..".
> +  @param[in]  OpenMode   The mode to open the file. The only valid combinations that the
> +                         file may be opened with are: Read, Read/Write, or Create/Read/Write.
> +  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the
> +                         attribute bits for the newly created file.
> +  @retval EFI_SUCCESS          The file was opened.
> +  @retval EFI_NOT_FOUND        The specified file could not be found on the device.
> +  @retval EFI_NO_MEDIA         The device has no medium.
> +  @retval EFI_MEDIA_CHANGED    The device has a different medium in it or the medium is no
> +                               longer supported.
> +  @retval EFI_DEVICE_ERROR     The device reported an error.
> +  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> +  @retval EFI_WRITE_PROTECTED  An attempt was made to create a file, or open a file for write
> +                               when the media is write-protected.
> +  @retval EFI_ACCESS_DENIED    The service denied access to the file.
> +  @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file.
> +  @retval EFI_VOLUME_FULL      The volume is full.
> +**/
> +EFI_STATUS
> +EFIAPI
> +Ext4Open (
> +  IN EFI_FILE_PROTOCOL   *This,
> +  OUT EFI_FILE_PROTOCOL  **NewHandle,
> +  IN CHAR16              *FileName,
> +  IN UINT64              OpenMode,
> +  IN UINT64              Attributes
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EXT4_FILE   *FoundFile;
> +  EXT4_FILE   *Source;
> +
> +  Source = (EXT4_FILE *)This;
> +
> +  //
> +  // Reset SymLoops counter
> +  //
> +  Source->Partition->Root->SymLoops = 0;
> Keeping SymLoops in the root FILE seems kind of out of place. My idea here (and this was going to be chained with a bunch of improvements to the path walking logic, which is why I never got around to adding symlink support) was to add a nameidata(see Onyx code, or NetBSD's namei(9) manpage)-like structure, as you would in typical UNIX fashion. You would then do your whole path traversal on that structure, which would keep your path walking state, walked_symlinks, etc.

We will fix it in the future patches, when we will provide namei-like mechanism for proceeding symlinks.

> +
> +  Status = Ext4OpenInternal (
> +             &FoundFile,
> +             Source,
> +             FileName,
> +             OpenMode,
> +             Attributes
> +             );
> +
> +  if (!EFI_ERROR (Status)) {
> +    *NewHandle = &FoundFile->Protocol;
> +  }
> +
> +  return Status;
> +}
> +
>  /**
>    Closes a specified file handle.
> 
> @@ -588,7 +893,7 @@ Ext4GetVolumeName (
> 
>    // s_volume_name is only valid on dynamic revision; old filesystems don't support this
>    if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
> -    CopyMem (TempVolName, (CONST CHAR8 *)Partition->SuperBlock.s_volume_name, 16);
> +    CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
>      TempVolName[16] = '\0';
> 
>      Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
> @@ -754,12 +1059,14 @@ Ext4GetInfo (
>    OUT VOID              *Buffer
>    )
>  {
> +  EXT4_FILE       *File;
>    EXT4_PARTITION  *Partition;
> 
> -  Partition = ((EXT4_FILE *)This)->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
> 
>    if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
> -    return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
> +    return Ext4GetFileInfo (File, Buffer, BufferSize);
>    }
> 
>    if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
> @@ -870,12 +1177,12 @@ Ext4SetInfo (
>    )
>  {
>    EXT4_FILE       *File;
> -  EXT4_PARTITION  *Part;
> +  EXT4_PARTITION  *Partition;
> 
> -  File = (EXT4_FILE *)This;
> -  Part = File->Partition;
> +  File      = (EXT4_FILE *)This;
> +  Partition = File->Partition;
> 
> -  if (Part->ReadOnly) {
> +  if (Partition->ReadOnly) {
>      return EFI_WRITE_PROTECTED;
>    }
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 831f5946e870..e7a6b3225709 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -255,6 +255,59 @@ Ext4FileIsDir (
>    return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
>  }
> 
> +/**
> +   Checks if a file is a symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) == EXT4_INO_TYPE_SYMLINK;
> +}
> +
> +/**
> +   Detects if a symlink is a fast symlink.
> +
> +   @param[in]      File          Pointer to the opened file.
> +
> +   @return BOOLEAN         Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> +  IN CONST EXT4_FILE  *File
> +  )
> +{
> +  //
> +  // Detection logic of the fast-symlink splits into two behaviors - old and new.
> +  // The old behavior is based on comparing the extended attribute blocks
> +  // with the inode's i_blocks, and if it's zero we know the inode isn't storing
> +  // the link in filesystem blocks, so we look to the inode->i_data.
> +  // The new behavior is apparently needed only with the large EA inode feature.
> +  // In this case we check that inode size less than maximum fast symlink size.
> +  // So, we revert to the old behavior if the large EA inode feature is not set.
> +  //
> +  UINT32  FileAcl;
> +  UINT32  ExtAttrBlocks;
> +
> +  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
> +    FileAcl = File->Inode->i_file_acl;
> +    if (EXT4_IS_64_BIT (File->Partition)) {
> +      FileAcl |= LShiftU64 (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);
> +    }
> +
> +    ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
> +
> +    return File->Inode->i_blocks == ExtAttrBlocks;
> +  }
> +
> +  return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
> +}
> +
>  /**
>     Checks if a file is a regular file.
>     @param[in]      File          Pointer to the opened file.
> -- 
> 2.37.1
> 
> 
> 
> -- 
> Pedro Falcato

All the best!
Savva Mitrofanov


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

[edk2-platforms][PATCH v4 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE

[Savva Mitrofanov]

We shouldn't use direct casts, because in the future it could break
the code, so using BASE_CR would be safe against possible structure
changes and rearrangements

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h |  2 ++
 Features/Ext4Pkg/Ext4Dxe/File.c    | 16 ++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index c1df9d1149e4..1e63e6282fa5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -382,6 +382,8 @@ struct _Ext4File {
   EXT4_DENTRY           *Dentry;
 };
 
+#define EXT4_FILE_FROM_THIS(This)  BASE_CR ((This), EXT4_FILE, Protocol)
+
 #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 ae9230d6422b..b1744ad56c98 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -562,7 +562,7 @@ Ext4Open (
   EXT4_FILE   *FoundFile;
   EXT4_FILE   *Source;
 
-  Source = (EXT4_FILE *)This;
+  Source = EXT4_FILE_FROM_THIS (This);
 
   //
   // Reset SymLoops counter
@@ -599,7 +599,7 @@ Ext4Close (
   IN EFI_FILE_PROTOCOL  *This
   )
 {
-  return Ext4CloseInternal ((EXT4_FILE *)This);
+  return Ext4CloseInternal (EXT4_FILE_FROM_THIS (This));
 }
 
 /**
@@ -680,7 +680,7 @@ Ext4ReadFile (
   EXT4_PARTITION  *Partition;
   EFI_STATUS      Status;
 
-  File      = (EXT4_FILE *)This;
+  File      = EXT4_FILE_FROM_THIS (This);
   Partition = File->Partition;
 
   ASSERT (Ext4FileIsOpenable (File));
@@ -731,7 +731,7 @@ Ext4WriteFile (
 {
   EXT4_FILE  *File;
 
-  File = (EXT4_FILE *)This;
+  File = EXT4_FILE_FROM_THIS (This);
 
   if (!(File->OpenMode & EFI_FILE_MODE_WRITE)) {
     return EFI_ACCESS_DENIED;
@@ -761,7 +761,7 @@ Ext4GetPosition (
 {
   EXT4_FILE  *File;
 
-  File = (EXT4_FILE *)This;
+  File = EXT4_FILE_FROM_THIS (This);
 
   if (Ext4FileIsDir (File)) {
     return EFI_UNSUPPORTED;
@@ -794,7 +794,7 @@ Ext4SetPosition (
 {
   EXT4_FILE  *File;
 
-  File = (EXT4_FILE *)This;
+  File = EXT4_FILE_FROM_THIS (This);
 
   // Only seeks to 0 (so it resets the ReadDir operation) are allowed
   if (Ext4FileIsDir (File) && (Position != 0)) {
@@ -1062,7 +1062,7 @@ Ext4GetInfo (
   EXT4_FILE       *File;
   EXT4_PARTITION  *Partition;
 
-  File      = (EXT4_FILE *)This;
+  File      = EXT4_FILE_FROM_THIS (This);
   Partition = File->Partition;
 
   if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
@@ -1179,7 +1179,7 @@ Ext4SetInfo (
   EXT4_FILE       *File;
   EXT4_PARTITION  *Partition;
 
-  File      = (EXT4_FILE *)This;
+  File      = EXT4_FILE_FROM_THIS (This);
   Partition = File->Partition;
 
   if (Partition->ReadOnly) {
-- 
2.37.1

Re: [edk2-platforms][PATCH v4 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE

[Marvin Häuser]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>
(Please just include this in future revisions, if any)

> On 28. Jul 2022, at 17:26, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> 
> We shouldn't use direct casts, because in the future it could break
> the code, so using BASE_CR would be safe against possible structure
> changes and rearrangements
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h |  2 ++
> Features/Ext4Pkg/Ext4Dxe/File.c    | 16 ++++++++--------
> 2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index c1df9d1149e4..1e63e6282fa5 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -382,6 +382,8 @@ struct _Ext4File {
>   EXT4_DENTRY           *Dentry;
> };
> 
> +#define EXT4_FILE_FROM_THIS(This)  BASE_CR ((This), EXT4_FILE, Protocol)
> +
> #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 ae9230d6422b..b1744ad56c98 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -562,7 +562,7 @@ Ext4Open (
>   EXT4_FILE   *FoundFile;
>   EXT4_FILE   *Source;
> 
> -  Source = (EXT4_FILE *)This;
> +  Source = EXT4_FILE_FROM_THIS (This);
> 
>   //
>   // Reset SymLoops counter
> @@ -599,7 +599,7 @@ Ext4Close (
>   IN EFI_FILE_PROTOCOL  *This
>   )
> {
> -  return Ext4CloseInternal ((EXT4_FILE *)This);
> +  return Ext4CloseInternal (EXT4_FILE_FROM_THIS (This));
> }
> 
> /**
> @@ -680,7 +680,7 @@ Ext4ReadFile (
>   EXT4_PARTITION  *Partition;
>   EFI_STATUS      Status;
> 
> -  File      = (EXT4_FILE *)This;
> +  File      = EXT4_FILE_FROM_THIS (This);
>   Partition = File->Partition;
> 
>   ASSERT (Ext4FileIsOpenable (File));
> @@ -731,7 +731,7 @@ Ext4WriteFile (
> {
>   EXT4_FILE  *File;
> 
> -  File = (EXT4_FILE *)This;
> +  File = EXT4_FILE_FROM_THIS (This);
> 
>   if (!(File->OpenMode & EFI_FILE_MODE_WRITE)) {
>     return EFI_ACCESS_DENIED;
> @@ -761,7 +761,7 @@ Ext4GetPosition (
> {
>   EXT4_FILE  *File;
> 
> -  File = (EXT4_FILE *)This;
> +  File = EXT4_FILE_FROM_THIS (This);
> 
>   if (Ext4FileIsDir (File)) {
>     return EFI_UNSUPPORTED;
> @@ -794,7 +794,7 @@ Ext4SetPosition (
> {
>   EXT4_FILE  *File;
> 
> -  File = (EXT4_FILE *)This;
> +  File = EXT4_FILE_FROM_THIS (This);
> 
>   // Only seeks to 0 (so it resets the ReadDir operation) are allowed
>   if (Ext4FileIsDir (File) && (Position != 0)) {
> @@ -1062,7 +1062,7 @@ Ext4GetInfo (
>   EXT4_FILE       *File;
>   EXT4_PARTITION  *Partition;
> 
> -  File      = (EXT4_FILE *)This;
> +  File      = EXT4_FILE_FROM_THIS (This);
>   Partition = File->Partition;
> 
>   if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
> @@ -1179,7 +1179,7 @@ Ext4SetInfo (
>   EXT4_FILE       *File;
>   EXT4_PARTITION  *Partition;
> 
> -  File      = (EXT4_FILE *)This;
> +  File      = EXT4_FILE_FROM_THIS (This);
>   Partition = File->Partition;
> 
>   if (Partition->ReadOnly) {
> -- 
> 2.37.1
>