From: "Pedro Falcato" <pedro.falcato@gmail.com>
To: Savva Mitrofanov <savvamtr@gmail.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
"Marvin Häuser" <mhaeuser@posteo.de>,
"Vitaly Cheptsov" <vit9696@protonmail.com>
Subject: Re: [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support
Date: Mon, 29 Aug 2022 23:12:35 +0100 [thread overview]
Message-ID: <CAKbZUD3xbgfPbqTZi7af5+sRD8WjDAc_Uij47e9GmxYzgbbSZQ@mail.gmail.com> (raw)
In-Reply-To: <20220728152644.11435-2-savvamtr@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 27329 bytes --]
Hi Savva,
Sorry for the huge delay. Comments inline.
On Thu, Jul 28, 2022 at 4:26 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3677
>
> Provided support for symlink file type. Added routine which allows
> reading and following them through recursive open() call. As a security
> meausure implemented simple symlink loop check with nest level limit
> equal 8. Also this patch moves Ext4Open functionality to internal
> routine.
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 13 +-
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 98 +++++-
> Features/Ext4Pkg/Ext4Dxe/File.c | 359 ++++++++++++++++++--
> Features/Ext4Pkg/Ext4Dxe/Inode.c | 53 +++
> 4 files changed, 485 insertions(+), 38 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index a55cd2fa68ad..a73e3f8622f1 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -171,7 +171,7 @@
> #define EXT4_DIRTY_FL 0x00000100
> #define EXT4_COMPRBLK_FL 0x00000200
> #define EXT4_NOCOMPR_FL 0x00000400
> -#define EXT4_ECOMPR_FL 0x00000800
> +#define EXT4_ENCRYPT_FL 0x00000800
> #define EXT4_BTREE_FL 0x00001000
> #define EXT4_INDEX_FL 0x00002000
> #define EXT4_JOURNAL_DATA_FL 0x00004000
> @@ -332,11 +332,12 @@ STATIC_ASSERT (
> "ext4 block group descriptor struct has incorrect size"
> );
>
> -#define EXT4_DBLOCKS 12
> -#define EXT4_IND_BLOCK 12
> -#define EXT4_DIND_BLOCK 13
> -#define EXT4_TIND_BLOCK 14
> -#define EXT4_NR_BLOCKS 15
> +#define EXT4_DBLOCKS 12
> +#define EXT4_IND_BLOCK 12
> +#define EXT4_DIND_BLOCK 13
> +#define EXT4_TIND_BLOCK 14
> +#define EXT4_NR_BLOCKS 15
> +#define EXT4_FAST_SYMLINK_MAX_SIZE EXT4_NR_BLOCKS * sizeof(UINT32)
>
> #define EXT4_GOOD_OLD_INODE_SIZE 128
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index b1508482b0a7..c1df9d1149e4 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -31,7 +31,9 @@
>
> #include "Ext4Disk.h"
>
> +#define SYMLOOP_MAX 8
>
I'm scared that this may not be enough. Could we use 40 here as Linux does?
> #define EXT4_NAME_MAX 255
> +#define EFI_PATH_MAX 4096
>
Don't use EFI_* in Ext4Dxe code, so prefix this with EXT4_ instead maybe?
>
> #define EXT4_DRIVER_VERSION 0x0000
>
> @@ -324,11 +326,11 @@ number of read bytes.
> **/
> EFI_STATUS
> Ext4Read (
> - IN EXT4_PARTITION *Partition,
> - IN EXT4_FILE *File,
> - OUT VOID *Buffer,
> - IN UINT64 Offset,
> - IN OUT UINTN *Length
> + IN EXT4_PARTITION *Partition,
> + IN EXT4_FILE *File,
> + OUT VOID *Buffer,
> + IN UINT64 Offset,
> + IN OUT UINTN *Length
> );
>
> /**
> @@ -368,6 +370,7 @@ struct _Ext4File {
>
> UINT64 OpenMode;
> UINT64 Position;
> + UINT32 SymLoops;
>
> EXT4_PARTITION *Partition;
>
> @@ -497,6 +500,45 @@ Ext4SetupFile (
> IN EXT4_PARTITION *Partition
> );
>
> +/**
> + Opens a new file relative to the source file's location.
> +
> + @param[out] FoundFile A pointer to the location to return the opened
> handle for the new
> + file.
> + @param[in] Source A pointer to the EXT4_FILE instance that is the
> file
> + handle to the source location. This would
> typically be an open
> + handle to a directory.
> + @param[in] FileName The Null-terminated string of the name of the
> file to be opened.
> + The file name may contain the following path
> modifiers: "\", ".",
> + and "..".
> + @param[in] OpenMode The mode to open the file. The only valid
> combinations that the
> + file may be opened with are: Read, Read/Write,
> or Create/Read/Write.
> + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which
> case these are the
> + attribute bits for the newly created file.
> +
> + @retval EFI_SUCCESS The file was opened.
> + @retval EFI_NOT_FOUND The specified file could not be found on
> the device.
> + @retval EFI_NO_MEDIA The device has no medium.
> + @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> the medium is no
> + longer supported.
> + @retval EFI_DEVICE_ERROR The device reported an error.
> + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or
> open a file for write
> + when the media is write-protected.
> + @retval EFI_ACCESS_DENIED The service denied access to the file.
> + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to
> open the file.
> + @retval EFI_VOLUME_FULL The volume is full.
> +
> +**/
> +EFI_STATUS
> +Ext4OpenInternal (
> + OUT EXT4_FILE **FoundFile,
>
First IN parameters, then OUT parameters.
> + IN EXT4_FILE *Source,
> + IN CHAR16 *FileName,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + );
> +
> /**
> Closes a file.
>
> @@ -774,6 +816,30 @@ Ext4FileIsDir (
> IN CONST EXT4_FILE *File
> );
>
> +/**
> + Checks if a file is a symlink.
> +
> + @param[in] File Pointer to the opened file.
> +
> + @return BOOLEAN Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> + IN CONST EXT4_FILE *File
> + );
> +
> +/**
> + Detects if a symlink is a fast symlink.
> +
> + @param[in] File Pointer to the opened file.
> +
> + @return BOOLEAN Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> + IN CONST EXT4_FILE *File
> + );
>
Does this need to be in Ext4Dxe.h? This is pretty symlink specific.
> +
> /**
> Checks if a file is a regular file.
> @param[in] File Pointer to the opened file.
> @@ -797,7 +863,7 @@ Ext4FileIsReg (
> it's a regular file or a directory, since most other file types
> don't make sense under UEFI.
> **/
> -#define Ext4FileIsOpenable(File) (Ext4FileIsReg(File) ||
> Ext4FileIsDir(File))
> +#define Ext4FileIsOpenable(File) (Ext4FileIsReg (File) || Ext4FileIsDir
> (File) || Ext4FileIsSymlink (File))
>
> #define EXT4_INODE_HAS_FIELD(Inode, Field)
> \
> (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >=
> \
> @@ -935,6 +1001,26 @@ Ext4ReadDir (
> IN OUT UINTN *OutLength
> );
>
> +/**
> + Reads a symlink file.
> +
> + @param[in] Partition Pointer to the ext4 partition.
> + @param[in] File Pointer to the open symlink file.
> + @param[out] Symlink Pointer to the output unicode symlink
> string.
> +
> + @retval EFI_SUCCESS Symlink was read.
> + @retval EFI_ACCESS_DENIED Symlink is encrypted.
> + @retval EFI_OUT_OF_RESOURCES Memory allocation error.
> + @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode
> value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> + IN EXT4_PARTITION *Partition,
> + IN EXT4_FILE *File,
> + OUT CHAR16 **Symlink
> + );
> +
> /**
> Initialises the (empty) extents map, that will work as a cache of
> extents.
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c
> b/Features/Ext4Pkg/Ext4Dxe/File.c
> index ff1746d5640a..ae9230d6422b 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
> @@ -134,14 +134,224 @@ Ext4DirCanLookup (
> return (File->Inode->i_mode & EXT4_INO_PERM_EXEC_OWNER) ==
> EXT4_INO_PERM_EXEC_OWNER;
> }
>
> +/**
> + Reads a fast symlink file.
> +
> + @param[in] Partition Pointer to the ext4 partition.
> + @param[in] File Pointer to the open symlink file.
> + @param[out] AsciiSymlink Pointer to the output ascii symlink
> string.
> + @param[out] AsciiSymlinkSize Pointer to the output ascii symlink
> string length.
> +
> + @retval EFI_SUCCESS Fast symlink was read.
> + @retval EFI_OUT_OF_RESOURCES Memory allocation error.
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadFastSymlink (
> + IN EXT4_PARTITION *Partition,
> + IN EXT4_FILE *File,
> + OUT CHAR8 **AsciiSymlink,
> + OUT UINT32 *AsciiSymlinkSize
> + )
> +{
> + UINT32 SymlinkSize;
> + CHAR8 *AsciiSymlinkTmp;
> + //
> + // Fast-symlink's EXT4_INODE_SIZE is not necessarily validated when we
> checked it in
> + // Ext4SymlinkIsFastSymlink(), so truncate if necessary.
> + //
>
I think the correct solution here is to check if the inode size fits in
EXT4_FAST_SYMLINK_MAX_SIZE in Ext4SymlinkIsFastSymlink. Or possibly check
it here and error-out (blatant corruption).
Truncating doesn't seem correct.
> + SymlinkSize = (UINT32)MIN (EXT4_INODE_SIZE (File->Inode),
> EXT4_FAST_SYMLINK_MAX_SIZE);
> +
> + AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1);
> + if (AsciiSymlinkTmp == NULL) {
> + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string
> buffer\n"));
>
Nit: DEBUG_ERROR?
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize);
> +
> + //
> + // Add null-terminator
> + //
> + AsciiSymlinkTmp[SymlinkSize] = '\0';
> +
> + *AsciiSymlink = AsciiSymlinkTmp;
> + *AsciiSymlinkSize = SymlinkSize + 1;
> +
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + Reads a slow symlink file.
> +
> + @param[in] Partition Pointer to the ext4 partition.
> + @param[in] File Pointer to the open symlink file.
> + @param[out] AsciiSymlink Pointer to the output ascii symlink
> string.
> + @param[out] AsciiSymlinkSize Pointer to the output ascii symlink
> string length.
> +
> + @retval EFI_SUCCESS Slow symlink was read.
> + @retval EFI_OUT_OF_RESOURCES Memory allocation error.
> + @retval EFI_INVALID_PARAMETER Slow symlink path has incorrect length
> + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode
> value
> +**/
> +STATIC
> +EFI_STATUS
> +Ext4ReadSlowSymlink (
> + IN EXT4_PARTITION *Partition,
> + IN EXT4_FILE *File,
> + OUT CHAR8 **AsciiSymlink,
> + OUT UINT32 *AsciiSymlinkSize
> + )
> +{
> + EFI_STATUS Status;
> + CHAR8 *SymlinkTmp;
> + UINT64 SymlinkSizeTmp;
> + UINT32 SymlinkAllocateSize;
> + UINTN ReadSize;
> +
> + SymlinkSizeTmp = EXT4_INODE_SIZE (File->Inode);
> +
> + //
> + // Allocate EXT4_INODE_SIZE + 1
> + //
> + if (SymlinkSizeTmp > EFI_PATH_MAX - 1) {
>
Why is this > EFI_PATH_MAX - 1 and not >= EFI_PATH_MAX? Also, why does the
inode size need to be strictly smaller than EFI_PATH_MAX?
> + DEBUG ((
> + DEBUG_FS,
>
Nit: DEBUG_ERROR?
> + "[ext4] Error! Symlink path maximum length was hit!\n"
> + ));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1;
> +
> + SymlinkTmp = AllocatePool (SymlinkAllocateSize);
> + if (SymlinkTmp == NULL) {
> + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string
> buffer\n"));
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
+ ReadSize = (UINTN)SymlinkSizeTmp;
> + Status = Ext4Read (Partition, File, SymlinkTmp, File->Position,
> &ReadSize);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_FS, "[ext4] Failed to read symlink from blocks with
> Status %r\n", Status));
>
Nit: lowercase "status" in the DEBUG.
> + FreePool (SymlinkTmp);
> + return Status;
> + }
> +
> + File->Position += ReadSize;
>
Taking Position into account (here and in the Ext4Read call) seems wrong.
Why would the file seek affect the readlink() operation?
> +
> + //
> + // Add null-terminator
> + //
> + SymlinkTmp[SymlinkSizeTmp] = '\0';
> +
> + if (SymlinkSizeTmp != ReadSize) {
> + DEBUG ((
> + DEBUG_FS,
> + "[ext4] Error! The sz of the read block doesn't match the value
> from the inode!\n"
>
Nit: size
> + ));
> + return EFI_VOLUME_CORRUPTED;
> + }
> +
> + *AsciiSymlinkSize = SymlinkAllocateSize;
> + *AsciiSymlink = SymlinkTmp;
> +
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + Reads a symlink file.
> +
> + @param[in] Partition Pointer to the ext4 partition.
> + @param[in] File Pointer to the open symlink file.
> + @param[out] Symlink Pointer to the output unicode symlink
> string.
> +
> + @retval EFI_SUCCESS Symlink was read.
> + @retval EFI_ACCESS_DENIED Symlink is encrypted.
> + @retval EFI_OUT_OF_RESOURCES Memory allocation error.
> + @retval EFI_INVALID_PARAMETER Symlink path has incorrect length
> + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode
> value
> +**/
> +EFI_STATUS
> +Ext4ReadSymlink (
> + IN EXT4_PARTITION *Partition,
> + IN EXT4_FILE *File,
> + OUT CHAR16 **Symlink
> + )
> +{
> + EFI_STATUS Status;
> + CHAR8 *SymlinkTmp;
> + UINT32 SymlinkSize;
> + CHAR16 *Symlink16Tmp;
> + CHAR16 *Needle;
> +
> + //
> + // Assume that we alread read Inode via Ext4ReadInode
> + // Skip reading, just check encryption flag
> + //
> + if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) {
> + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n"));
>
Nit: DEBUG_ERROR and "Error: ".
> + return EFI_ACCESS_DENIED;
> + }
> +
> + if (Ext4SymlinkIsFastSymlink (File)) {
> + Status = Ext4ReadFastSymlink (Partition, File, &SymlinkTmp,
> &SymlinkSize);
> + } else {
> + Status = Ext4ReadSlowSymlink (Partition, File, &SymlinkTmp,
> &SymlinkSize);
> + }
> +
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_FS, "[ext4] Symlink read error with Status %r\n",
> Status));
> + return Status;
> + }
> +
> + Symlink16Tmp = AllocateZeroPool (SymlinkSize * sizeof (CHAR16));
> + if (Symlink16Tmp == NULL) {
> + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string
> buffer\n"));
> + FreePool (SymlinkTmp);
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + Status = AsciiStrToUnicodeStrS (
> + SymlinkTmp,
> + Symlink16Tmp,
> + SymlinkSize
> + );
> +
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_FS,
> + "[ext4] Failed to convert ascii symlink to unicode with Status
> %r\n",
> + Status
> + ));
> + FreePool (Symlink16Tmp);
> + FreePool (SymlinkTmp);
> + return Status;
> + }
> +
> + //
> + // Convert to UEFI slashes
> + //
> + for (Needle = Symlink16Tmp; *Needle != L'\0'; Needle++) {
> + if (*Needle == L'/') {
> + *Needle = L'\\';
> + }
> + }
> +
> + *Symlink = Symlink16Tmp;
> +
> + FreePool (SymlinkTmp);
> + return Status;
> +}
> +
> /**
> Opens a new file relative to the source file's location.
>
> - @param[in] This A pointer to the EFI_FILE_PROTOCOL instance that
> is the file
> + @param[out] FoundFile A pointer to the location to return the opened
> handle for the new
> + file.
> + @param[in] Source A pointer to the EXT4_FILE instance that is the
> file
> handle to the source location. This would
> typically be an open
> handle to a directory.
> - @param[out] NewHandle A pointer to the location to return the opened
> handle for the new
> - file.
> @param[in] FileName The Null-terminated string of the name of the
> file to be opened.
> The file name may contain the following path
> modifiers: "\", ".",
> and "..".
> @@ -165,13 +375,12 @@ Ext4DirCanLookup (
>
> **/
> EFI_STATUS
> -EFIAPI
> -Ext4Open (
> - IN EFI_FILE_PROTOCOL *This,
> - OUT EFI_FILE_PROTOCOL **NewHandle,
> - IN CHAR16 *FileName,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> +Ext4OpenInternal (
> + OUT EXT4_FILE **FoundFile,
> + IN EXT4_FILE *Source,
> + IN CHAR16 *FileName,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> )
> {
> EXT4_FILE *Current;
> @@ -180,13 +389,14 @@ Ext4Open (
> CHAR16 PathSegment[EXT4_NAME_MAX + 1];
> UINTN Length;
> EXT4_FILE *File;
> + CHAR16 *Symlink;
> EFI_STATUS Status;
>
> - Current = (EXT4_FILE *)This;
> + Current = Source;
> Partition = Current->Partition;
> Level = 0;
>
> - DEBUG ((DEBUG_FS, "[ext4] Ext4Open %s\n", FileName));
> + DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName));
> // If the path starts with a backslash, we treat the root directory as
> the base directory
> if (FileName[0] == L'\\') {
> FileName++;
> @@ -194,6 +404,11 @@ Ext4Open (
> }
>
> while (FileName[0] != L'\0') {
> + if (Partition->Root->
> > SYMLOOP_MAX) {
> + DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n"));
> + return EFI_ACCESS_DENIED;
> + }
> +
> // Discard leading path separators
> while (FileName[0] == L'\\') {
> FileName++;
> @@ -238,18 +453,45 @@ Ext4Open (
> }
>
> // Check if this is a valid file to open in EFI
> -
> - // What to do with symlinks? They're nonsense when absolute but may
> - // be useful when they're relative. Right now, they're ignored, since
> they
> - // bring a lot of trouble for something that's not as useful in our
> case.
> - // If you want to link, use hard links.
> -
> if (!Ext4FileIsOpenable (File)) {
> Ext4CloseInternal (File);
> // This looks like an /okay/ status to return.
> return EFI_ACCESS_DENIED;
> }
>
> + //
> + // Reading symlink and then trying to follow it
> + //
> + if (Ext4FileIsSymlink (File)) {
> + Partition->Root->SymLoops++;
> + DEBUG ((DEBUG_FS, "[ext4] File %s is symlink, trying to read it\n",
> PathSegment));
> + Status = Ext4ReadSymlink (Partition, File, &Symlink);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_FS, "[ext4] Error reading %s symlink!\n",
> PathSegment));
> + return Status;
> + }
> +
> + DEBUG ((DEBUG_FS, "[ext4] File %s is linked to %s\n", PathSegment,
> Symlink));
> + //
> + // Close symlink file
> + //
> + Ext4CloseInternal (File);
> + //
> + // Open linked file by recursive call of Ext4OpenFile
> + //
> + Status = Ext4OpenInternal (FoundFile, Current, Symlink, OpenMode,
> Attributes);
> + FreePool (Symlink);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_FS, "[ext4] Error opening linked file %s\n",
> Symlink));
> + return Status;
> + }
> +
> + //
> + // Set File to newly opened
> + //
> + File = *FoundFile;
> + }
> +
> if (Level != 0) {
> // Careful not to close the base directory
> Ext4CloseInternal (Current);
> @@ -273,12 +515,75 @@ Ext4Open (
> return EFI_ACCESS_DENIED;
> }
>
> - *NewHandle = &Current->Protocol;
> + *FoundFile = Current;
>
> DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n",
> Current->Dentry->Name));
> return EFI_SUCCESS;
> }
>
> +/**
> + Opens a new file relative to the source file's location.
> + @param[in] This A pointer to the EFI_FILE_PROTOCOL instance that
> is the file
> + handle to the source location. This would
> typically be an open
> + handle to a directory.
> + @param[out] NewHandle A pointer to the location to return the opened
> handle for the new
> + file.
> + @param[in] FileName The Null-terminated string of the name of the
> file to be opened.
> + The file name may contain the following path
> modifiers: "\", ".",
> + and "..".
> + @param[in] OpenMode The mode to open the file. The only valid
> combinations that the
> + file may be opened with are: Read, Read/Write,
> or Create/Read/Write.
> + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which
> case these are the
> + attribute bits for the newly created file.
> + @retval EFI_SUCCESS The file was opened.
> + @retval EFI_NOT_FOUND The specified file could not be found on
> the device.
> + @retval EFI_NO_MEDIA The device has no medium.
> + @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> the medium is no
> + longer supported.
> + @retval EFI_DEVICE_ERROR The device reported an error.
> + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or
> open a file for write
> + when the media is write-protected.
> + @retval EFI_ACCESS_DENIED The service denied access to the file.
> + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to
> open the file.
> + @retval EFI_VOLUME_FULL The volume is full.
> +**/
> +EFI_STATUS
> +EFIAPI
> +Ext4Open (
> + IN EFI_FILE_PROTOCOL *This,
> + OUT EFI_FILE_PROTOCOL **NewHandle,
> + IN CHAR16 *FileName,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + )
> +{
> + EFI_STATUS Status;
> + EXT4_FILE *FoundFile;
> + EXT4_FILE *Source;
> +
> + Source = (EXT4_FILE *)This;
> +
> + //
> + // Reset SymLoops counter
> + //
> + Source->Partition->Root->SymLoops = 0;
>
Keeping SymLoops in the root FILE seems kind of out of place. My idea here
(and this was going to be chained with a bunch of improvements to the path
walking logic, which is why I never got around to adding symlink support)
was to add a nameidata(see Onyx code, or NetBSD's namei(9) manpage)-like
structure, as you would in typical UNIX fashion. You would then do your
whole path traversal on that structure, which would keep your path walking
state, walked_symlinks, etc.
> +
> + Status = Ext4OpenInternal (
> + &FoundFile,
> + Source,
> + FileName,
> + OpenMode,
> + Attributes
> + );
> +
> + if (!EFI_ERROR (Status)) {
> + *NewHandle = &FoundFile->Protocol;
> + }
> +
> + return Status;
> +}
> +
> /**
> Closes a specified file handle.
>
> @@ -588,7 +893,7 @@ Ext4GetVolumeName (
>
> // s_volume_name is only valid on dynamic revision; old filesystems
> don't support this
> if (Partition->SuperBlock.s_rev_level == EXT4_DYNAMIC_REV) {
> - CopyMem (TempVolName, (CONST CHAR8
> *)Partition->SuperBlock.s_volume_name, 16);
> + CopyMem (TempVolName, Partition->SuperBlock.s_volume_name, 16);
> TempVolName[16] = '\0';
>
> Status = UTF8StrToUCS2 (TempVolName, &VolumeName);
> @@ -754,12 +1059,14 @@ Ext4GetInfo (
> OUT VOID *Buffer
> )
> {
> + EXT4_FILE *File;
> EXT4_PARTITION *Partition;
>
> - Partition = ((EXT4_FILE *)This)->Partition;
> + File = (EXT4_FILE *)This;
> + Partition = File->Partition;
>
> if (CompareGuid (InformationType, &gEfiFileInfoGuid)) {
> - return Ext4GetFileInfo ((EXT4_FILE *)This, Buffer, BufferSize);
> + return Ext4GetFileInfo (File, Buffer, BufferSize);
> }
>
> if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
> @@ -870,12 +1177,12 @@ Ext4SetInfo (
> )
> {
> EXT4_FILE *File;
> - EXT4_PARTITION *Part;
> + EXT4_PARTITION *Partition;
>
> - File = (EXT4_FILE *)This;
> - Part = File->Partition;
> + File = (EXT4_FILE *)This;
> + Partition = File->Partition;
>
> - if (Part->ReadOnly) {
> + if (Partition->ReadOnly) {
> return EFI_WRITE_PROTECTED;
> }
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 831f5946e870..e7a6b3225709 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -255,6 +255,59 @@ Ext4FileIsDir (
> return (File->Inode->i_mode & EXT4_INO_TYPE_DIR) == EXT4_INO_TYPE_DIR;
> }
>
> +/**
> + Checks if a file is a symlink.
> +
> + @param[in] File Pointer to the opened file.
> +
> + @return BOOLEAN Whether file is a symlink
> +**/
> +BOOLEAN
> +Ext4FileIsSymlink (
> + IN CONST EXT4_FILE *File
> + )
> +{
> + return (File->Inode->i_mode & EXT4_INO_TYPE_SYMLINK) ==
> EXT4_INO_TYPE_SYMLINK;
> +}
> +
> +/**
> + Detects if a symlink is a fast symlink.
> +
> + @param[in] File Pointer to the opened file.
> +
> + @return BOOLEAN Whether symlink is a fast symlink
> +**/
> +BOOLEAN
> +Ext4SymlinkIsFastSymlink (
> + IN CONST EXT4_FILE *File
> + )
> +{
> + //
> + // Detection logic of the fast-symlink splits into two behaviors - old
> and new.
> + // The old behavior is based on comparing the extended attribute blocks
> + // with the inode's i_blocks, and if it's zero we know the inode isn't
> storing
> + // the link in filesystem blocks, so we look to the inode->i_data.
> + // The new behavior is apparently needed only with the large EA inode
> feature.
> + // In this case we check that inode size less than maximum fast symlink
> size.
> + // So, we revert to the old behavior if the large EA inode feature is
> not set.
> + //
> + UINT32 FileAcl;
> + UINT32 ExtAttrBlocks;
> +
> + if ((File->Inode->i_flags & EXT4_EA_INODE_FL) == 0) {
> + FileAcl = File->Inode->i_file_acl;
> + if (EXT4_IS_64_BIT (File->Partition)) {
> + FileAcl |= LShiftU64
> (File->Inode->i_osd2.data_linux.l_i_file_acl_high, 32);
> + }
> +
> + ExtAttrBlocks = FileAcl != 0 ? (File->Partition->BlockSize >> 9) : 0;
> +
> + return File->Inode->i_blocks == ExtAttrBlocks;
> + }
> +
> + return EXT4_INODE_SIZE (File->Inode) <= EXT4_FAST_SYMLINK_MAX_SIZE;
> +}
> +
> /**
> Checks if a file is a regular file.
> @param[in] File Pointer to the opened file.
> --
> 2.37.1
>
>
--
Pedro Falcato
[-- Attachment #2: Type: text/html, Size: 34041 bytes --]
next prev parent reply other threads:[~2022-08-29 22:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-28 15:26 [edk2-platforms][PATCH v4 0/2] Ext4Pkg: Add Symbolic Links support Savva Mitrofanov
2022-07-28 15:26 ` [edk2-platforms][PATCH v4 1/2] Ext4Pkg: Add symbolic links support Savva Mitrofanov
2022-07-28 16:43 ` Marvin Häuser
2022-09-04 11:31 ` Savva Mitrofanov
2022-08-29 22:12 ` Pedro Falcato [this message]
2022-09-07 13:53 ` Savva Mitrofanov
2022-07-28 15:26 ` [edk2-platforms][PATCH v4 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE Savva Mitrofanov
2022-07-28 16:33 ` Marvin Häuser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKbZUD3xbgfPbqTZi7af5+sRD8WjDAc_Uij47e9GmxYzgbbSZQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox