* [edk2-platforms][PATCH 0/2] Ext4Pkg: Add Symbolic Links support @ 2022-07-20 6:17 Savva Mitrofanov 2022-07-20 6:17 ` [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Savva Mitrofanov 2022-07-20 6:17 ` [edk2-platforms][PATCH 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE Savva Mitrofanov 0 siblings, 2 replies; 5+ messages in thread From: Savva Mitrofanov @ 2022-07-20 6:17 UTC (permalink / raw) To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov Hi all, This patchset adds symbolic links support with simple recursion protection based on symbolic link nest level limitation, also I included patch which adds BASE_CR to extract EXT4_FILE private structure to prevent possible code corruption caused by structure changes and rearrangements in future. REF: https://github.com/savvamitrofanov/edk2-platforms/tree/ext4pkg_symlink_support Cc: Marvin Häuser <mhaeuser@posteo.de> Cc: Pedro Falcato <pedro.falcato@gmail.com> Cc: Vitaly Cheptsov <vit9696@protonmail.com> Savva Mitrofanov (2): Ext4Pkg: Add symbolic links support Ext4Pkg: Add base containing record macro for EXT4_FILE Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 2 +- Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 101 +++++- Features/Ext4Pkg/Ext4Dxe/File.c | 375 ++++++++++++++++++-- Features/Ext4Pkg/Ext4Dxe/Inode.c | 38 ++ 4 files changed, 477 insertions(+), 39 deletions(-) -- 2.37.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support 2022-07-20 6:17 [edk2-platforms][PATCH 0/2] Ext4Pkg: Add Symbolic Links support Savva Mitrofanov @ 2022-07-20 6:17 ` Savva Mitrofanov 2022-07-20 19:15 ` Marvin Häuser 2022-07-20 6:17 ` [edk2-platforms][PATCH 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE Savva Mitrofanov 1 sibling, 1 reply; 5+ messages in thread From: Savva Mitrofanov @ 2022-07-20 6:17 UTC (permalink / raw) To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov 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 | 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 +#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 OpenMode; UINT64 Position; + UINT32 Symloops; 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 + file. + @param[in] Source A pointer to the EXT4_FILE instance that is the file + handle to the source location. This would typically be an open + handle to a directory. + @param[in] FileName The Null-terminated string of the name of the file to be opened. + The file name may contain the following path modifiers: "\", ".", + and "..". + @param[in] OpenMode The mode to open the file. The only valid combinations that the + file may be opened with are: Read, Read/Write, or Create/Read/Write. + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the + attribute bits for the newly created file. + + @retval EFI_SUCCESS The file was opened. + @retval EFI_NOT_FOUND The specified file could not be found on the device. + @retval EFI_NO_MEDIA The device has no medium. + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no + longer supported. + @retval EFI_DEVICE_ERROR The device reported an error. + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write + when the media is write-protected. + @retval EFI_ACCESS_DENIED The service denied access to the file. + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. + @retval EFI_VOLUME_FULL The volume is full. + +**/ +EFI_STATUS +Ext4OpenInternal ( + OUT EXT4_FILE **FoundFile, + IN EXT4_FILE *Source, + IN CHAR16 *FileName, + IN UINT64 OpenMode, + IN UINT64 Attributes + ); + /** Closes a file. @@ -774,6 +817,28 @@ Ext4FileIsDir ( IN CONST EXT4_FILE *File ); +/** + 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 + ); + +/** + Detects if a symlink is a fast symlink. + @param[in] File Pointer to the opened file. + + @return TRUE if file is a fast symlink. +**/ +BOOLEAN +Ext4SymlinkIsFastSymlink ( + IN CONST EXT4_FILE *File + ); + /** Checks if a file is a regular file. @param[in] File Pointer to the opened file. @@ -797,7 +862,7 @@ Ext4FileIsReg ( it's a regular file or a directory, since most other file types don't make sense under UEFI. **/ -#define Ext4FileIsOpenable(File) (Ext4FileIsReg(File) || Ext4FileIsDir(File)) +#define Ext4FileIsOpenable(File) (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File)) #define EXT4_INODE_HAS_FIELD(Inode, Field) \ (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >= \ @@ -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) == 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 = AllocatePool (EXT4_FAST_SYMLINK_SIZE + 1); + if (AsciiSymlinkTmp == NULL) { + Status = EFI_OUT_OF_RESOURCES; + 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] = '\0'; + + *AsciiSymlink = AsciiSymlinkTmp; + *AsciiSymlinkSize = 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 = EXT4_INODE_SIZE (File->Inode); + + // + // Allocate EXT4_INODE_SIZE + 1 + // + if (SymlinkSizeTmp <= EFI_PATH_MAX - 1) { + SymlinkAllocateSize = (UINT32)SymlinkSizeTmp + 1; + } else { + DEBUG (( + DEBUG_FS, + "[ext4] Error! Symlink path maximum length was hit!\n" + )); + return EFI_INVALID_PARAMETER; + } + + 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)) { + File->Position += 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] = '\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 == AsciiStrSize (SymlinkTmp)); + // + + if (SymlinkSizeTmp != ReadSize) { + DEBUG (( + DEBUG_FS, + "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n" + )); + return EFI_VOLUME_CORRUPTED; + } + + *AsciiSymlinkSize = SymlinkAllocateSize; + *AsciiSymlink = SymlinkTmp; + + return EFI_SUCCESS; +} + +/** + Reads a symlink file. + + @param[in] Partition Pointer to the ext4 partition. + @param[in] File Pointer to the open symlink file. + @param[out] Symlink Pointer to the output unicode symlink string. + + @retval EFI_SUCCESS Symlink was read. + @retval EFI_ACCESS_DENIED Symlink is encrypted. + @retval EFI_OUT_OF_RESOURCES Memory allocation error. + @retval EFI_INVALID_PARAMETER Symlink path has incorrect length + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode value +**/ +EFI_STATUS +Ext4ReadSymlink ( + IN EXT4_PARTITION *Partition, + IN EXT4_FILE *File, + OUT CHAR16 **Symlink + ) +{ + EFI_STATUS Status; + CHAR8 *SymlinkTmp; + UINT32 SymlinkSize; + CHAR16 *Symlink16Tmp; + CHAR16 *Needle; + + // + // Assume that we alread read Inode via Ext4ReadInode + // Skip reading, just check encryption flag + // + if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) { + Status = EFI_ACCESS_DENIED; + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n")); + return Status; + } + + 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) { + Status = EFI_OUT_OF_RESOURCES; + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n")); + FreePool (SymlinkTmp); + return Status; + } + + 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 + // + Needle = Symlink16Tmp; + while ((Needle = StrStr (Needle, L"/")) != NULL) { + *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 +381,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 +395,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 +410,11 @@ Ext4Open ( } while (FileName[0] != L'\0') { + if (Partition->Root->Symloops > SYMLOOP_MAX) { + DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n")); + return EFI_ACCESS_DENIED; + } + // Discard leading path separators while (FileName[0] == L'\\') { FileName++; @@ -238,18 +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 = 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 +521,75 @@ Ext4Open ( return EFI_ACCESS_DENIED; } - *NewHandle = &Current->Protocol; + *FoundFile = Current; DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name)); return EFI_SUCCESS; } +/** + Opens a new file relative to the source file's location. + @param[in] This A pointer to the EFI_FILE_PROTOCOL instance that is the file + handle to the source location. This would typically be an open + handle to a directory. + @param[out] NewHandle A pointer to the location to return the opened handle for the new + file. + @param[in] FileName The Null-terminated string of the name of the file to be opened. + The file name may contain the following path modifiers: "\", ".", + and "..". + @param[in] OpenMode The mode to open the file. The only valid combinations that the + file may be opened with are: Read, Read/Write, or Create/Read/Write. + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the + attribute bits for the newly created file. + @retval EFI_SUCCESS The file was opened. + @retval EFI_NOT_FOUND The specified file could not be found on the device. + @retval EFI_NO_MEDIA The device has no medium. + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no + longer supported. + @retval EFI_DEVICE_ERROR The device reported an error. + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write + when the media is write-protected. + @retval EFI_ACCESS_DENIED The service denied access to the file. + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. + @retval EFI_VOLUME_FULL The volume is full. +**/ +EFI_STATUS +EFIAPI +Ext4Open ( + IN EFI_FILE_PROTOCOL *This, + OUT EFI_FILE_PROTOCOL **NewHandle, + IN CHAR16 *FileName, + IN UINT64 OpenMode, + IN UINT64 Attributes + ) +{ + EFI_STATUS Status; + EXT4_FILE *FoundFile; + EXT4_FILE *Source; + + Source = (EXT4_FILE *)This; + + // + // Reset Symloops counter + // + Source->Partition->Root->Symloops = 0; + + Status = Ext4OpenInternal ( + &FoundFile, + Source, + FileName, + OpenMode, + Attributes + ); + + if (!EFI_ERROR (Status)) { + *NewHandle = &FoundFile->Protocol; + } + + return Status; +} + /** Closes a specified file handle. @@ -588,7 +899,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 +1065,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 +1183,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..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) == 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) == 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 + // 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 = File->Inode->i_file_acl ? (File->Partition->BlockSize >> 9) : 0; + + return ( File->Inode->i_blocks == ExtAttrBlocks + && EXT4_INODE_SIZE (File->Inode) <= 60 + ); +} + /** Checks if a file is a regular file. @param[in] File Pointer to the opened file. -- 2.37.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support 2022-07-20 6:17 ` [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Savva Mitrofanov @ 2022-07-20 19:15 ` Marvin Häuser 2022-07-24 9:57 ` Savva Mitrofanov 0 siblings, 1 reply; 5+ messages in thread From: Marvin Häuser @ 2022-07-20 19:15 UTC (permalink / raw) To: Savva Mitrofanov; +Cc: edk2-devel-groups-io, Pedro Falcato, Vitaly Cheptsov [-- Attachment #1: Type: text/plain, Size: 26534 bytes --] > 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ä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 | 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 > +#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 OpenMode; > UINT64 Position; > + UINT32 Symloops; I think the code style demands "L" is capitalised. > > EXT4_PARTITION *Partition; > > @@ -497,6 +501,45 @@ Ext4SetupFile ( > IN EXT4_PARTITION *Partition > ); > > +/** > + Opens a new file relative to the source file's location. > + > + @param[out] FoundFile A pointer to the location to return the opened handle for the new > + file. > + @param[in] Source A pointer to the EXT4_FILE instance that is the file > + handle to the source location. This would typically be an open > + handle to a directory. > + @param[in] FileName The Null-terminated string of the name of the file to be opened. > + The file name may contain the following path modifiers: "\", ".", > + and "..". > + @param[in] OpenMode The mode to open the file. The only valid combinations that the > + file may be opened with are: Read, Read/Write, or Create/Read/Write. > + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the > + attribute bits for the newly created file. > + > + @retval EFI_SUCCESS The file was opened. > + @retval EFI_NOT_FOUND The specified file could not be found on the device. > + @retval EFI_NO_MEDIA The device has no medium. > + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no > + longer supported. > + @retval EFI_DEVICE_ERROR The device reported an error. > + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. > + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write > + when the media is write-protected. > + @retval EFI_ACCESS_DENIED The service denied access to the file. > + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. > + @retval EFI_VOLUME_FULL The volume is full. > + > +**/ > +EFI_STATUS > +Ext4OpenInternal ( > + OUT EXT4_FILE **FoundFile, > + IN EXT4_FILE *Source, > + IN CHAR16 *FileName, > + IN UINT64 OpenMode, > + IN UINT64 Attributes > + ); > + > /** > Closes a file. > > @@ -774,6 +817,28 @@ Ext4FileIsDir ( > IN CONST EXT4_FILE *File > ); > > +/** > + Checks if a file is a symlink. <line break> > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file is a symlink. I think you can use "@returns Whether file is a symlink", because strictly speaking you would require a "FALSE" case with this syntax. > +**/ > +BOOLEAN > +Ext4FileIsSymlink ( > + IN CONST EXT4_FILE *File > + ); > + > +/** > + Detects if a symlink is a fast symlink. <line break> > + @param[in] File Pointer to the opened file. > + > + @return TRUE if file is a fast symlink. > +**/ > +BOOLEAN > +Ext4SymlinkIsFastSymlink ( > + IN CONST EXT4_FILE *File > + ); > + > /** > Checks if a file is a regular file. > @param[in] File Pointer to the opened file. > @@ -797,7 +862,7 @@ Ext4FileIsReg ( > it's a regular file or a directory, since most other file types > don't make sense under UEFI. > **/ > -#define Ext4FileIsOpenable(File) (Ext4FileIsReg(File) || Ext4FileIsDir(File)) > +#define Ext4FileIsOpenable(File) (Ext4FileIsReg (File) || Ext4FileIsDir (File) || Ext4FileIsSymlink (File)) > > #define EXT4_INODE_HAS_FIELD(Inode, Field) \ > (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >= \ > @@ -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) == 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 = AllocatePool (EXT4_FAST_SYMLINK_SIZE + 1); Hmm, why do we use constant sizes instead of the inode size? The size constant seems to imply fast links have a fixed size of 60 Bytes, but this is not true according to the spec, which says "less than 60 Bytes" (i.e., one more for the 0-terminator). > + if (AsciiSymlinkTmp == NULL) { > + Status = 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] = '\0'; > + > + *AsciiSymlink = AsciiSymlinkTmp; > + *AsciiSymlinkSize = 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 = EXT4_INODE_SIZE (File->Inode); > + > + // > + // Allocate EXT4_INODE_SIZE + 1 > + // > + if (SymlinkSizeTmp <= EFI_PATH_MAX - 1) { > + SymlinkAllocateSize = (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 = 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)) { > + File->Position += 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] = '\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 == 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 != ReadSize) { > + DEBUG (( > + DEBUG_FS, > + "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n" > + )); > + return EFI_VOLUME_CORRUPTED; > + } > + > + *AsciiSymlinkSize = SymlinkAllocateSize; > + *AsciiSymlink = SymlinkTmp; > + > + return EFI_SUCCESS; > +} > + > +/** > + Reads a symlink file. > + > + @param[in] Partition Pointer to the ext4 partition. > + @param[in] File Pointer to the open symlink file. > + @param[out] Symlink Pointer to the output unicode symlink string. > + > + @retval EFI_SUCCESS Symlink was read. > + @retval EFI_ACCESS_DENIED Symlink is encrypted. > + @retval EFI_OUT_OF_RESOURCES Memory allocation error. > + @retval EFI_INVALID_PARAMETER Symlink path has incorrect length > + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode value > +**/ > +EFI_STATUS > +Ext4ReadSymlink ( > + IN EXT4_PARTITION *Partition, > + IN EXT4_FILE *File, > + OUT CHAR16 **Symlink Capitalised "L". > + ) > +{ > + EFI_STATUS Status; > + CHAR8 *SymlinkTmp; > + UINT32 SymlinkSize; > + CHAR16 *Symlink16Tmp; > + CHAR16 *Needle; > + > + // > + // Assume that we alread read Inode via Ext4ReadInode > + // Skip reading, just check encryption flag > + // > + if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) { > + Status = EFI_ACCESS_DENIED; > + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n")); > + return Status; > + } > + > + 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) { > + Status = EFI_OUT_OF_RESOURCES; > + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n")); > + FreePool (SymlinkTmp); > + return Status; > + } > + > + 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 > + // > + Needle = Symlink16Tmp; > + while ((Needle = StrStr (Needle, L"/")) != NULL) { > + *Needle++ = L'\\'; > + } I'm actually wondering whether a regular loop is not both easier and faster, as this is a 1-character search. > + > + *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 +381,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 +395,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 +410,11 @@ Ext4Open ( > } > > while (FileName[0] != L'\0') { > + if (Partition->Root->Symloops > SYMLOOP_MAX) { > + DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n")); > + return EFI_ACCESS_DENIED; > + } > + > // Discard leading path separators > while (FileName[0] == L'\\') { > FileName++; > @@ -238,18 +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 = 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 +521,75 @@ Ext4Open ( > return EFI_ACCESS_DENIED; > } > > - *NewHandle = &Current->Protocol; > + *FoundFile = Current; > > DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name)); > return EFI_SUCCESS; > } > > +/** > + Opens a new file relative to the source file's location. > + @param[in] This A pointer to the EFI_FILE_PROTOCOL instance that is the file > + handle to the source location. This would typically be an open > + handle to a directory. > + @param[out] NewHandle A pointer to the location to return the opened handle for the new > + file. > + @param[in] FileName The Null-terminated string of the name of the file to be opened. > + The file name may contain the following path modifiers: "\", ".", > + and "..". > + @param[in] OpenMode The mode to open the file. The only valid combinations that the > + file may be opened with are: Read, Read/Write, or Create/Read/Write. > + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the > + attribute bits for the newly created file. > + @retval EFI_SUCCESS The file was opened. > + @retval EFI_NOT_FOUND The specified file could not be found on the device. > + @retval EFI_NO_MEDIA The device has no medium. > + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no > + longer supported. > + @retval EFI_DEVICE_ERROR The device reported an error. > + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. > + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write > + when the media is write-protected. > + @retval EFI_ACCESS_DENIED The service denied access to the file. > + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. > + @retval EFI_VOLUME_FULL The volume is full. > +**/ > +EFI_STATUS > +EFIAPI > +Ext4Open ( > + IN EFI_FILE_PROTOCOL *This, > + OUT EFI_FILE_PROTOCOL **NewHandle, > + IN CHAR16 *FileName, > + IN UINT64 OpenMode, > + IN UINT64 Attributes > + ) > +{ > + EFI_STATUS Status; > + EXT4_FILE *FoundFile; > + EXT4_FILE *Source; > + > + Source = (EXT4_FILE *)This; > + > + // > + // Reset Symloops counter > + // > + Source->Partition->Root->Symloops = 0; > + > + Status = Ext4OpenInternal ( > + &FoundFile, > + Source, > + FileName, > + OpenMode, > + Attributes > + ); > + > + if (!EFI_ERROR (Status)) { > + *NewHandle = &FoundFile->Protocol; > + } > + > + return Status; > +} > + > /** > Closes a specified file handle. > > @@ -588,7 +899,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 +1065,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 +1183,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..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) == 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) == 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 <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 = File->Inode->i_file_acl ? (File->Partition->BlockSize >> 9) : 0; As per the code style requirements, declarations and definitions must be separate for local variables. i_file_acl has a "_hi" variant in EXT4 you did not account for. Cc Pedro. > + > + return ( File->Inode->i_blocks == ExtAttrBlocks > + && EXT4_INODE_SIZE (File->Inode) <= 60 Is this "60" not "EXT4_FAST_SYMLINK_SIZE"? Maybe we should rather call it "EXT4_FAST_SYMLINK_MAX_SIZE"? Best regards, Marvin > + ); > +} > + > /** > Checks if a file is a regular file. > @param[in] File Pointer to the opened file. > -- > 2.37.0 > [-- Attachment #2: Type: text/html, Size: 48998 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support 2022-07-20 19:15 ` Marvin Häuser @ 2022-07-24 9:57 ` Savva Mitrofanov 0 siblings, 0 replies; 5+ messages in thread From: Savva Mitrofanov @ 2022-07-24 9:57 UTC (permalink / raw) To: Marvin Häuser; +Cc: Pedro Falcato, vit9696, devel [-- Attachment #1.1: Type: text/plain, Size: 31814 bytes --] Hello, Marvin! Thank you for your time and this code-review > On 21 Jul 2022, at 01:15, Marvin Häuser <mhaeuser@posteo.de> wrote: > > > >> On 20. Jul 2022, at 08:17, Savva Mitrofanov <savvamtr@gmail.com <mailto: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äuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> >> Cc: Pedro Falcato <pedro.falcato@gmail.com <mailto:pedro.falcato@gmail.com>> >> Cc: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@protonmail.com>> >> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com <mailto: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 >> +#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 OpenMode; >> UINT64 Position; >> + UINT32 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 >> + file. >> + @param[in] Source A pointer to the EXT4_FILE instance that is the file >> + handle to the source location. This would typically be an open >> + handle to a directory. >> + @param[in] FileName The Null-terminated string of the name of the file to be opened. >> + The file name may contain the following path modifiers: "\", ".", >> + and "..". >> + @param[in] OpenMode The mode to open the file. The only valid combinations that the >> + file may be opened with are: Read, Read/Write, or Create/Read/Write. >> + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the >> + attribute bits for the newly created file. >> + >> + @retval EFI_SUCCESS The file was opened. >> + @retval EFI_NOT_FOUND The specified file could not be found on the device. >> + @retval EFI_NO_MEDIA The device has no medium. >> + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no >> + longer supported. >> + @retval EFI_DEVICE_ERROR The device reported an error. >> + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. >> + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write >> + when the media is write-protected. >> + @retval EFI_ACCESS_DENIED The service denied access to the file. >> + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. >> + @retval EFI_VOLUME_FULL The volume is full. >> + >> +**/ >> +EFI_STATUS >> +Ext4OpenInternal ( >> + OUT EXT4_FILE **FoundFile, >> + IN EXT4_FILE *Source, >> + IN CHAR16 *FileName, >> + IN UINT64 OpenMode, >> + IN UINT64 Attributes >> + ); >> + >> /** >> Closes a file. >> >> @@ -774,6 +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) \ >> (Inode->i_extra_isize + EXT4_GOOD_OLD_INODE_SIZE >= \ >> @@ -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) == 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 = 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 Ext4ReadFastSymlink ( IN EXT4_PARTITION *Partition, IN EXT4_FILE *File, OUT CHAR8 **AsciiSymlink, OUT UINT32 *AsciiSymlinkSize ) { UINT32 SymlinkSize; CHAR8 *AsciiSymlinkTmp; SymlinkSize = EXT4_INODE_SIZE (File->Inode); ASSERT (SymlinkSize <= EXT4_FAST_SYMLINK_MAX_SIZE); AsciiSymlinkTmp = AllocatePool (SymlinkSize + 1); if (AsciiSymlinkTmp == NULL) { DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink ascii string buffer\n")); return EFI_OUT_OF_RESOURCES; } CopyMem (AsciiSymlinkTmp, File->Inode->i_data, SymlinkSize); // // Add null-terminator // AsciiSymlinkTmp[SymlinkSize] = '\0'; *AsciiSymlink = AsciiSymlinkTmp; *AsciiSymlinkSize = SymlinkSize + 1; return EFI_SUCCESS; } >> + if (AsciiSymlinkTmp == NULL) { >> + Status = 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] = '\0'; >> + >> + *AsciiSymlink = AsciiSymlinkTmp; >> + *AsciiSymlinkSize = 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 = EXT4_INODE_SIZE (File->Inode); >> + >> + // >> + // Allocate EXT4_INODE_SIZE + 1 >> + // >> + if (SymlinkSizeTmp <= EFI_PATH_MAX - 1) { >> + SymlinkAllocateSize = (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 = 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)) { >> + File->Position += 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] = '\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 == 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 != ReadSize) { >> + DEBUG (( >> + DEBUG_FS, >> + "[ext4] Error! The sz of the read block doesn't match the value from the inode!\n" >> + )); >> + return EFI_VOLUME_CORRUPTED; >> + } >> + >> + *AsciiSymlinkSize = SymlinkAllocateSize; >> + *AsciiSymlink = SymlinkTmp; >> + >> + return EFI_SUCCESS; >> +} >> + >> +/** >> + Reads a symlink file. >> + >> + @param[in] Partition Pointer to the ext4 partition. >> + @param[in] File Pointer to the open symlink file. >> + @param[out] Symlink Pointer to the output unicode symlink string. >> + >> + @retval EFI_SUCCESS Symlink was read. >> + @retval EFI_ACCESS_DENIED Symlink is encrypted. >> + @retval EFI_OUT_OF_RESOURCES Memory allocation error. >> + @retval EFI_INVALID_PARAMETER Symlink path has incorrect length >> + @retval EFI_VOLUME_CORRUPTED Symlink read block size differ from inode value >> +**/ >> +EFI_STATUS >> +Ext4ReadSymlink ( >> + IN EXT4_PARTITION *Partition, >> + IN EXT4_FILE *File, >> + OUT CHAR16 **Symlink > > 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) != 0) { >> + Status = EFI_ACCESS_DENIED; >> + DEBUG ((DEBUG_FS, "[ext4] Error, symlink is encrypted\n")); >> + return Status; >> + } >> + >> + 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) { >> + Status = EFI_OUT_OF_RESOURCES; >> + DEBUG ((DEBUG_FS, "[ext4] Failed to allocate symlink unicode string buffer\n")); >> + FreePool (SymlinkTmp); >> + return Status; >> + } >> + >> + 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 >> + // >> + Needle = Symlink16Tmp; >> + while ((Needle = StrStr (Needle, L"/")) != NULL) { >> + *Needle++ = 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 = 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 +381,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 +395,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 +410,11 @@ Ext4Open ( >> } >> >> while (FileName[0] != L'\0') { >> + if (Partition->Root->Symloops > SYMLOOP_MAX) { >> + DEBUG ((DEBUG_FS, "[ext4] Symloop limit is hit !\n")); >> + return EFI_ACCESS_DENIED; >> + } >> + >> // Discard leading path separators >> while (FileName[0] == L'\\') { >> FileName++; >> @@ -238,18 +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 = 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 +521,75 @@ Ext4Open ( >> return EFI_ACCESS_DENIED; >> } >> >> - *NewHandle = &Current->Protocol; >> + *FoundFile = Current; >> >> DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name)); >> return EFI_SUCCESS; >> } >> >> +/** >> + Opens a new file relative to the source file's location. >> + @param[in] This A pointer to the EFI_FILE_PROTOCOL instance that is the file >> + handle to the source location. This would typically be an open >> + handle to a directory. >> + @param[out] NewHandle A pointer to the location to return the opened handle for the new >> + file. >> + @param[in] FileName The Null-terminated string of the name of the file to be opened. >> + The file name may contain the following path modifiers: "\", ".", >> + and "..". >> + @param[in] OpenMode The mode to open the file. The only valid combinations that the >> + file may be opened with are: Read, Read/Write, or Create/Read/Write. >> + @param[in] Attributes Only valid for EFI_FILE_MODE_CREATE, in which case these are the >> + attribute bits for the newly created file. >> + @retval EFI_SUCCESS The file was opened. >> + @retval EFI_NOT_FOUND The specified file could not be found on the device. >> + @retval EFI_NO_MEDIA The device has no medium. >> + @retval EFI_MEDIA_CHANGED The device has a different medium in it or the medium is no >> + longer supported. >> + @retval EFI_DEVICE_ERROR The device reported an error. >> + @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. >> + @retval EFI_WRITE_PROTECTED An attempt was made to create a file, or open a file for write >> + when the media is write-protected. >> + @retval EFI_ACCESS_DENIED The service denied access to the file. >> + @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the file. >> + @retval EFI_VOLUME_FULL The volume is full. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +Ext4Open ( >> + IN EFI_FILE_PROTOCOL *This, >> + OUT EFI_FILE_PROTOCOL **NewHandle, >> + IN CHAR16 *FileName, >> + IN UINT64 OpenMode, >> + IN UINT64 Attributes >> + ) >> +{ >> + EFI_STATUS Status; >> + EXT4_FILE *FoundFile; >> + EXT4_FILE *Source; >> + >> + Source = (EXT4_FILE *)This; >> + >> + // >> + // Reset Symloops counter >> + // >> + Source->Partition->Root->Symloops = 0; >> + >> + Status = Ext4OpenInternal ( >> + &FoundFile, >> + Source, >> + FileName, >> + OpenMode, >> + Attributes >> + ); >> + >> + if (!EFI_ERROR (Status)) { >> + *NewHandle = &FoundFile->Protocol; >> + } >> + >> + return Status; >> +} >> + >> /** >> Closes a specified file handle. >> >> @@ -588,7 +899,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 +1065,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 +1183,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..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) == 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) == 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 <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 <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 = 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: 1) https://github.com/torvalds/linux/commit/407cd7fb83c0ebabb490190e673d8c71ee7df97e <https://github.com/torvalds/linux/commit/407cd7fb83c0ebabb490190e673d8c71ee7df97e> 2) https://github.com/torvalds/linux/commit/fc82228a5e3860502dbf3bfa4a9570cb7093cf7f <https://github.com/torvalds/linux/commit/fc82228a5e3860502dbf3bfa4a9570cb7093cf7f> 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 = 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; } >> + >> + return ( File->Inode->i_blocks == ExtAttrBlocks >> + && EXT4_INODE_SIZE (File->Inode) <= 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 [-- Attachment #1.2: Type: text/html, Size: 57859 bytes --] [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [edk2-platforms][PATCH 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE 2022-07-20 6:17 [edk2-platforms][PATCH 0/2] Ext4Pkg: Add Symbolic Links support Savva Mitrofanov 2022-07-20 6:17 ` [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Savva Mitrofanov @ 2022-07-20 6:17 ` Savva Mitrofanov 1 sibling, 0 replies; 5+ messages in thread From: Savva Mitrofanov @ 2022-07-20 6:17 UTC (permalink / raw) To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov We shouldn't use direct casts, because in the future it could break the code, so using BASE_CR would be safe against possible structure changes and rearrangements 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/Ext4Dxe.h | 2 ++ Features/Ext4Pkg/Ext4Dxe/File.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h index a1eb32aa2cff..3ff7b29e7133 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h @@ -383,6 +383,8 @@ struct _Ext4File { EXT4_DENTRY *Dentry; }; +#define EXT4_FILE_FROM_THIS(This) BASE_CR ((This), EXT4_FILE, Protocol) + #define EXT4_FILE_FROM_OPEN_FILES_NODE(Node) \ BASE_CR(Node, EXT4_FILE, OpenFilesListNode) diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c index 0fb9e05e6647..750397010443 100644 --- a/Features/Ext4Pkg/Ext4Dxe/File.c +++ b/Features/Ext4Pkg/Ext4Dxe/File.c @@ -568,7 +568,7 @@ Ext4Open ( EXT4_FILE *FoundFile; EXT4_FILE *Source; - Source = (EXT4_FILE *)This; + Source = EXT4_FILE_FROM_THIS (This); // // Reset Symloops counter @@ -605,7 +605,7 @@ Ext4Close ( IN EFI_FILE_PROTOCOL *This ) { - return Ext4CloseInternal ((EXT4_FILE *)This); + return Ext4CloseInternal (EXT4_FILE_FROM_THIS (This)); } /** @@ -686,7 +686,7 @@ Ext4ReadFile ( EXT4_PARTITION *Partition; EFI_STATUS Status; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); Partition = File->Partition; ASSERT (Ext4FileIsOpenable (File)); @@ -737,7 +737,7 @@ Ext4WriteFile ( { EXT4_FILE *File; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); if (!(File->OpenMode & EFI_FILE_MODE_WRITE)) { return EFI_ACCESS_DENIED; @@ -767,7 +767,7 @@ Ext4GetPosition ( { EXT4_FILE *File; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); if (Ext4FileIsDir (File)) { return EFI_UNSUPPORTED; @@ -800,7 +800,7 @@ Ext4SetPosition ( { EXT4_FILE *File; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); // Only seeks to 0 (so it resets the ReadDir operation) are allowed if (Ext4FileIsDir (File) && (Position != 0)) { @@ -1068,7 +1068,7 @@ Ext4GetInfo ( EXT4_FILE *File; EXT4_PARTITION *Partition; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); Partition = File->Partition; if (CompareGuid (InformationType, &gEfiFileInfoGuid)) { @@ -1185,7 +1185,7 @@ Ext4SetInfo ( EXT4_FILE *File; EXT4_PARTITION *Partition; - File = (EXT4_FILE *)This; + File = EXT4_FILE_FROM_THIS (This); Partition = File->Partition; if (Partition->ReadOnly) { -- 2.37.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-24 9:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-20 6:17 [edk2-platforms][PATCH 0/2] Ext4Pkg: Add Symbolic Links support Savva Mitrofanov 2022-07-20 6:17 ` [edk2-platforms][PATCH 1/2] Ext4Pkg: Add symbolic links support Savva Mitrofanov 2022-07-20 19:15 ` Marvin Häuser 2022-07-24 9:57 ` Savva Mitrofanov 2022-07-20 6:17 ` [edk2-platforms][PATCH 2/2] Ext4Pkg: Add base containing record macro for EXT4_FILE Savva Mitrofanov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox