From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web09.1357.1658344517333860731 for ; Wed, 20 Jul 2022 12:15:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=U36NaGaO; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 7809F240027 for ; Wed, 20 Jul 2022 21:15:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1658344515; bh=ui9CWu61RjuBVzgiCT/U8lJwCGp7h6qxi564HCXMzOM=; h=From:Subject:Date:Cc:To:From; b=U36NaGaO4f4kHyFoZbRvD1ZmRySRvqePqN8sBtZygOloI8rI05tnEY6qnGhfTc4YB qpIzatqoIthbjwL9HdoPosAUUx4LYBv0vCl+z1YIbAIv4CzE3X8QeasmLombTxlsUa VA8xV0l6x5LTeOa25M9TsJpkfhBHEr0Nfc8dTCOkKrodKONIKi+sr8tb0OoH0nRwS0 6DI3QVS+r2FEItgEqSLRv/Mw2Uu8qNtsQsB8xx/45EPz08JtSOvA4ye3cXhg2M8PpY 1VHeQc+98h8BTPYoAOyddVdLlMijHbl+Fo1PnMRziXb0p5M3XuYnUksbAKHnAgvRbp uW8Nc5YN49j1Q== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Lp54d68xdz9rxF; Wed, 20 Jul 2022 21:15:13 +0200 (CEST) From: =?utf-8?Q?Marvin_H=C3=A4user?= Message-Id: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Subject: Re: [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Date: Wed, 20 Jul 2022 19:15:13 +0000 In-Reply-To: <20220720061702.40770-2-savvamtr@gmail.com> Cc: edk2-devel-groups-io , Pedro Falcato , Vitaly Cheptsov To: Savva Mitrofanov References: <20220720061702.40770-1-savvamtr@gmail.com> <20220720061702.40770-2-savvamtr@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_7B29D92F-2D56-428E-8580-C527D66C1A71" --Apple-Mail=_7B29D92F-2D56-428E-8580-C527D66C1A71 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 20. Jul 2022, at 08:17, Savva Mitrofanov = wrote: >=20 > 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. >=20 > Cc: Marvin H=C3=A4user > Cc: Pedro Falcato > Cc: Vitaly Cheptsov > Signed-off-by: Savva Mitrofanov > --- > Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 2 +- > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 99 +++++- > Features/Ext4Pkg/Ext4Dxe/File.c | 365 ++++++++++++++++++-- > Features/Ext4Pkg/Ext4Dxe/Inode.c | 38 ++ > 4 files changed, 470 insertions(+), 34 deletions(-) >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h > index a55cd2fa68ad..6f83dcf65429 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 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > index b1508482b0a7..a1eb32aa2cff 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > @@ -31,7 +31,10 @@ >=20 > #include "Ext4Disk.h" >=20 > -#define EXT4_NAME_MAX 255 > +#define SYMLOOP_MAX 8 > +#define EXT4_NAME_MAX 255 > +#define EFI_PATH_MAX 4096 > +#define EXT4_FAST_SYMLINK_SIZE 60 >=20 > #define EXT4_DRIVER_VERSION 0x0000 >=20 > @@ -324,11 +327,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 > ); >=20 > /** > @@ -368,6 +371,7 @@ struct _Ext4File { >=20 > UINT64 OpenMode; > UINT64 Position; > + UINT32 Symloops; I think the code style demands "L" is capitalised. >=20 > EXT4_PARTITION *Partition; >=20 > @@ -497,6 +501,45 @@ Ext4SetupFile ( > IN EXT4_PARTITION *Partition > ); >=20 > +/** > + 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. >=20 > @@ -774,6 +817,28 @@ Ext4FileIsDir ( > IN CONST EXT4_FILE *File > ); >=20 > +/** > + Checks if a file is a symlink. > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file is a symlink. I think you can use "@returns Whether file is a symlink", because = strictly speaking you would require a "FALSE" case with this syntax. > +**/ > +BOOLEAN > +Ext4FileIsSymlink ( > + IN CONST EXT4_FILE *File > + ); > + > +/** > + Detects if a symlink is a fast symlink. > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file 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 +862,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)) >=20 > #define EXT4_INODE_HAS_FIELD(Inode, Field) = \ > (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=3D = \ > @@ -935,6 +1000,26 @@ Ext4ReadDir ( > IN OUT UINTN *OutLength > ); >=20 > +/** > + 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. >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c = b/Features/Ext4Pkg/Ext4Dxe/File.c > index ff1746d5640a..0fb9e05e6647 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/File.c > +++ b/Features/Ext4Pkg/Ext4Dxe/File.c > @@ -134,14 +134,230 @@ Ext4DirCanLookup ( > return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) =3D=3D = EXT4_INO_PERM_EXEC_OWNER; > } >=20 > +/** > + 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 > + ) > +{ > + EFI_STATUS Status; > + CHAR8 *AsciiSymlinkTmp; > + > + AsciiSymlinkTmp =3D AllocatePool (EXT4_FAST_SYMLINK_SIZE + 1); Hmm, why do we use constant sizes instead of the inode size? The size = constant seems to imply fast links have a fixed size of 60 Bytes, but = this is not true according to the spec, which says "less than 60 Bytes" = (i.e., one more for the 0-terminator). > + if (AsciiSymlinkTmp =3D=3D NULL) { > + Status =3D EFI_OUT_OF_RESOURCES; Please return the status code as part of "return" unless necessary to do = it otherwise (this goes for all similar places). > + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string = buffer\n")); > + return Status; > + } > + > + CopyMem (AsciiSymlinkTmp, File->Inode->i_data, = EXT4_FAST_SYMLINK_SIZE); > + > + // > + // Add null-terminator > + // > + AsciiSymlinkTmp[EXT4_FAST_SYMLINK_SIZE] =3D '\0'; > + > + *AsciiSymlink =3D AsciiSymlinkTmp; > + *AsciiSymlinkSize =3D EXT4_FAST_SYMLINK_SIZE + 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 =3D EXT4_INODE_SIZE (File->Inode); > + > + // > + // Allocate EXT4_INODE_SIZE + 1 > + // > + if (SymlinkSizeTmp <=3D EFI_PATH_MAX - 1) { > + SymlinkAllocateSize =3D (UINT32)SymlinkSizeTmp + 1; In my opinion, check error conditions and return and only then perform = the success operations. This nesting looks to close to normal branching, = where there could be two alternative paths. This pattern may also cause = issues with basic static analyzers that cannot really keep track of = branching (this goes for all similar places). > + } else { > + DEBUG (( > + DEBUG_FS, > + "[ext4] Error! Symlink path maximum length was hit!\n" > + )); > + return EFI_INVALID_PARAMETER; > + } > + > + SymlinkTmp =3D AllocatePool (SymlinkAllocateSize); > + if (SymlinkTmp =3D=3D NULL) { > + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string = buffer\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + ReadSize =3D (UINTN)SymlinkSizeTmp; > + Status =3D Ext4Read (Partition, File, SymlinkTmp, File->Position, = &ReadSize); > + if (!EFI_ERROR (Status)) { > + File->Position +=3D ReadSize; > + } else { > + DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with = Status %r\n", Status)); > + FreePool (SymlinkTmp); > + return Status; > + } > + > + // > + // Add null-terminator > + // > + SymlinkTmp[SymlinkSizeTmp] =3D '\0'; > + > + // > + // It is not clear, should we check that symlink allocation size is > + // equal symlink string size or not. However there is no checks in = existing > + // Ext4 implementations, so we also don't check it here relying on = the fact > + // we terminated string ourselves above. > + // > + // ASSERT (SymlinkAllocateSize =3D=3D AsciiStrSize (SymlinkTmp)); > + // I would not keep commented out code. This check is especially = problematic, as unterminated paths are not disallowed and this check = will behave differently between terminated and unterminated paths. > + > + if (SymlinkSizeTmp !=3D 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 =3D SymlinkAllocateSize; > + *AsciiSymlink =3D 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 Capitalised "L". > + ) > +{ > + 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) !=3D 0) { > + Status =3D EFI_ACCESS_DENIED; > + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n")); > + return Status; > + } > + > + if (Ext4SymlinkIsFastSymlink (File)) { > + Status =3D Ext4ReadFastSymlink (Partition, File, &SymlinkTmp, = &SymlinkSize); > + } else { > + Status =3D Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp, = &SymlinkSize); > + } > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n", = Status)); > + return Status; > + } > + > + Symlink16Tmp =3D AllocateZeroPool (SymlinkSize * sizeof (CHAR16)); > + if (Symlink16Tmp =3D=3D NULL) { > + Status =3D EFI_OUT_OF_RESOURCES; > + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode = string buffer\n")); > + FreePool (SymlinkTmp); > + return Status; > + } > + > + Status =3D 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 > + // > + Needle =3D Symlink16Tmp; > + while ((Needle =3D StrStr (Needle, L"/")) !=3D NULL) { > + *Needle++ =3D L'\\'; > + } I'm actually wondering whether a regular loop is not both easier and = faster, as this is a 1-character search. > + > + *Symlink =3D Symlink16Tmp; > + > + FreePool (SymlinkTmp); > + return Status; > +} > + > /** > Opens a new file relative to the source file's location. >=20 > - @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 +381,12 @@ Ext4DirCanLookup ( >=20 > **/ > 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 +395,14 @@ Ext4Open ( > CHAR16 PathSegment[EXT4_NAME_MAX + 1]; > UINTN Length; > EXT4_FILE *File; > + CHAR16 *Symlink; > EFI_STATUS Status; >=20 > - Current =3D (EXT4_FILE *)This; > + Current =3D Source; > Partition =3D Current->Partition; > Level =3D 0; >=20 > - 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] =3D=3D L'\\') { > FileName++; > @@ -194,6 +410,11 @@ Ext4Open ( > } >=20 > while (FileName[0] !=3D 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] =3D=3D L'\\') { > FileName++; > @@ -238,18 +459,45 @@ Ext4Open ( > } >=20 > // 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; > } >=20 > + // > + // 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 =3D 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 =3D 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 =3D *FoundFile; > + } > + > if (Level !=3D 0) { > // Careful not to close the base directory > Ext4CloseInternal (Current); > @@ -273,12 +521,75 @@ Ext4Open ( > return EFI_ACCESS_DENIED; > } >=20 > - *NewHandle =3D &Current->Protocol; > + *FoundFile =3D Current; >=20 > DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", = Current->Dentry->Name)); > return EFI_SUCCESS; > } >=20 > +/** > + 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 =3D (EXT4_FILE *)This; > + > + // > + // Reset Symloops counter > + // > + Source->Partition->Root->Symloops =3D 0; > + > + Status =3D Ext4OpenInternal ( > + &FoundFile, > + Source, > + FileName, > + OpenMode, > + Attributes > + ); > + > + if (!EFI_ERROR (Status)) { > + *NewHandle =3D &FoundFile->Protocol; > + } > + > + return Status; > +} > + > /** > Closes a specified file handle. >=20 > @@ -588,7 +899,7 @@ Ext4GetVolumeName ( >=20 > // s_volume_name is only valid on dynamic revision; old filesystems = don't support this > if (Partition->SuperBlock.s_rev_level =3D=3D EXT4_DYNAMIC_REV) { > - CopyMem (TempVolName, (CONST CHAR8 = *)Partition->SuperBlock.s_volume_name, 16); > + CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16); > TempVolName[16] =3D '\0'; >=20 > Status =3D UTF8StrToUCS2 (TempVolName, &VolumeName); > @@ -754,12 +1065,14 @@ Ext4GetInfo ( > OUT VOID *Buffer > ) > { > + EXT4_FILE *File; > EXT4_PARTITION *Partition; >=20 > - Partition =3D ((EXT4_FILE *)This)->Partition; > + File =3D (EXT4_FILE *)This; > + Partition =3D File->Partition; >=20 > if (CompareGuid (InformationType, &gEfiFileInfoGuid)) { > - return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize); > + return Ext4GetFileInfo (File, Buffer, BufferSize); > } >=20 > if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) { > @@ -870,12 +1183,12 @@ Ext4SetInfo ( > ) > { > EXT4_FILE *File; > - EXT4_PARTITION *Part; > + EXT4_PARTITION *Partition; >=20 > - File =3D (EXT4_FILE *)This; > - Part =3D File->Partition; > + File =3D (EXT4_FILE *)This; > + Partition =3D File->Partition; >=20 > - if (Part->ReadOnly) { > + if (Partition->ReadOnly) { > return EFI_WRITE_PROTECTED; > } >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c = b/Features/Ext4Pkg/Ext4Dxe/Inode.c > index 831f5946e870..e95299c3aee0 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c > @@ -255,6 +255,44 @@ Ext4FileIsDir ( > return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) =3D=3D = EXT4_INO_TYPE_DIR; > } >=20 > +/** > + Checks if a file is a symlink. > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file is a symlink. > +**/ > +BOOLEAN > +Ext4FileIsSymlink ( > + IN CONST EXT4_FILE *File > + ) > +{ > + return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) =3D=3D = EXT4_INO_TYPE_SYMLINK; > +} > + > +/** > + Detects if a symlink is a fast symlink. > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file is a fast symlink. > +**/ > +BOOLEAN > +Ext4SymlinkIsFastSymlink ( > + IN CONST EXT4_FILE *File > + ) > +{ > + // > + // REF: = https://github.com/heatd/Onyx/blob/master/kernel/kernel/fs/ext2/symlink.cp= p#L26 (No offense, Pedro :), but) This is not an authority. Please cite the = kernel docs: = https://www.kernel.org/doc/html/latest/filesystems/ext4/dynamic.html#symbo= lic-links = For the EA check, Cc Pedro for a source. This is not in the kernl docs. > + // Essentially, we're 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 = ext2_inode->i_data. > + // > + INTN ExtAttrBlocks =3D File->Inode->i_file_acl ? = (File->Partition->BlockSize >> 9) : 0; As per the code style requirements, declarations and definitions must be = separate for local variables. i_file_acl has a "_hi" variant in EXT4 you did not account for. Cc = Pedro. > + > + return ( File->Inode->i_blocks =3D=3D ExtAttrBlocks > + && EXT4_INODE_SIZE (File->Inode) <=3D 60 Is this "60" not "EXT4_FAST_SYMLINK_SIZE"? Maybe we should rather call = it "EXT4_FAST_SYMLINK_MAX_SIZE"? Best regards, Marvin > + ); > +} > + > /** > Checks if a file is a regular file. > @param[in] File Pointer to the opened file. > --=20 > 2.37.0 >=20 --Apple-Mail=_7B29D92F-2D56-428E-8580-C527D66C1A71 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 20. Jul 2022, at 08:17, Savva Mitrofanov <savvamtr@gmail.com> = wrote:

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=C3=A4user <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 |   2 +-
= Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  99 +++++-
= Features/Ext4Pkg/Ext4Dxe/File.c     | 365 = ++++++++++++++++++--
Features/Ext4Pkg/Ext4Dxe/Inode.c =    |  38 ++
4 files changed, 470 = insertions(+), 34 deletions(-)

diff --git = a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index = a55cd2fa68ad..6f83dcf65429 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
diff --git = a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index = b1508482b0a7..a1eb32aa2cff 100644
--- = a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -31,7 +31,10 @@

#include "Ext4Disk.h"

-#define EXT4_NAME_MAX  255
+#define = SYMLOOP_MAX =             8<= br class=3D"">+#define EXT4_NAME_MAX =           255
+#define EFI_PATH_MAX =            4096
+#define EXT4_FAST_SYMLINK_SIZE  60

#define EXT4_DRIVER_VERSION  0x0000

@@ -324,11 +327,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 +371,7 @@ struct _Ext4File {

  UINT64 =             &n= bsp;  OpenMode;
  UINT64 =             &n= bsp;  Position;
+  UINT32 =             &n= bsp;  Symloops;

I think the code style demands "L" is = capitalised.


  EXT4_PARTITION =        *Partition;

@@ -497,6 +501,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
+ =             &n= bsp;           file= .
+  @param[in]  Source =     A pointer to the EXT4_FILE instance that is the = file
+ =             &n= bsp;           hand= le to the source location. This would typically be an open
+= =             &n= bsp;           hand= le to a directory.
+  @param[in]  FileName =   The Null-terminated string of the name of the file to be = opened.
+ =             &n= bsp;           The = file name may contain the following path modifiers: "\", ".",
+ =             &n= bsp;           and = "..".
+  @param[in]  OpenMode   The = mode to open the file. The only valid combinations that the
+ =             &n= bsp;           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
+ =             &n= bsp;           attr= ibute 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
+ =             &n= bsp;           &nbs= p;     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
+ =             &n= bsp;           &nbs= p;     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 = +817,28 @@ Ext4FileIsDir (
  IN CONST EXT4_FILE =  *File
  );

+/**
+   Checks if a file is a = symlink.

<line break>

+ =   @param[in]      File =          Pointer to the = opened file.
+
+   @return TRUE if = file is a symlink.

I think you can use "@returns Whether file is a = symlink", because strictly speaking you would require a "FALSE" case = with this syntax.

+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE =  *File
+  );
+
+/**+   Detects if a symlink is a fast symlink.

<line = break>

+   @param[in] =      File =          Pointer to the = opened file.
+
+   @return TRUE if = file 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 +862,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) =             &n= bsp;           &nbs= p;           \
  (Inode->i_extra_isize + = EXT4_GOOD_OLD_INODE_SIZE >=3D =             &n= bsp;           &nbs= p;\
@@ -935,6 +1000,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..0fb9e05e6647 100644
--- = a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -134,14 +134,230 @@ = Ext4DirCanLookup (
  return = (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) =3D=3D = 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+  )
+{
+  EFI_STATUS =  Status;
+  CHAR8 =       *AsciiSymlinkTmp;
+
+  AsciiSymlinkTmp =3D AllocatePool = (EXT4_FAST_SYMLINK_SIZE + 1);

Hmm, = why do we use constant sizes instead of the inode size? The size = constant seems to imply fast links have a fixed size of 60 Bytes, but = this is not true according to the spec, which says "less than 60 Bytes" = (i.e., one more for the 0-terminator).

+  if = (AsciiSymlinkTmp =3D=3D NULL) {
+    Status = =3D EFI_OUT_OF_RESOURCES;

Please return the status code as part of "return" = unless necessary to do it otherwise (this goes for all similar = places).

+    DEBUG ((DEBUG_FS, "[ext4] = Failed to allocate symlink ascii string buffer\n"));
+ =    return Status;
+  }
+
+  CopyMem (AsciiSymlinkTmp, = File->Inode->i_data, EXT4_FAST_SYMLINK_SIZE);
+
+  //
+  // Add null-terminator
+  //
+ =  AsciiSymlinkTmp[EXT4_FAST_SYMLINK_SIZE] =3D '\0';
++  *AsciiSymlink     =3D = AsciiSymlinkTmp;
+  *AsciiSymlinkSize =3D = EXT4_FAST_SYMLINK_SIZE + 1;
+
+  return = EFI_SUCCESS;
+}
+
+/**
+  Reads a slow symlink file.
+
+  @param[in]      Partition =        Pointer to the ext4 = partition.
+  @param[in] =      File =             Po= inter 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 =3D EXT4_INODE_SIZE = (File->Inode);
+
+  //
+=  // Allocate EXT4_INODE_SIZE + 1
+  //
+  if (SymlinkSizeTmp <=3D EFI_PATH_MAX - 1) {
+    SymlinkAllocateSize =3D = (UINT32)SymlinkSizeTmp + 1;

In my = opinion, check error conditions and return and only then perform the = success operations. This nesting looks to close to normal branching, = where there could be two alternative paths. This pattern may also cause = issues with basic static analyzers that cannot really keep track of = branching (this goes for all similar places).

+  } else {
+    DEBUG = ((
+      DEBUG_FS,
+=      "[ext4] Error! Symlink path maximum = length was hit!\n"
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  SymlinkTmp =3D = AllocatePool (SymlinkAllocateSize);
+  if (SymlinkTmp = =3D=3D NULL) {
+    DEBUG ((DEBUG_FS, = "[ext4] Failed to allocate symlink ascii string buffer\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  ReadSize =3D = (UINTN)SymlinkSizeTmp;
+  Status   =3D = Ext4Read (Partition, File, SymlinkTmp, File->Position, = &ReadSize);
+  if (!EFI_ERROR (Status)) {
+    File->Position +=3D ReadSize;
+  } else {
+    DEBUG = ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with Status = %r\n", Status));
+    FreePool = (SymlinkTmp);
+    return Status;
+  }
+
+  //
+  // Add null-terminator
+  //
+  SymlinkTmp[SymlinkSizeTmp] =3D '\0';
++  //
+  // It is not clear, should = we check that symlink allocation size is
+  // equal = symlink string size or not. However there is no checks in existing
+  // Ext4 implementations, so we also don't check it = here relying on the fact
+  // we terminated string = ourselves above.
+  //
+  // = ASSERT (SymlinkAllocateSize =3D=3D AsciiStrSize (SymlinkTmp));
+  //

I would not keep commented out code. This check is = especially problematic, as unterminated paths are not disallowed and = this check will behave differently between terminated and unterminated = paths.

+
+  if (SymlinkSizeTmp !=3D= 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 =3D = SymlinkAllocateSize;
+  *AsciiSymlink =     =3D 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

Capitalised "L".

+  )
+{
+  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) !=3D 0) {
+    Status =3D EFI_ACCESS_DENIED;
+    DEBUG ((DEBUG_FS, "[ext4] Error, symlink = is encrypted\n"));
+    return Status;
+  }
+
+  if = (Ext4SymlinkIsFastSymlink (File)) {
+ =    Status =3D Ext4ReadFastSymlink (Partition, File, = &SymlinkTmp, &SymlinkSize);
+  } else {
+    Status =3D Ext4ReadSlowSymlink = (Partition, File, &SymlinkTmp, &SymlinkSize);
+ =  }
+
+  if (EFI_ERROR (Status)) = {
+    DEBUG ((DEBUG_FS, "[ext4] Symlink = read error with Status %r\n", Status));
+ =    return Status;
+  }
+
+  Symlink16Tmp =3D AllocateZeroPool = (SymlinkSize * sizeof (CHAR16));
+  if (Symlink16Tmp = =3D=3D NULL) {
+    Status =3D = EFI_OUT_OF_RESOURCES;
+    DEBUG = ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string = buffer\n"));
+    FreePool (SymlinkTmp);
+    return Status;
+  }
+
+  Status =3D AsciiStrToUnicodeStrS (
+ =             Sy= mlinkTmp,
+ =             Sy= mlink16Tmp,
+ =             Sy= mlinkSize
+ =             );=
+
+  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
+  //
+  Needle =3D = Symlink16Tmp;
+  while ((Needle =3D StrStr (Needle, = L"/")) !=3D NULL) {
+    *Needle++ =3D = L'\\';
+  }

I'm = actually wondering whether a regular loop is not both easier and faster, = as this is a 1-character search.

+
+=  *Symlink =3D 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
+ =             &n= bsp;           file= .
+  @param[in]  Source =     A pointer to the EXT4_FILE instance that is the = file
=             &n= bsp;           &nbs= p;handle to the source location. This would typically be an open
=             &n= bsp;           &nbs= p;handle to a directory.
-  @param[out] NewHandle =  A pointer to the location to return the opened handle for the = new
- =             &n= bsp;           file= .
  @param[in]  FileName   The = Null-terminated string of the name of the file to be opened.
=             &n= bsp;           &nbs= p;The file name may contain the following path modifiers: "\", ".",
=             &n= bsp;           &nbs= p;and "..".
@@ -165,13 +381,12 @@ Ext4DirCanLookup (

**/
EFI_STATUS
-EFIAPI
-Ext4Open (
-  IN = EFI_FILE_PROTOCOL   *This,
-  OUT = EFI_FILE_PROTOCOL  **NewHandle,
-  IN CHAR16 =             &n= bsp;*FileName,
-  IN UINT64 =             &n= bsp;OpenMode,
-  IN UINT64 =             &n= bsp;Attributes
+Ext4OpenInternal (
+ =  OUT EXT4_FILE  **FoundFile,
+  IN =  EXT4_FILE  *Source,
+  IN  CHAR16 =     *FileName,
+  IN  UINT64 =     OpenMode,
+  IN  UINT64 =     Attributes
  )
{
  EXT4_FILE =       *Current;
@@ -180,13 = +395,14 @@ Ext4Open (
  CHAR16 =          PathSegment[EXT4_NAM= E_MAX + 1];
  UINTN =           Length;
  EXT4_FILE =       *File;
+  CHAR16 =          *Symlink;
  EFI_STATUS =      Status;

- =  Current   =3D (EXT4_FILE *)This;
+ =  Current   =3D Source;
=   Partition =3D Current->Partition;
=   Level     =3D 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] =3D=3D L'\\') {
    FileName++;
@@ -194,6 = +410,11 @@ Ext4Open (
  }

  while (FileName[0] !=3D 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] =3D=3D L'\\') {
=       FileName++;
@@ -238,18 = +459,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 =3D 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 =3D 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 =3D *FoundFile;
+ =    }
+
=     if (Level !=3D 0) {
=       // Careful not to close the base = directory
=       Ext4CloseInternal (Current);
@@ -273,12 +521,75 @@ Ext4Open (
=     return EFI_ACCESS_DENIED;
=   }

-  *NewHandle =3D = &Current->Protocol;
+  *FoundFile =3D = 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
+ =             &n= bsp;           hand= le to the source location. This would typically be an open
+= =             &n= bsp;           hand= le to a directory.
+  @param[out] NewHandle  A = pointer to the location to return the opened handle for the new
+ =             &n= bsp;           file= .
+  @param[in]  FileName   The = Null-terminated string of the name of the file to be opened.
+ =             &n= bsp;           The = file name may contain the following path modifiers: "\", ".",
+ =             &n= bsp;           and = "..".
+  @param[in]  OpenMode   The = mode to open the file. The only valid combinations that the
+ =             &n= bsp;           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
+ =             &n= bsp;           attr= ibute 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
+ =             &n= bsp;           &nbs= p;     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
+ =             &n= bsp;           &nbs= p;     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 =             &n= bsp;*FileName,
+  IN UINT64 =             &n= bsp;OpenMode,
+  IN UINT64 =             &n= bsp;Attributes
+  )
+{
+ =  EFI_STATUS  Status;
+  EXT4_FILE =   *FoundFile;
+  EXT4_FILE =   *Source;
+
+  Source =3D = (EXT4_FILE *)This;
+
+  //
+  // Reset Symloops counter
+  //
+  Source->Partition->Root->Symloops =3D 0;
+
+  Status =3D Ext4OpenInternal (
+ =             &a= mp;FoundFile,
+ =             So= urce,
+ =             Fi= leName,
+ =             Op= enMode,
+ =             At= tributes
+ =             );=
+
+  if (!EFI_ERROR (Status)) {
+    *NewHandle =3D = &FoundFile->Protocol;
+  }
+
+  return Status;
+}
+
/**
  Closes a specified file = handle.

@@ -588,7 +899,7 @@ = Ext4GetVolumeName (

  // = s_volume_name is only valid on dynamic revision; old filesystems don't = support this
  if = (Partition->SuperBlock.s_rev_level =3D=3D EXT4_DYNAMIC_REV) {
-    CopyMem (TempVolName, (CONST CHAR8 = *)Partition->SuperBlock.s_volume_name, 16);
+ =    CopyMem (TempVolName, = Partition->SuperBlock.s_volume_name, 16);
=     TempVolName[16] =3D '\0';

    Status =3D UTF8StrToUCS2 = (TempVolName, &VolumeName);
@@ -754,12 +1065,14 @@ = Ext4GetInfo (
  OUT VOID =             &n= bsp;*Buffer
  )
{
+ =  EXT4_FILE       *File;
=   EXT4_PARTITION  *Partition;

-  Partition =3D ((EXT4_FILE *)This)->Partition;
+  File      =3D (EXT4_FILE = *)This;
+  Partition =3D File->Partition;

  if (CompareGuid (InformationType, = &gEfiFileInfoGuid)) {
-    return = Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
+ =    return Ext4GetFileInfo (File, Buffer, BufferSize);
  }

  if = (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
@@ -870,12 +1183,12 @@ Ext4SetInfo (
=   )
{
  EXT4_FILE =       *File;
- =  EXT4_PARTITION  *Part;
+  EXT4_PARTITION =  *Partition;

-  File =3D = (EXT4_FILE *)This;
-  Part =3D File->Partition;
+  File      =3D (EXT4_FILE = *)This;
+  Partition =3D 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..e95299c3aee0 100644
--- = a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -255,6 +255,44 @@ = Ext4FileIsDir (
  return = (File->Inode->i_mode & EXT4_INO_TYPE_DIR) =3D=3D = EXT4_INO_TYPE_DIR;
}

+/**
+   Checks if a file is a symlink.
+ =   @param[in]      File =          Pointer to the = opened file.
+
+   @return TRUE if = file is a symlink.
+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE =  *File
+  )
+{
+ =  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) =3D=3D= EXT4_INO_TYPE_SYMLINK;
+}
+
+/**
+   Detects if a symlink is a = fast symlink.
+   @param[in] =      File =          Pointer to the = opened file.
+
+   @return TRUE if = file is a fast symlink.
+**/
+BOOLEAN
+Ext4SymlinkIsFastSymlink (
+  IN CONST = EXT4_FILE  *File
+  )
+{
+  //
+  // REF: https://github.com/heatd/Onyx/blob/master/kernel/kernel/fs/ext2= /symlink.cpp#L26

(No offense, Pedro :), but) This is not an = authority. Please cite the kernel docs: https://www.kernel.org/doc/html/latest/filesystems/ext4/dynamic= .html#symbolic-links

For the EA = check, Cc Pedro for a source. This is not in the kernl docs.

+  // Essentially, we're 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 = ext2_inode->i_data.
+  //
+ =  INTN  ExtAttrBlocks =3D File->Inode->i_file_acl ? = (File->Partition->BlockSize >> 9) : 0;

As = per the code style requirements, declarations and definitions must be = separate for local variables.

i_file_acl has a "_hi" variant in EXT4 you did not = account for. Cc Pedro.

+
+ =  return (  File->Inode->i_blocks =3D=3D ExtAttrBlocks
+         && = EXT4_INODE_SIZE (File->Inode) <=3D 60

Is = this "60" not "EXT4_FAST_SYMLINK_SIZE"? Maybe we should rather call it = "EXT4_FAST_SYMLINK_MAX_SIZE"?

Best = regards,
Marvin

+ =            );
+}
+
/**
=    Checks if a file is a regular file.
=    @param[in]      File =          Pointer to the = opened file.
--
2.37.0


= --Apple-Mail=_7B29D92F-2D56-428E-8580-C527D66C1A71--