From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) by mx.groups.io with SMTP id smtpd.web10.15271.1658656633082307198 for ; Sun, 24 Jul 2022 02:57:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=oX42jfJI; spf=pass (domain: gmail.com, ip: 209.85.167.51, mailfrom: savvamtr@gmail.com) Received: by mail-lf1-f51.google.com with SMTP id bf9so13909098lfb.13 for ; Sun, 24 Jul 2022 02:57:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=HeZw3SDoZdUTmC9ZDJ+BU+jxGIcJEArXZcmhlB1idoo=; b=oX42jfJIaTzQEFuTL2JS1mRVCPwgin3Xi+Plf+3EjkcXyBEceN8D8WU6VLemT1bkHo WnFHfinrX7MAFqAaNh6yYuSUsaU68y4i9OLandEan9G8TfMl1CkrbVe79PWLOIufzGrV +PGcQLISDM5WNFnNFWZcjGrAuD0AtSMOwWub/EJT0FxYyZ+dp9a9f9S/V1T9BcvvoAX1 PnOuYPTi+UjgbkeKK2w2ykFTimU+d83JPkefcti3+xM7idwN7TeZo5dMP+gaII+ylUgW XagYwrj73CkO39X4EniVYCpEnblA1tfYyAGUobI8j0WO8K6GCZRMV1AOym6N7GcwxkHz UUfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=HeZw3SDoZdUTmC9ZDJ+BU+jxGIcJEArXZcmhlB1idoo=; b=FZhyMkM2OcckSSRSm7SZIpRwSB29WVeZMEbEQ5kZshC6rCk7AF5wA9zFm41om1TYjc z+Bx8xN1kBTbv8iXMhgTpFQwoBEqVhK9iC7UPS+opBAknDANiI6Hlj6dFZ12X0qczvZG oKE0Ajbhfa5YyuKkFZtOGxU55fp72YPAQgqztljRRNn8Qk2WtZFhknMKCpZnou5FYACy yBQUZcfc/52DjpPjR584RnhfxV/lHVVUmtPAg7agZUsKUVazbYLP7SAxqzrHX4lTtkiL cfHEDSpnUApX5Ebz41R9McPMENH4xXR37Csl5iR1bn3YgKXrYSbyPLuyCEE3+/W6U65I /lxg== X-Gm-Message-State: AJIora/DDnnMY+XDqZHGOo6y174I3ChaiJNCeGMHzlwYA6d7iJ4XaLCf ULlaR1JwCHMj4pL8AIyWdCc= X-Google-Smtp-Source: AGRyM1vszc67lD/VSzTwg/bJduTcbUHp/tNTa1a+wKZb7MFSyXrOqUfOpUGWZytqrwtkUOXjGYp2ZQ== X-Received: by 2002:a05:6512:308d:b0:48a:7d81:b41c with SMTP id z13-20020a056512308d00b0048a7d81b41cmr2690966lfd.86.1658656630539; Sun, 24 Jul 2022 02:57:10 -0700 (PDT) Return-Path: Received: from smtpclient.apple ([207.180.219.167]) by smtp.gmail.com with ESMTPSA id d23-20020a196b17000000b00488d1acb7b0sm2105279lfa.130.2022.07.24.02.57.08 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 24 Jul 2022 02:57:09 -0700 (PDT) From: "Savva Mitrofanov" Message-Id: <00A3A212-0C03-4454-9A69-18EEFC7EE03D@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\)) Subject: Re: [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Date: Sun, 24 Jul 2022 15:57:01 +0600 In-Reply-To: Cc: Pedro Falcato , vit9696@protonmail.com, devel@edk2.groups.io To: =?utf-8?Q?Marvin_H=C3=A4user?= References: <20220720061702.40770-1-savvamtr@gmail.com> <20220720061702.40770-2-savvamtr@gmail.com> X-Mailer: Apple Mail (2.3654.120.0.1.13) X-Groupsio-MsgNum: 91767 Content-Type: multipart/signed; boundary="Apple-Mail=_2983DAA6-027B-4A17-BCC9-7362C4164A65"; protocol="application/pgp-signature"; micalg=pgp-sha256 --Apple-Mail=_2983DAA6-027B-4A17-BCC9-7362C4164A65 Content-Type: multipart/alternative; boundary="Apple-Mail=_7512BCF4-F3C6-4BAE-857E-BF34EC9FB891" --Apple-Mail=_7512BCF4-F3C6-4BAE-857E-BF34EC9FB891 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hello, Marvin! Thank you for your time and this code-review > On 21 Jul 2022, at 01:15, Marvin H=C3=A4user = wrote: >=20 >=20 >=20 >> 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; >=20 > I think the code style demands "L" is capitalised. Yes, in this case we should capitalise L for camel-case style. >=20 >>=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. >=20 > I want to mention, that such code-style flaws present throughout entire = project. So I think we can additionally create separate commit with = code-style corrections in "Ext4Pkg: Code correctness and security improvements" patch. >=20 >> + @param[in] File Pointer to the opened file. >> + >> + @return TRUE if file is a symlink. >=20 > I think you can use "@returns Whether file is a symlink", because = strictly speaking you would require a "FALSE" case with this syntax. Same as above, such comments present in other functions. >=20 >> +**/ >> +BOOLEAN >> +Ext4FileIsSymlink ( >> + IN CONST EXT4_FILE *File >> + ); >> + >> +/** >> + Detects if a symlink is a fast symlink. >=20 > >=20 >> + @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); >=20 > 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). This function I peeped as is in Pedro's Onyx. Strictly speaking in such = case, when we allocating maximum buffer size, we should use = AllocateZeroPool then, to be sure that string terminated correct. = Anyway, I agree with you, we should take symlink size from inode like it = does in linux, uboot and other projects which implements Ext4 fs driver. = So we can rewrite this function like this to be based on real inode size = of symlink: STATIC EFI_STATUS Ext4ReadFastSymlink ( IN EXT4_PARTITION *Partition, IN EXT4_FILE *File, OUT CHAR8 **AsciiSymlink, OUT UINT32 *AsciiSymlinkSize ) { UINT32 SymlinkSize; CHAR8 *AsciiSymlinkTmp; SymlinkSize =3D EXT4_INODE_SIZE (File->Inode); ASSERT (SymlinkSize <=3D EXT4_FAST_SYMLINK_MAX_SIZE); AsciiSymlinkTmp =3D AllocatePool (SymlinkSize + 1); if (AsciiSymlinkTmp =3D=3D 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] =3D '\0'; *AsciiSymlink =3D AsciiSymlinkTmp; *AsciiSymlinkSize =3D SymlinkSize + 1; return EFI_SUCCESS; } >> + if (AsciiSymlinkTmp =3D=3D NULL) { >> + Status =3D EFI_OUT_OF_RESOURCES; >=20 > Please return the status code as part of "return" unless necessary to = do it otherwise (this goes for all similar places). >=20 >> + 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; >=20 > 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). >=20 >> + } 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)); >> + // >=20 > 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. >=20 >> + >> + 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 >=20 > Capitalised "L". >=20 Yes, this word is contraction of symbolic link, but as we discussed in = this case we should keep it as is, because it is standard single-word. >> + ) >> +{ >> + 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'\\'; >> + } >=20 > I'm actually wondering whether a regular loop is not both easier and = faster, as this is a 1-character search. I agree with you, in my opinion this code is overloaded with a call to = the substring search function and can be simplified to: for (Needle =3D Symlink16Tmp; *Needle !=3D L'\0'; Needle++) { if (*Needle =3D=3D L'/') { *Needle =3D L'\\'; } } >=20 >> + >> + *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 = >=20 > (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 = >=20 > For the EA check, Cc Pedro for a source. This is not in the kernl = docs. >=20 >> + // 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; >=20 > As per the code style requirements, declarations and definitions must = be separate for local variables. >=20 > i_file_acl has a "_hi" variant in EXT4 you did not account for. Cc = Pedro. >=20 Thanks for good catch! We definitely should take i_file_acl_high into = account. Investigating this function more deeply opens to me = fast-symlink problems history in Linux: 1) = https://github.com/torvalds/linux/commit/407cd7fb83c0ebabb490190e673d8c71e= e7df97e = 2) = https://github.com/torvalds/linux/commit/fc82228a5e3860502dbf3bfa4a9570cb7= 093cf7f = In short, new behaviour with extended attributes logic should be used = only if large extended attribute flag is set in inode, otherwise - use = old behaviour based on inode size check. So as a result I propose to = refactor the code in the following way BOOLEAN Ext4SymlinkIsFastSymlink ( IN CONST EXT4_FILE *File ) { // // Essentially, if EXT4_EA_INODE_FL is set 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 inode->i_data. // If large extended attributes flag isn't set, then we check that = inode size less // than maximum fast symlink size // UINT32 FileAcl; INTN ExtAttrBlocks; if (File->Inode->i_flags & EXT4_EA_INODE_FL) { FileAcl =3D File->Inode->i_file_acl; if (EXT4_IS_64_BIT (File->Partition)) { FileAcl |=3D LShiftU64 = (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32); } ExtAttrBlocks =3D FileAcl !=3D 0 ? (File->Partition->BlockSize >> 9) = : 0; return File->Inode->i_blocks =3D=3D ExtAttrBlocks; } return EXT4_INODE_SIZE (File->Inode) <=3D EXT4_FAST_SYMLINK_MAX_SIZE; } >> + >> + return ( File->Inode->i_blocks =3D=3D ExtAttrBlocks >> + && EXT4_INODE_SIZE (File->Inode) <=3D 60 >=20 > Is this "60" not "EXT4_FAST_SYMLINK_SIZE"? Maybe we should rather call = it "EXT4_FAST_SYMLINK_MAX_SIZE"? >=20 Yes, we should, this will match the purpose of this constant. In my view = it is better to make it based on EXT4_NR_BLOCKS, like #define EXT4_FAST_SYMLINK_MAX_SIZE EXT4_NR_BLOCKS * sizeof(UINT32) and move this constant to Ext4Disk.h from Ext4Dxe.h > Best regards, > Marvin >=20 >> + ); >> +} >> + >> /** >> Checks if a file is a regular file. >> @param[in] File Pointer to the opened file. >> -- >> 2.37.0 >>=20 >=20 All in all, I have prepared patch v3 with the corrections and will send = it soon. Best regards, Savva Mitrofanov --Apple-Mail=_7512BCF4-F3C6-4BAE-857E-BF34EC9FB891 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Hello, Marvin!
Thank you for your time and this code-review
On = 21 Jul 2022, at 01:15, Marvin H=C3=A4user <mhaeuser@posteo.de> = wrote:



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.

Yes, in this case we should capitalise L for camel-case = style.



  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>

I want to = mention, that such code-style flaws present throughout entire project. = So I think we can additionally create separate commit with code-style = corrections in 
"Ext4Pkg: Code correctness and security = improvements" patch.


+   @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.

Same= as above, such comments present in other functions.


+**/
+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).


This function I = peeped as is in Pedro's Onyx. Strictly speaking in such case, when we = allocating maximum buffer size, we should use AllocateZeroPool then, to = be sure that string terminated correct. Anyway, I agree with you, we = should take symlink size from inode like it does in linux, uboot and = other projects which implements Ext4 fs driver. So we can rewrite this = function like this to be based on real inode size of = symlink:

STATIC
EFI_STATUS
Ext4ReadF= astSymlink (
  IN     EXT4_PARTITION =  *Partition,
  IN     EXT4_FILE   =     *File,
  OUT    CHAR8   =         **AsciiSymlink,
  OUT   =  UINT32         =  *AsciiSymlinkSize
  )
{
  = UINT32  SymlinkSize;
  CHAR8   = *AsciiSymlinkTmp;

  SymlinkSize = =3D EXT4_INODE_SIZE (File->Inode);

  ASSERT (SymlinkSize <=3D = EXT4_FAST_SYMLINK_MAX_SIZE);

  = AsciiSymlinkTmp =3D AllocatePool (SymlinkSize + 1);
  if = (AsciiSymlinkTmp =3D=3D 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] =3D '\0';

  *AsciiSymlink     =3D = AsciiSymlinkTmp;
  *AsciiSymlinkSize =3D SymlinkSize + = 1;

  return = EFI_SUCCESS;
}

+  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".

Yes, this word is = contraction of symbolic link, but as we discussed in this case we should = keep it as is, because it is standard single-word.

+  )
+{
+  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.

I agree with you, in my opinion this code is overloaded = with a call to the substring search function and can be simplified = to:

  for (Needle =3D = Symlink16Tmp; *Needle !=3D L'\0'; Needle++) {
    if = (*Needle =3D=3D L'/') {
      *Needle =3D = L'\\';
    }
  }


+
+=  *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.

Thanks for good catch! = We definitely should take i_file_acl_high into account. Investigating = this function more deeply opens to me fast-symlink problems history in = Linux:
In short, new behaviour with = extended attributes logic should be used only if large extended = attribute flag is set in inode, otherwise - use old behaviour based on = inode size check. So as a result I propose to refactor the code in the = following way

BOOLEAN
Ext4SymlinkIsFastSymlink = (
  IN CONST EXT4_FILE  *File
  = )
{
  //
  // Essentially, if = EXT4_EA_INODE_FL is set 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 inode->i_data.
  = // If large extended attributes flag isn't set, then we check that inode = size less
  // than maximum fast symlink = size
  //
  UINT32 =  FileAcl;
  INTN   =  ExtAttrBlocks;

  if = (File->Inode->i_flags & EXT4_EA_INODE_FL) {
  =   FileAcl =3D File->Inode->i_file_acl;
  =   if (EXT4_IS_64_BIT (File->Partition)) {
  =     FileAcl |=3D LShiftU64 = (File->Inode->i_osd2.data_linux.l_i_file_acl_high, = 32);
    }

  =   ExtAttrBlocks =3D FileAcl !=3D 0 ? = (File->Partition->BlockSize >> 9) : 0;

    return File->Inode->i_blocks = =3D=3D ExtAttrBlocks;
  }

  return EXT4_INODE_SIZE (File->Inode) = <=3D EXT4_FAST_SYMLINK_MAX_SIZE;
}

+
+ =  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"?

Yes, we should, = this will match the purpose of this constant. In my view it is better to = make it based on EXT4_NR_BLOCKS, like
#define = EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * = sizeof(UINT32)
and move this constant to Ext4Disk.h from = Ext4Dxe.h


Best regards,
Marvin

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



All in all, I have prepared patch v3 with the corrections and = will send it soon.

Best regards,
Savva Mitrofanov

= --Apple-Mail=_7512BCF4-F3C6-4BAE-857E-BF34EC9FB891-- --Apple-Mail=_2983DAA6-027B-4A17-BCC9-7362C4164A65 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEQCRE8H+GNIHmmeufd0kkAxdQv2QFAmLdF20ACgkQd0kkAxdQ v2TW6BAApOo/tYVoICZb+E7edUqZ/lKCiQV4BZeA2TKjSuHHgvoA9h3dn3JuEfgR YRRZrGqmFmyQXkt5sg1yyV1PVfvoAPDgF3aNVacRV0wNkp5Xry1zCwVGHY1/hAlQ u7EJdFty4zL0xEVkBGpxQ1OHSE4Z7li9FJUqINsRTXWnwP0RscHN6O3m5RxPzlbS +OpbCpOwdIhk5D4pzwXy6pbcgRvHEB8MNBkWqDbs3C3P5Hooyy4RXvpx8Av+itja eM6sPZRz1B8WX1Tf1xuD6hFXH/HQPJLZMbOaLQOVNQlss607kNmk6DMKlgZM6ff3 vzEER9kfrmvd0iFgPFNcPc7eWJIGIHQI1qJb85R/JRbN1+MwVZZH8901MW/DgKhZ jsvgKxKbLbqwdy5IlFd7PuVL0PJInY46+biCIvrV4YeLNUa8XJpJgH+ejhsLm652 Q8UFwNl8lIw3iV8EWSWq1DqjK8dzWcF6gNSH7YgbnfK2Y4GsKFJ56RL1BiJ6DUDB I9+YsaD+PreHi0Zf96u1qzGtjI8yOiHjYyE66v5TKmpsYoW0ug6BqzRYRduGb/B9 yw9QjSoSbWbx52iEtQ8ieqN6bRcEsv9qk04im+5CVyCVWaY7OpuGkhJ772rOadKV UJqie/snl+Tr9dKvAt232FHaPQQmD0s4oArQfEtceTXEwDFckCg= =RJN0 -----END PGP SIGNATURE----- --Apple-Mail=_2983DAA6-027B-4A17-BCC9-7362C4164A65--