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

["Savva Mitrofanov" <savvamtr@gmail.com>]

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
>> 
>