From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by mx.groups.io with SMTP id smtpd.web08.7941.1662558844665028662 for ; Wed, 07 Sep 2022 06:54:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=gTJdV70q; spf=pass (domain: gmail.com, ip: 209.85.167.41, mailfrom: savvamtr@gmail.com) Received: by mail-lf1-f41.google.com with SMTP id p7so22564247lfu.3 for ; Wed, 07 Sep 2022 06:54:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date; bh=5Akn6zKl7XE9GsvopNqhqvA1nCfpLkZ82WqydViGGys=; b=gTJdV70qTMyEQeBpY7P2E2OYEDiKOHM7kSUgbkCnyBckbmZziK1j/uJi/kaoFBQfb5 MbII4H4B461hPMRgmz+ls7nxMeNZ3Sr8maAnTSsFGK8Q+BbOs3YqAFZrV/sTLB/AViq5 AkLNqYWb9JhW699ATujUymE6EpxZC2wOf8gVbg+KI37l3gBXt2Di6w8HqjAUxzsnR3Tn bLmnd9cTIiO0O0L75Nijz9vzYZ2Sa6jUYeJZkIm0S1DOyYxalF0Rssttvq69yBlxlBcf N7UzWjSla6iPuSlbws3gxrWOvxgSSSTlSegh4ehadK2cOIG/3hxqY7srfq9EV+8Byp/Q rGDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date; bh=5Akn6zKl7XE9GsvopNqhqvA1nCfpLkZ82WqydViGGys=; b=4LSXqKlVFuxQJZawtOMhBX7VgO0reXY1eqmBhi6mW8ciBdQdOiXSsyOKfr9OPr6pig 8qM7bIttFUbbHTJW+x9ZMWl/jpWYKxRK/H8mW+S8W/0yh+07IS0/JXYBIae3Pn+3IO1d GflKEiEsLNHpt3pMpyqmlrIaIIsV9oDwCK5i5e1KG0kovXAyHeMeJPUCRELcyHI8qett zT5zI5bIROriGeMInK2uSPrZ7YqSCDdrzR9aEL9BYszmrASnHIm1SKokfn/hjg7QW0lV 9FetE90dbhFVqlVPPf0BuadTq/MG+pPLqfqNHUG6eBF/PzgUUm6rq+zm2I/9cVpiCUNq yDUQ== X-Gm-Message-State: ACgBeo1HrdSEtovtILi0j4oa/0x6Od/m7A0UKlKCsIS10J7JzuUZFITV FsmAp+VD0oXMqa5m0LguJNU= X-Google-Smtp-Source: AA6agR4SyOWxd2BN0OKvjWaRKkZK/Cik+Kc/ZhAAoixolEZo471aJEribfVcJ3CsETIKVHGl5UYK2g== X-Received: by 2002:a19:6706:0:b0:494:b2d0:3b3e with SMTP id b6-20020a196706000000b00494b2d03b3emr1093017lfc.179.1662558842672; Wed, 07 Sep 2022 06:54:02 -0700 (PDT) Return-Path: Received: from smtpclient.apple ([176.59.149.223]) by smtp.gmail.com with ESMTPSA id m14-20020a056512358e00b004979dc8038esm1044322lfr.201.2022.09.07.06.54.00 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Sep 2022 06:54:02 -0700 (PDT) From: "Savva Mitrofanov" Message-Id: <63002AE1-1C14-4A56-B8DF-CA851BE5E400@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) Subject: Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support Date: Wed, 7 Sep 2022 19:53:58 +0600 In-Reply-To: Cc: vit9696@protonmail.com, =?utf-8?Q?Marvin_H=C3=A4user?= , devel@edk2.groups.io To: Pedro Falcato References: <20220728152644.11435-1-savvamtr@gmail.com> <20220728152644.11435-2-savvamtr@gmail.com> X-Mailer: Apple Mail (2.3696.120.41.1.1) Content-Type: multipart/alternative; boundary="Apple-Mail=_96E02D21-FAEF-439B-8DBE-DA10127F6F28" --Apple-Mail=_96E02D21-FAEF-439B-8DBE-DA10127F6F28 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Pedro, Thank you for your code-review. I answer to comments inline too. > On 30 Aug 2022, at 04:12, Pedro Falcato = wrote: >=20 > Hi Savva, >=20 > Sorry for the huge delay. Comments inline. >=20 > On Thu, Jul 28, 2022 at 4:26 PM Savva Mitrofanov > wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3677 = >=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 | 13 +- > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 98 +++++- > Features/Ext4Pkg/Ext4Dxe/File.c | 359 ++++++++++++++++++-- > Features/Ext4Pkg/Ext4Dxe/Inode.c | 53 +++ > 4 files changed, 485 insertions(+), 38 deletions(-) >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h > index a55cd2fa68ad..a73e3f8622f1 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h > @@ -171,7 +171,7 @@ > #define EXT4_DIRTY_FL 0x00000100 > #define EXT4_COMPRBLK_FL 0x00000200 > #define EXT4_NOCOMPR_FL 0x00000400 > -#define EXT4_ECOMPR_FL 0x00000800 > +#define EXT4_ENCRYPT_FL 0x00000800 > #define EXT4_BTREE_FL 0x00001000 > #define EXT4_INDEX_FL 0x00002000 > #define EXT4_JOURNAL_DATA_FL 0x00004000 > @@ -332,11 +332,12 @@ STATIC_ASSERT ( > "ext4 block group descriptor struct has incorrect size" > ); >=20 > -#define EXT4_DBLOCKS 12 > -#define EXT4_IND_BLOCK 12 > -#define EXT4_DIND_BLOCK 13 > -#define EXT4_TIND_BLOCK 14 > -#define EXT4_NR_BLOCKS 15 > +#define EXT4_DBLOCKS 12 > +#define EXT4_IND_BLOCK 12 > +#define EXT4_DIND_BLOCK 13 > +#define EXT4_TIND_BLOCK 14 > +#define EXT4_NR_BLOCKS 15 > +#define EXT4_FAST_SYMLINK_MAX_SIZE EXT4_NR_BLOCKS * sizeof(UINT32) >=20 > #define EXT4_GOOD_OLD_INODE_SIZE 128 >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > index b1508482b0a7..c1df9d1149e4 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > @@ -31,7 +31,9 @@ >=20 > #include "Ext4Disk.h" >=20 > +#define SYMLOOP_MAX 8 > I'm scared that this may not be enough. Could we use 40 here as Linux = does? As we discussed, the limit equal 8 should be compatible for booting all = modern operation systems using this fs driver. In this recursion-based symlink implementation it is a bit dangerous due = to possible exceed of stack size. So we decided keep this limit until we are do the lookup logic like namei. > #define EXT4_NAME_MAX 255 > +#define EFI_PATH_MAX 4096 > Don't use EFI_* in Ext4Dxe code, so prefix this with EXT4_ instead = maybe? As a result of consensus, it was decided to use EXT4_EFI_PATH_MAX. >=20 > #define EXT4_DRIVER_VERSION 0x0000 >=20 > @@ -324,11 +326,11 @@ number of read bytes. > **/ > EFI_STATUS > Ext4Read ( > - IN EXT4_PARTITION *Partition, > - IN EXT4_FILE *File, > - OUT VOID *Buffer, > - IN UINT64 Offset, > - IN OUT UINTN *Length > + IN EXT4_PARTITION *Partition, > + IN EXT4_FILE *File, > + OUT VOID *Buffer, > + IN UINT64 Offset, > + IN OUT UINTN *Length > ); >=20 > /** > @@ -368,6 +370,7 @@ struct _Ext4File { >=20 > UINT64 OpenMode; > UINT64 Position; > + UINT32 SymLoops; >=20 > EXT4_PARTITION *Partition; >=20 > @@ -497,6 +500,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, > First IN parameters, then OUT parameters. Due to lack of strict order conventions on procedure parameters, I = consider to keep it unchanged, because all important C APIs have destination first. >=20 > + IN EXT4_FILE *Source, > + IN CHAR16 *FileName, > + IN UINT64 OpenMode, > + IN UINT64 Attributes > + ); > + > /** > Closes a file. >=20 > @@ -774,6 +816,30 @@ Ext4FileIsDir ( > IN CONST EXT4_FILE *File > ); >=20 > +/** > + Checks if a file is a symlink. > + > + @param[in] File Pointer to the opened file. > + > + @return BOOLEAN Whether file is a symlink > +**/ > +BOOLEAN > +Ext4FileIsSymlink ( > + IN CONST EXT4_FILE *File > + ); > + > +/** > + Detects if a symlink is a fast symlink. > + > + @param[in] File Pointer to the opened file. > + > + @return BOOLEAN Whether symlink is a fast symlink > +**/ > +BOOLEAN > +Ext4SymlinkIsFastSymlink ( > + IN CONST EXT4_FILE *File > + ); > Does this need to be in Ext4Dxe.h? This is pretty symlink specific.=20 I agree with you, in the next patch I'll separate symlink-related = functions into independent file. > + > /** > Checks if a file is a regular file. > @param[in] File Pointer to the opened file. > @@ -797,7 +863,7 @@ Ext4FileIsReg ( > it's a regular file or a directory, since most other file = types > don't make sense under UEFI. > **/ > -#define Ext4FileIsOpenable(File) (Ext4FileIsReg(File) || = Ext4FileIsDir(File)) > +#define Ext4FileIsOpenable(File) (Ext4FileIsReg (File) || = Ext4FileIsDir (File) || Ext4FileIsSymlink (File)) >=20 > #define EXT4_INODE_HAS_FIELD(Inode, Field) = \ > (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=3D = \ > @@ -935,6 +1001,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..ae9230d6422b 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/File.c > +++ b/Features/Ext4Pkg/Ext4Dxe/File.c > @@ -134,14 +134,224 @@ Ext4DirCanLookup ( > return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) =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 > + ) > +{ > + UINT32 SymlinkSize; > + CHAR8 *AsciiSymlinkTmp; > + // > + // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when = we checked it in > + // Ext4SymlinkIsFastSymlink(), so truncate if necessary. > + // > I think the correct solution here is to check if the inode size fits = in EXT4_FAST_SYMLINK_MAX_SIZE in Ext4SymlinkIsFastSymlink. Or possibly = check it here and error-out (blatant corruption). > Truncating doesn't seem correct. The origin problem comes from old filesystems (ext2,ext3) which can have = wrong inode size for symlinks. There are two behaviours on detection = logic, the old one,=20 which is suitable for old filesystems is based on checking presence of = extended attribute and doesnt have inode size check. So in such case we = just additionally truncate it by EXT4_FAST_SYMLINK_MAX_SIZE using MIN = macro. > + SymlinkSize =3D (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), = 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")); > Nit: DEBUG_ERROR?=20 > + 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; > +} > + > +/** > + 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 > EFI_PATH_MAX - 1) { > Why is this > EFI_PATH_MAX - 1 and not >=3D EFI_PATH_MAX? Also, why = does the inode size need to be strictly smaller than EFI_PATH_MAX?=20 Yes, my mistake, I'll correct it in the next patch. > + DEBUG (( > + DEBUG_FS, > Nit: DEBUG_ERROR?=20 I'm not agree that there we should use DEBUG_ERROR, because it will = threats like fatal error, and it can breaks boot process in some = configurations, that why I kept DEBUG_FS in this condition. The logic is = simple - if something wrong with symlink then just skip it. So, as a = result of the discussion, we came to a consensus to replace such places = with DEBUG_WARN macros. > + "[ext4] Error! Symlink path maximum length was hit!\n" > + )); > + return EFI_INVALID_PARAMETER; > + } > + > + SymlinkAllocateSize =3D (UINT32)SymlinkSizeTmp + 1; > + > + 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; > + } > +=20 > + ReadSize =3D (UINTN)SymlinkSizeTmp; > + Status =3D Ext4Read (Partition, File, SymlinkTmp, File->Position, = &ReadSize); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with = Status %r\n", Status)); > Nit: lowercase "status" in the DEBUG. > + FreePool (SymlinkTmp); > + return Status; > + } > + > + File->Position +=3D ReadSize; > Taking Position into account (here and in the Ext4Read call) seems = wrong. Why would the file seek affect the readlink() operation? Yes, it's pointless there, I'll remove it in the next revision. > + > + // > + // Add null-terminator > + // > + SymlinkTmp[SymlinkSizeTmp] =3D '\0'; > + > + if (SymlinkSizeTmp !=3D ReadSize) { > + DEBUG (( > + DEBUG_FS, > + "[ext4] Error! The sz of the read block doesn't match the value = from the inode!\n" > Nit: size=20 > + )); > + 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 > + ) > +{ > + 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) { > + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n")); > Nit: DEBUG_ERROR and "Error: ". > + return EFI_ACCESS_DENIED; > + } > + > + 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) { > + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode = string buffer\n")); > + FreePool (SymlinkTmp); > + return EFI_OUT_OF_RESOURCES; > + } > + > + 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 > + // > + 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. >=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 +375,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 +389,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 +404,11 @@ Ext4Open ( > } >=20 > while (FileName[0] !=3D L'\0') { > + if (Partition->Root-> > =20 > > 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 +453,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 +515,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; > Keeping SymLoops in the root FILE seems kind of out of place. My idea = here (and this was going to be chained with a bunch of improvements to = the path walking logic, which is why I never got around to adding = symlink support) was to add a nameidata(see Onyx code, or NetBSD's = namei(9) manpage)-like structure, as you would in typical UNIX fashion. = You would then do your whole path traversal on that structure, which = would keep your path walking state, walked_symlinks, etc. We will fix it in the future patches, when we will provide namei-like = mechanism for proceeding symlinks. > + > + Status =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 +893,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 +1059,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 +1177,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..e7a6b3225709 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c > @@ -255,6 +255,59 @@ Ext4FileIsDir ( > return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) =3D=3D = EXT4_INO_TYPE_DIR; > } >=20 > +/** > + Checks if a file is a symlink. > + > + @param[in] File Pointer to the opened file. > + > + @return BOOLEAN Whether file is a symlink > +**/ > +BOOLEAN > +Ext4FileIsSymlink ( > + IN CONST EXT4_FILE *File > + ) > +{ > + return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) =3D=3D = EXT4_INO_TYPE_SYMLINK; > +} > + > +/** > + Detects if a symlink is a fast symlink. > + > + @param[in] File Pointer to the opened file. > + > + @return BOOLEAN Whether symlink is a fast symlink > +**/ > +BOOLEAN > +Ext4SymlinkIsFastSymlink ( > + IN CONST EXT4_FILE *File > + ) > +{ > + // > + // Detection logic of the fast-symlink splits into two behaviors - = old and new. > + // The old behavior is based on comparing the extended attribute = blocks > + // with the inode's i_blocks, and if it's zero we know the inode = isn't storing > + // the link in filesystem blocks, so we look to the inode->i_data. > + // The new behavior is apparently needed only with the large EA = inode feature. > + // In this case we check that inode size less than maximum fast = symlink size. > + // So, we revert to the old behavior if the large EA inode feature = is not set. > + // > + UINT32 FileAcl; > + UINT32 ExtAttrBlocks; > + > + if ((File->Inode->i_flags & EXT4_EA_INODE_FL) =3D=3D 0) { > + 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; > +} > + > /** > Checks if a file is a regular file. > @param[in] File Pointer to the opened file. > --=20 > 2.37.1 >=20 >=20 >=20 > --=20 > Pedro Falcato All the best! Savva Mitrofanov --Apple-Mail=_96E02D21-FAEF-439B-8DBE-DA10127F6F28 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Hi Pedro,

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

On 30 = Aug 2022, at 04:12, Pedro Falcato <pedro.falcato@gmail.com> wrote:

Hi Savva,

Sorry for the huge delay. Comments = inline.

On Thu, Jul 28, 2022 at 4:26 PM Savva = Mitrofanov <savvamtr@gmail.com> wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3677

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 |  13 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  98 +++++-
 Features/Ext4Pkg/Ext4Dxe/File.c     | 359 = ++++++++++++++++++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c    |  53 +++
 4 files changed, 485 insertions(+), 38 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index a55cd2fa68ad..a73e3f8622f1 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -171,7 +171,7 @@
 #define EXT4_DIRTY_FL        =  0x00000100
 #define EXT4_COMPRBLK_FL      0x00000200
 #define EXT4_NOCOMPR_FL       0x00000400
-#define EXT4_ECOMPR_FL        0x00000800
+#define EXT4_ENCRYPT_FL       0x00000800
 #define EXT4_BTREE_FL        =  0x00001000
 #define EXT4_INDEX_FL        =  0x00002000
 #define EXT4_JOURNAL_DATA_FL  0x00004000
@@ -332,11 +332,12 @@ STATIC_ASSERT (
   "ext4 block group descriptor struct has incorrect size"
   );

-#define EXT4_DBLOCKS     12
-#define EXT4_IND_BLOCK   12
-#define EXT4_DIND_BLOCK  13
-#define EXT4_TIND_BLOCK  14
-#define EXT4_NR_BLOCKS   15
+#define EXT4_DBLOCKS              =   12
+#define EXT4_IND_BLOCK              = 12
+#define EXT4_DIND_BLOCK            =  13
+#define EXT4_TIND_BLOCK            =  14
+#define EXT4_NR_BLOCKS              = 15
+#define EXT4_FAST_SYMLINK_MAX_SIZE  EXT4_NR_BLOCKS * = sizeof(UINT32)

 #define EXT4_GOOD_OLD_INODE_SIZE  128

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index b1508482b0a7..c1df9d1149e4 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -31,7 +31,9 @@

 #include "Ext4Disk.h"

+#define SYMLOOP_MAX    8
I'm scared that this may not be enough. Could we use 40 here = as Linux does?
As we discussed, the limit equal 8 should be = compatible for booting all modern operation systems using this fs = driver.
In this recursion-based symlink implementation it is a = bit dangerous due to possible exceed of stack size. So = we
decided keep this limit until we are do the lookup logic = like namei.

 #define EXT4_NAME_MAX  255
+#define EFI_PATH_MAX   4096
Don't use EFI_* in Ext4Dxe code, so prefix this with EXT4_ = instead maybe?
As a result of consensus, it was decided to use = EXT4_EFI_PATH_MAX.


 #define EXT4_DRIVER_VERSION  0x0000

@@ -324,11 +326,11 @@ number of read bytes.
 **/
 EFI_STATUS
 Ext4Read (
-  IN EXT4_PARTITION  *Partition,
-  IN EXT4_FILE       *File,
-  OUT VOID           *Buffer,
-  IN UINT64          Offset,
= -  IN OUT UINTN       *Length
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE      =  *File,
+  OUT    VOID            = *Buffer,
+  IN     UINT64          = Offset,
+  IN OUT UINTN           *Length
   );

 /**
@@ -368,6 +370,7 @@ struct _Ext4File {

   UINT64              =   OpenMode;
   UINT64              =   Position;
+  UINT32                = SymLoops;

   EXT4_PARTITION        *Partition;

@@ -497,6 +500,45 @@ Ext4SetupFile (
   IN EXT4_PARTITION  *Partition
   );

+/**
+  Opens a new file relative to the source file's location.
+
+  @param[out] FoundFile  A pointer to the location to return = the opened handle for the new
+                    =      file.
+  @param[in]  Source     A pointer to the = EXT4_FILE instance that is the file
+                    =      handle to the source location. This would typically = be an open
+                    =      handle to a directory.
+  @param[in]  FileName   The Null-terminated string = of the name of the file to be opened.
+                    =      The file name may contain the following path = modifiers: "\", ".",
+                    =      and "..".
+  @param[in]  OpenMode   The mode to open the file. = The only valid combinations that the
+                    =      file may be opened with are: Read, Read/Write, or = Create/Read/Write.
+  @param[in]  Attributes Only valid for EFI_FILE_MODE_CREATE, = in which case these are the
+                    =      attribute bits for the newly created file.
+
+  @retval EFI_SUCCESS          The file = was opened.
+  @retval EFI_NOT_FOUND        The specified = file could not be found on the device.
+  @retval EFI_NO_MEDIA         The device = has no medium.
+  @retval EFI_MEDIA_CHANGED    The device has a = different medium in it or the medium is no
+                    =            longer supported.
+  @retval EFI_DEVICE_ERROR     The device reported = an error.
+  @retval EFI_VOLUME_CORRUPTED The file system structures are = corrupted.
+  @retval EFI_WRITE_PROTECTED  An attempt was made to create = a file, or open a file for write
+                    =            when the media is = write-protected.
+  @retval EFI_ACCESS_DENIED    The service denied access = to the file.
+  @retval EFI_OUT_OF_RESOURCES Not enough resources were available = to open the file.
+  @retval EFI_VOLUME_FULL      The volume is = full.
+
+**/
+EFI_STATUS
+Ext4OpenInternal (
+  OUT EXT4_FILE  **FoundFile,
First IN parameters, then OUT = parameters.

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


+  IN  EXT4_FILE  *Source,
+  IN  CHAR16     *FileName,
+  IN  UINT64     OpenMode,
+  IN  UINT64     Attributes
+  );
+
 /**
    Closes a file.

@@ -774,6 +816,30 @@ Ext4FileIsDir (
   IN CONST EXT4_FILE  *File
   );

+/**
+   Checks if a file is a symlink.
+
+   @param[in]      File      =     Pointer to the opened file.
+
+   @return BOOLEAN         Whether = file is a symlink
+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE  *File
+  );
+
+/**
+   Detects if a symlink is a fast symlink.
+
+   @param[in]      File      =     Pointer to the opened file.
+
+   @return BOOLEAN         Whether = symlink is a fast symlink
+**/
+BOOLEAN
+Ext4SymlinkIsFastSymlink (
+  IN CONST EXT4_FILE  *File
+  );
Does this need to = be in Ext4Dxe.h? This is pretty symlink specific.

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

+
 /**
    Checks if a file is a regular file.
    @param[in]      File      =     Pointer to the opened file.
@@ -797,7 +863,7 @@ Ext4FileIsReg (
            it's a regular file or a = directory, since most other file types
            don't make sense under = UEFI.
 **/
-#define Ext4FileIsOpenable(File)  (Ext4FileIsReg(File) || = Ext4FileIsDir(File))
+#define Ext4FileIsOpenable(File)  (Ext4FileIsReg (File) || = Ext4FileIsDir (File) || Ext4FileIsSymlink (File))

 #define EXT4_INODE_HAS_FIELD(Inode, Field)      =                     =            \
   (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE = >=3D                  =         \
@@ -935,6 +1001,26 @@ Ext4ReadDir (
   IN OUT UINTN       *OutLength
   );

+/**
+  Reads a symlink file.
+
+  @param[in]      Partition   Pointer to = the ext4 partition.
+  @param[in]      File        = Pointer to the open symlink file.
+  @param[out]     Symlink    =  Pointer to the output unicode symlink string.
+
+  @retval EFI_SUCCESS          =  Symlink was read.
+  @retval EFI_ACCESS_DENIED     Symlink is = encrypted.
+  @retval EFI_OUT_OF_RESOURCES  Memory allocation error.
+  @retval EFI_INVALID_PARAMETER Symlink path has incorrect = length
+  @retval EFI_VOLUME_CORRUPTED  Symlink read block size = differ from inode value
+**/
+EFI_STATUS
+Ext4ReadSymlink (
+  IN     EXT4_PARTITION  *Partition,
+  IN     EXT4_FILE      =  *File,
+  OUT    CHAR16          = **Symlink
+  );
+
 /**
    Initialises the (empty) extents map, that will work as a = cache of extents.

diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c = b/Features/Ext4Pkg/Ext4Dxe/File.c
index ff1746d5640a..ae9230d6422b 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -134,14 +134,224 @@ Ext4DirCanLookup (
   return (File->Inode->i_mode & = EXT4_INO_PERM_EXEC_OWNER) =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
+  )
+{
+  UINT32  SymlinkSize;
+  CHAR8   *AsciiSymlinkTmp;
+  //
+  // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated = when we checked it in
+  // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
+  //
I think the = correct solution here is to check if the inode size fits in = EXT4_FAST_SYMLINK_MAX_SIZE in Ext4SymlinkIsFastSymlink. Or possibly = check it here and error-out (blatant corruption).
Truncating doesn't seem correct.

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

+  SymlinkSize =3D (UINT32)MIN (EXT4_INODE_SIZE (File->Inode), = 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"));
Nit: = DEBUG_ERROR?
+    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;
+}
+
+/**
+  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 > EFI_PATH_MAX - 1) {
Why is this > EFI_PATH_MAX - = 1 and not >=3D EFI_PATH_MAX? Also, why does the inode size need to be = strictly smaller than EFI_PATH_MAX?

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

+    DEBUG ((
+      DEBUG_FS,
Nit: DEBUG_ERROR?

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


+      "[ext4] Error! Symlink path maximum length was = hit!\n"
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  SymlinkAllocateSize =3D (UINT32)SymlinkSizeTmp + 1;
= +
+  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)) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from = blocks with Status %r\n", Status));
Nit: lowercase "status" in the DEBUG.
+    FreePool (SymlinkTmp);
+    return Status;
+  }
+
+  File->Position +=3D ReadSize;
Taking Position into account (here and in the Ext4Read call) = seems wrong. Why would the file seek affect the readlink() operation?

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

+
+  //
+  // Add null-terminator
+  //
+  SymlinkTmp[SymlinkSizeTmp] =3D '\0';
+
+  if (SymlinkSizeTmp !=3D ReadSize) {
+    DEBUG ((
+      DEBUG_FS,
+      "[ext4] Error! The sz of the read block doesn't = match the value from the inode!\n"
Nit: size
+      ));
+    return EFI_VOLUME_CORRUPTED;
+  }
+
+  *AsciiSymlinkSize =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
+  )
+{
+  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) = {
+    DEBUG ((DEBUG_FS, "[ext4] Error, symlink is = encrypted\n"));
Nit: = DEBUG_ERROR and "Error: ".
+    return EFI_ACCESS_DENIED;
+  }
+
+  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) {
+    DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink = unicode string buffer\n"));
+    FreePool (SymlinkTmp);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  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
+  //
+  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
+                    =      file.
+  @param[in]  Source     A pointer to the = EXT4_FILE instance that is the file
                    =       handle to the source location. This would typically = be an open
                    =       handle to a directory.
-  @param[out] NewHandle  A pointer to the location to return = the opened handle for the new
-                    =      file.
   @param[in]  FileName   The Null-terminated = string of the name of the file to be opened.
                    =       The file name may contain the following path = modifiers: "\", ".",
                    =       and "..".
@@ -165,13 +375,12 @@ Ext4DirCanLookup (

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

-  Current   =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 +404,11 @@ Ext4Open (
   }

   while (FileName[0] !=3D L'\0') {
+    if (Partition->Root->
 
> SYMLOOP_MAX) {
+      DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit = !\n"));
+      return EFI_ACCESS_DENIED;
+    }
+
     // Discard leading path separators
     while (FileName[0] =3D=3D L'\\') {
       FileName++;
@@ -238,18 +453,45 @@ Ext4Open (
     }

     // Check if this is a valid file to open in EFI
-
-    // What to do with symlinks? They're nonsense when = absolute but may
-    // be useful when they're relative. Right now, they're = ignored, since they
-    // bring a lot of trouble for something that's not as = useful in our case.
-    // If you want to link, use hard links.
-
     if (!Ext4FileIsOpenable (File)) {
       Ext4CloseInternal (File);
       // This looks like an /okay/ status to = return.
       return EFI_ACCESS_DENIED;
     }

+    //
+    // Reading symlink and then trying to follow it
+    //
+    if (Ext4FileIsSymlink (File)) {
+      Partition->Root->SymLoops++;
+      DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, = trying to read it\n", PathSegment));
+      Status =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 +515,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
+                    =      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;
Keeping SymLoops in the root = FILE seems kind of out of place. My idea here (and this was going to be = chained with a bunch of improvements to the path walking logic, which is = why I never got around to adding symlink support) was to add a = nameidata(see Onyx code, or NetBSD's namei(9) manpage)-like structure, = as you would in typical UNIX fashion. You would then do your whole path = traversal on that structure, which would keep your path walking state, = walked_symlinks, etc.

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

+
+  Status =3D Ext4OpenInternal (
+             &FoundFile,
+             Source,
+             FileName,
+             OpenMode,
+             Attributes
= +             );
+
+  if (!EFI_ERROR (Status)) {
+    *NewHandle =3D &FoundFile->Protocol;
+  }
+
+  return Status;
+}
+
 /**
   Closes a specified file handle.

@@ -588,7 +893,7 @@ Ext4GetVolumeName (

   // s_volume_name is only valid on dynamic revision; old = filesystems don't support this
   if (Partition->SuperBlock.s_rev_level =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 +1059,14 @@ Ext4GetInfo (
   OUT VOID              = *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 +1177,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..e7a6b3225709 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -255,6 +255,59 @@ Ext4FileIsDir (
   return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) = =3D=3D EXT4_INO_TYPE_DIR;
 }

+/**
+   Checks if a file is a symlink.
+
+   @param[in]      File      =     Pointer to the opened file.
+
+   @return BOOLEAN         Whether = file is a symlink
+**/
+BOOLEAN
+Ext4FileIsSymlink (
+  IN CONST EXT4_FILE  *File
+  )
+{
+  return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) = =3D=3D EXT4_INO_TYPE_SYMLINK;
+}
+
+/**
+   Detects if a symlink is a fast symlink.
+
+   @param[in]      File      =     Pointer to the opened file.
+
+   @return BOOLEAN         Whether = symlink is a fast symlink
+**/
+BOOLEAN
+Ext4SymlinkIsFastSymlink (
+  IN CONST EXT4_FILE  *File
+  )
+{
+  //
+  // Detection logic of the fast-symlink splits into two behaviors = - old and new.
+  // The old behavior is based on comparing the extended attribute = blocks
+  // with the inode's i_blocks, and if it's zero we know the inode = isn't storing
+  // the link in filesystem blocks, so we look to the = inode->i_data.
+  // The new behavior is apparently needed only with the large EA = inode feature.
+  // In this case we check that inode size less than maximum fast = symlink size.
+  // So, we revert to the old behavior if the large EA inode = feature is not set.
+  //
+  UINT32  FileAcl;
+  UINT32  ExtAttrBlocks;
+
+  if ((File->Inode->i_flags & EXT4_EA_INODE_FL) =3D=3D = 0) {
+    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;
+}
+
 /**
    Checks if a file is a regular file.
    @param[in]      File      =     Pointer to the opened file.
--
2.37.1



--
Pedro Falcato

All the = best!
Savva Mitrofanov

= --Apple-Mail=_96E02D21-FAEF-439B-8DBE-DA10127F6F28--