From 05702eddbdb7857a9f2a1a372344c037c79a9ec7 Mon Sep 17 00:00:00 2001 From: Mikhail Krichanov Date: Fri, 28 Apr 2023 10:09:44 +0300 Subject: [PATCH] Ext4Pkg: Various improvements based on Sydr fuzzing results. Signed-off-by: Savva Mitrofanov --- Ext4Pkg/Ext4Dxe/BlockGroup.c | 5 ++ Ext4Pkg/Ext4Dxe/BlockMap.c | 6 +-- Ext4Pkg/Ext4Dxe/Directory.c | 97 +++++++++++++++++++++++++----------- Ext4Pkg/Ext4Dxe/DiskUtil.c | 18 +++++-- Ext4Pkg/Ext4Dxe/Ext4Disk.h | 34 +++++++++++-- Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 51 +++++++++++++------ Ext4Pkg/Ext4Dxe/Extents.c | 20 ++++++-- Ext4Pkg/Ext4Dxe/File.c | 25 +++++++--- Ext4Pkg/Ext4Dxe/Inode.c | 8 +-- Ext4Pkg/Ext4Dxe/Superblock.c | 22 +++++--- Ext4Pkg/Ext4Dxe/Symlink.c | 16 +++--- 11 files changed, 217 insertions(+), 85 deletions(-) diff --git a/Ext4Pkg/Ext4Dxe/BlockGroup.c b/Ext4Pkg/Ext4Dxe/BlockGroup.c index cba96cd95a..f34cdc5dba 100644 --- a/Ext4Pkg/Ext4Dxe/BlockGroup.c +++ b/Ext4Pkg/Ext4Dxe/BlockGroup.c @@ -50,6 +50,11 @@ Ext4ReadInode ( EXT4_BLOCK_NR InodeTableStart; EFI_STATUS Status; + if (!EXT4_IS_VALID_INODE_NR (Partition, InodeNum)) { + DEBUG ((DEBUG_ERROR, "[ext4] Error reading inode: inode number %lu isn't valid\n", InodeNum)); + return EFI_VOLUME_CORRUPTED; + } + BlockGroupNumber = (UINT32)DivU64x64Remainder ( InodeNum - 1, Partition->SuperBlock.s_inodes_per_group, diff --git a/Ext4Pkg/Ext4Dxe/BlockMap.c b/Ext4Pkg/Ext4Dxe/BlockMap.c index 2bc629fe9d..8fee4476a5 100644 --- a/Ext4Pkg/Ext4Dxe/BlockMap.c +++ b/Ext4Pkg/Ext4Dxe/BlockMap.c @@ -131,7 +131,7 @@ Ext4GetBlockPath ( /** @brief Get an extent from a block map - Note: Also parses file holes and creates uninitialised extents from them. + Note: Also parses file holes and creates uninitialized extents from them. @param[in] Buffer Buffer of block pointers @param[in] IndEntries Number of entries in this block pointer table @@ -173,7 +173,7 @@ Ext4GetExtentInBlockMap ( } } - // We mark the extent as uninitialised, although there's a difference between uninit + // We mark the extent as uninitialized, although there's a difference between uninit // extents and file holes. Extent->ee_len = EXT4_EXTENT_MAX_INITIALIZED + Count; return; @@ -206,7 +206,7 @@ Ext4GetExtentInBlockMap ( @param[in] LogicalBlock Block number which the returned extent must cover. @param[out] Extent Pointer to the output buffer, where the extent will be copied to. - @retval EFI_SUCCESS Retrieval was succesful. + @retval EFI_SUCCESS Retrieval was successful. @retval EFI_NO_MAPPING Block has no mapping. **/ EFI_STATUS diff --git a/Ext4Pkg/Ext4Dxe/Directory.c b/Ext4Pkg/Ext4Dxe/Directory.c index 4441e6d192..88f89a4053 100644 --- a/Ext4Pkg/Ext4Dxe/Directory.c +++ b/Ext4Pkg/Ext4Dxe/Directory.c @@ -1,7 +1,7 @@ /** @file Directory related routines - Copyright (c) 2021 Pedro Falcato All rights reserved. + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -16,8 +16,9 @@ @param[in] Entry Pointer to a EXT4_DIR_ENTRY. @param[out] Ucs2FileName Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1. - @retval EFI_SUCCESS The filename was succesfully retrieved and converted to UCS2. - @retval !EFI_SUCCESS Failure. + @retval EFI_SUCCESS The filename was successfully retrieved and converted to UCS2. + @retval EFI_INVALID_PARAMETER The filename is not valid UTF-8. + @retval !EFI_SUCCESS Failure. **/ EFI_STATUS Ext4GetUcs2DirentName ( @@ -27,9 +28,16 @@ Ext4GetUcs2DirentName ( { CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1]; UINT16 *Str; + UINT8 Index; EFI_STATUS Status; - CopyMem (Utf8NameBuf, Entry->name, Entry->name_len); + for (Index = 0; Index < Entry->name_len; ++Index) { + if (Entry->name[Index] == '\0') { + return EFI_INVALID_PARAMETER; + } + + Utf8NameBuf[Index] = Entry->name[Index]; + } Utf8NameBuf[Entry->name_len] = '\0'; @@ -112,8 +120,7 @@ Ext4RetrieveDirent ( UINTN ToCopy; UINTN BlockOffset; - Status = EFI_NOT_FOUND; - Buf = AllocatePool (Partition->BlockSize); + Buf = AllocatePool (Partition->BlockSize); if (Buf == NULL) { return EFI_OUT_OF_RESOURCES; @@ -127,7 +134,8 @@ Ext4RetrieveDirent ( DivU64x32Remainder (DirInoSize, Partition->BlockSize, &BlockRemainder); if (BlockRemainder != 0) { // Directory inodes need to have block aligned sizes - return EFI_VOLUME_CORRUPTED; + Status = EFI_VOLUME_CORRUPTED; + goto Out; } while (Off < DirInoSize) { @@ -136,8 +144,7 @@ Ext4RetrieveDirent ( Status = Ext4Read (Partition, Directory, Buf, Off, &Length); if (Status != EFI_SUCCESS) { - FreePool (Buf); - return Status; + goto Out; } for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) { @@ -145,19 +152,19 @@ Ext4RetrieveDirent ( RemainingBlock = Partition->BlockSize - BlockOffset; // Check if the minimum directory entry fits inside [BlockOffset, EndOfBlock] if (RemainingBlock < EXT4_MIN_DIR_ENTRY_LEN) { - FreePool (Buf); - return EFI_VOLUME_CORRUPTED; + Status = EFI_VOLUME_CORRUPTED; + goto Out; } if (!Ext4ValidDirent (Entry)) { - FreePool (Buf); - return EFI_VOLUME_CORRUPTED; + Status = EFI_VOLUME_CORRUPTED; + goto Out; } if ((Entry->name_len > RemainingBlock) || (Entry->rec_len > RemainingBlock)) { // Corrupted filesystem - FreePool (Buf); - return EFI_VOLUME_CORRUPTED; + Status = EFI_VOLUME_CORRUPTED; + goto Out; } // Unused entry @@ -174,10 +181,16 @@ Ext4RetrieveDirent ( * need to form valid ASCII/UTF-8 sequences. */ if (EFI_ERROR (Status)) { - // If we error out, skip this entry - // I'm not sure if this is correct behaviour, but I don't think there's a precedent here. - BlockOffset += Entry->rec_len; - continue; + if (Status == EFI_INVALID_PARAMETER) { + // If we error out due to a bad UTF-8 sequence (see Ext4GetUcs2DirentName), skip this entry. + // I'm not sure if this is correct behaviour, but I don't think there's a precedent here. + BlockOffset += Entry->rec_len; + continue; + } + + // Other sorts of errors should just error out. + FreePool (Buf); + return Status; } if ((Entry->name_len == StrLen (Name)) && @@ -186,8 +199,8 @@ Ext4RetrieveDirent ( ToCopy = MIN (Entry->rec_len, sizeof (EXT4_DIR_ENTRY)); CopyMem (Result, Entry, ToCopy); - FreePool (Buf); - return EFI_SUCCESS; + Status = EFI_SUCCESS; + goto Out; } BlockOffset += Entry->rec_len; @@ -196,8 +209,11 @@ Ext4RetrieveDirent ( Off += Partition->BlockSize; } + Status = EFI_NOT_FOUND; + +Out: FreePool (Buf); - return EFI_NOT_FOUND; + return Status; } /** @@ -248,13 +264,18 @@ Ext4OpenDirent ( // Using the parent's parent's dentry File->Dentry = Directory->Dentry->Parent; - ASSERT (File->Dentry != NULL); + if (!File->Dentry) { + // Someone tried .. on root, so direct them to / + // This is an illegal EFI Open() but is possible to hit from a variety of internal code + File->Dentry = Directory->Dentry; + } Ext4RefDentry (File->Dentry); } else { File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry); - if (!File->Dentry) { + if (File->Dentry == NULL) { + Status = EFI_OUT_OF_RESOURCES; goto Error; } } @@ -436,6 +457,7 @@ Ext4ReadDir ( EXT4_FILE *TempFile; BOOLEAN ShouldSkip; BOOLEAN IsDotOrDotDot; + CHAR16 DirentUcs2Name[EXT4_NAME_MAX + 1]; DirIno = File->Inode; Status = EFI_SUCCESS; @@ -491,18 +513,35 @@ Ext4ReadDir ( // or a checksum at the end of the directory block. // memcmp (and CompareMem) return 0 when the passed length is 0. - IsDotOrDotDot = Entry.name_len != 0 && - (CompareMem (Entry.name, ".", Entry.name_len) == 0 || - CompareMem (Entry.name, "..", Entry.name_len) == 0); + // We must bound name_len as > 0 and <= 2 to avoid any out-of-bounds accesses or bad detection of + // "." and "..". + IsDotOrDotDot = Entry.name_len > 0 && Entry.name_len <= 2 && + CompareMem (Entry.name, "..", Entry.name_len) == 0; - // When inode = 0, it's unused. - ShouldSkip = Entry.inode == 0 || IsDotOrDotDot; + // When inode = 0, it's unused. When name_len == 0, it's a nameless entry + // (which we should not expose to ReadDir). + ShouldSkip = Entry.inode == 0 || Entry.name_len == 0 || IsDotOrDotDot; if (ShouldSkip) { Offset += Entry.rec_len; continue; } + // Test if the dirent is valid utf-8. This is already done inside Ext4OpenDirent but EFI_INVALID_PARAMETER + // has the danger of its meaning being overloaded in many places, so we can't skip according to that. + // So test outside of it, explicitly. + Status = Ext4GetUcs2DirentName (&Entry, DirentUcs2Name); + + if (EFI_ERROR (Status)) { + if (Status == EFI_INVALID_PARAMETER) { + // Bad UTF-8, skip. + Offset += Entry.rec_len; + continue; + } + + goto Out; + } + Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File); if (EFI_ERROR (Status)) { diff --git a/Ext4Pkg/Ext4Dxe/DiskUtil.c b/Ext4Pkg/Ext4Dxe/DiskUtil.c index 32da35f7d9..5df9ce5baf 100644 --- a/Ext4Pkg/Ext4Dxe/DiskUtil.c +++ b/Ext4Pkg/Ext4Dxe/DiskUtil.c @@ -54,17 +54,20 @@ Ext4ReadBlocks ( UINT64 Offset; UINTN Length; + ASSERT (NumberBlocks != 0); + ASSERT (BlockNumber != EXT4_BLOCK_FILE_HOLE); + Offset = MultU64x32 (BlockNumber, Partition->BlockSize); Length = NumberBlocks * Partition->BlockSize; // Check for overflow on the block -> byte conversions. // Partition->BlockSize is never 0, so we don't need to check for that. - if (Offset > DivU64x32 ((UINT64)-1, Partition->BlockSize)) { + if (DivU64x64Remainder (Offset, BlockNumber, NULL) != Partition->BlockSize) { return EFI_INVALID_PARAMETER; } - if (Length > (UINTN)-1/Partition->BlockSize) { + if (Length / NumberBlocks != Partition->BlockSize) { return EFI_INVALID_PARAMETER; } @@ -92,14 +95,21 @@ Ext4AllocAndReadBlocks ( VOID *Buf; UINTN Length; + // Check that number of blocks isn't empty, because + // this is incorrect condition for opened partition, + // so we just early-exit + if ((NumberBlocks == 0) || (BlockNumber == EXT4_BLOCK_FILE_HOLE)) { + return NULL; + } + Length = NumberBlocks * Partition->BlockSize; - if (Length > (UINTN)-1/Partition->BlockSize) { + // Check for integer overflow + if (Length / NumberBlocks != Partition->BlockSize) { return NULL; } Buf = AllocatePool (Length); - if (Buf == NULL) { return NULL; } diff --git a/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Ext4Pkg/Ext4Dxe/Ext4Disk.h index 4fd91a4233..70cb6c3209 100644 --- a/Ext4Pkg/Ext4Dxe/Ext4Disk.h +++ b/Ext4Pkg/Ext4Dxe/Ext4Disk.h @@ -1,7 +1,7 @@ /** @file Raw filesystem data structures - Copyright (c) 2021 Pedro Falcato All rights reserved. + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent Layout of an EXT2/3/4 filesystem: @@ -397,12 +397,29 @@ typedef struct _Ext4Inode { UINT32 i_projid; } EXT4_INODE; +#define EXT4_NAME_MAX 255 + typedef struct { + // offset 0x0: inode number (if 0, unused entry, should skip.) UINT32 inode; + // offset 0x4: Directory entry's length. + // Note: rec_len >= name_len + EXT4_MIN_DIR_ENTRY_LEN and rec_len % 4 == 0. UINT16 rec_len; + // offset 0x6: Directory entry's name's length UINT8 name_len; + // offset 0x7: Directory entry's file type indicator UINT8 file_type; - CHAR8 name[255]; + // offset 0x8: name[name_len]: Variable length character array; not null-terminated. + CHAR8 name[EXT4_NAME_MAX]; + // Further notes on names: + // 1) We use EXT4_NAME_MAX here instead of flexible arrays for ease of use around the driver. + // + // 2) ext4 directories are defined, as the documentation puts it, as: + // "a directory is more or less a flat file that maps an arbitrary byte string + // (usually ASCII) to an inode number on the filesystem". So, they are not + // necessarily encoded with ASCII, UTF-8, or any of the sort. We must treat it + // as a bag of bytes. When interacting with EFI interfaces themselves (which expect UCS-2) + // we skip any directory entry that is not valid UTF-8. } EXT4_DIR_ENTRY; #define EXT4_MIN_DIR_ENTRY_LEN 8 @@ -467,8 +484,17 @@ typedef UINT64 EXT4_BLOCK_NR; typedef UINT32 EXT2_BLOCK_NR; typedef UINT32 EXT4_INO_NR; -// 2 is always the root inode number in ext4 -#define EXT4_ROOT_INODE_NR 2 +/* Special inode numbers */ +#define EXT4_ROOT_INODE_NR 2 +#define EXT4_USR_QUOTA_INODE_NR 3 +#define EXT4_GRP_QUOTA_INODE_NR 4 +#define EXT4_BOOT_LOADER_INODE_NR 5 +#define EXT4_UNDEL_DIR_INODE_NR 6 +#define EXT4_RESIZE_INODE_NR 7 +#define EXT4_JOURNAL_INODE_NR 8 + +/* First non-reserved inode for old ext4 filesystems */ +#define EXT4_GOOD_OLD_FIRST_INODE_NR 11 #define EXT4_BLOCK_FILE_HOLE 0 diff --git a/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Ext4Pkg/Ext4Dxe/Ext4Dxe.h index adf3c13f6e..786e20f49f 100644 --- a/Ext4Pkg/Ext4Dxe/Ext4Dxe.h +++ b/Ext4Pkg/Ext4Dxe/Ext4Dxe.h @@ -1,7 +1,7 @@ /** @file Common header for the driver - Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved. + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -31,8 +31,7 @@ #include "Ext4Disk.h" -#define SYMLOOP_MAX 8 -#define EXT4_NAME_MAX 255 +#define SYMLOOP_MAX 8 // // We need to specify path length limit for security purposes, to prevent possible // overflows and dead-loop conditions. Originally this limit is absent in FS design, @@ -41,6 +40,20 @@ #define EXT4_EFI_PATH_MAX 4096 #define EXT4_DRIVER_VERSION 0x0000 +// +// The EXT4 Specification doesn't strictly limit block size and this value could be up to 2^31, +// but in practice it is limited by PAGE_SIZE due to performance significant impact. +// Many EXT4 implementations have size of block limited to PAGE_SIZE. In many cases it's limited +// to 4096, which is a commonly supported page size on most MMU-capable hardware, and up to 65536. +// So, to take a balance between compatibility and security measures, it is decided to use the +// value of 2MiB as the limit, which is equal to large page size on new hardware. +// As for supporting big block sizes, EXT4 has a RO_COMPAT_FEATURE called BIGALLOC, which changes +// EXT4 to use clustered allocation, so that each bit in the ext4 block allocation bitmap addresses +// a power of two number of blocks. So it would be wiser to implement and use this feature +// if there is such a need instead of big block size. +// +#define EXT4_LOG_BLOCK_SIZE_MAX 11 + /** Opens an ext4 partition and installs the Simple File System protocol. @@ -156,7 +169,7 @@ Ext4UnrefDentry ( @param[out] Partition Partition structure to fill with filesystem details. - @retval EFI_SUCCESS Parsing was succesful and the partition is a + @retval EFI_SUCCESS Parsing was successful and the partition is a valid ext4 partition. **/ EFI_STATUS @@ -288,6 +301,16 @@ Ext4GetBlockGroupDesc ( IN UINT32 BlockGroup ); +/** + Checks inode number validity across superblock of the opened partition. + + @param[in] Partition Pointer to the opened ext4 partition. + + @return TRUE if inode number is valid. +**/ +#define EXT4_IS_VALID_INODE_NR(Partition, InodeNum) \ + (((InodeNum) > 0) && (InodeNum) <= (Partition->SuperBlock.s_inodes_count)) + /** Reads an inode from disk. @@ -323,7 +346,7 @@ Ext4ReadInode ( @param[out] Buffer Pointer to the buffer. @param[in] Offset Offset of the read. @param[in out] Length Pointer to the length of the buffer, in bytes. - After a succesful read, it's updated to the + After a successful read, it's updated to the number of read bytes. @return Status of the read operation. @@ -356,7 +379,7 @@ cover. @param[out] Extent Pointer to the output buffer, where the extent will be copied to. - @retval EFI_SUCCESS Retrieval was succesful. + @retval EFI_SUCCESS Retrieval was successful. @retval EFI_NO_MAPPING Block has no mapping. **/ EFI_STATUS @@ -945,11 +968,11 @@ Ext4StrCmpInsensitive ( Retrieves the filename of the directory entry and converts it to UTF-16/UCS-2 @param[in] Entry Pointer to a EXT4_DIR_ENTRY. - @param[out] Ucs2FileName Pointer to an array of CHAR16's, of size -EXT4_NAME_MAX + 1. + @param[out] Ucs2FileName Pointer to an array of CHAR16's, of size EXT4_NAME_MAX + 1. - @retval EFI_SUCCESS Unicode collation was successfully initialised. - @retval !EFI_SUCCESS Failure. + @retval EFI_SUCCESS The filename was successfully retrieved and converted to UCS2. + @retval EFI_INVALID_PARAMETER The filename is not valid UTF-8. + @retval !EFI_SUCCESS Failure. **/ EFI_STATUS Ext4GetUcs2DirentName ( @@ -1107,7 +1130,7 @@ Ext4CalculateBlockGroupDescChecksum ( ); /** - Verifies the existance of a particular RO compat feature set. + Verifies the existence of a particular RO compat feature set. @param[in] Partition Pointer to the opened EXT4 partition. @param[in] RoCompatFeatureSet Feature set to test. @@ -1117,7 +1140,7 @@ Ext4CalculateBlockGroupDescChecksum ( ((Partition->FeaturesRoCompat & RoCompatFeatureSet) == RoCompatFeatureSet) /** - Verifies the existance of a particular compat feature set. + Verifies the existence of a particular compat feature set. @param[in] Partition Pointer to the opened EXT4 partition. @param[in] CompatFeatureSet Feature set to test. @@ -1127,7 +1150,7 @@ Ext4CalculateBlockGroupDescChecksum ( ((Partition->FeaturesCompat & CompatFeatureSet) == CompatFeatureSet) /** - Verifies the existance of a particular compat feature set. + Verifies the existence of a particular compat feature set. @param[in] Partition Pointer to the opened EXT4 partition. @param[in] IncompatFeatureSet Feature set to test. @@ -1218,7 +1241,7 @@ Ext4GetExtentLength ( @param[in] LogicalBlock Block number which the returned extent must cover. @param[out] Extent Pointer to the output buffer, where the extent will be copied to. - @retval EFI_SUCCESS Retrieval was succesful. + @retval EFI_SUCCESS Retrieval was successful. @retval EFI_NO_MAPPING Block has no mapping. **/ EFI_STATUS diff --git a/Ext4Pkg/Ext4Dxe/Extents.c b/Ext4Pkg/Ext4Dxe/Extents.c index f1964426d2..9e4364e50d 100644 --- a/Ext4Pkg/Ext4Dxe/Extents.c +++ b/Ext4Pkg/Ext4Dxe/Extents.c @@ -219,7 +219,7 @@ Ext4ExtentIdxLeafBlock ( @param[in] LogicalBlock Block number which the returned extent must cover. @param[out] Extent Pointer to the output buffer, where the extent will be copied to. - @retval EFI_SUCCESS Retrieval was succesful. + @retval EFI_SUCCESS Retrieval was successful. @retval EFI_NO_MAPPING Block has no mapping. **/ EFI_STATUS @@ -237,6 +237,7 @@ Ext4GetExtent ( EXT4_EXTENT_HEADER *ExtHeader; EXT4_EXTENT_INDEX *Index; EFI_STATUS Status; + EXT4_BLOCK_NR BlockNumber; Inode = File->Inode; Ext = NULL; @@ -288,7 +289,17 @@ Ext4GetExtent ( // Therefore, we can use binary search, and it's actually the standard for doing so // (see FreeBSD). - Index = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); + Index = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); + BlockNumber = Ext4ExtentIdxLeafBlock (Index); + + // Check that block isn't file hole + if (BlockNumber == EXT4_BLOCK_FILE_HOLE) { + if (Buffer != NULL) { + FreePool (Buffer); + } + + return EFI_VOLUME_CORRUPTED; + } if (Buffer == NULL) { Buffer = AllocatePool (Partition->BlockSize); @@ -298,8 +309,7 @@ Ext4GetExtent ( } // Read the leaf block onto the previously-allocated buffer. - - Status = Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlock (Index)); + Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber); if (EFI_ERROR (Status)) { FreePool (Buffer); return Status; @@ -615,7 +625,7 @@ Ext4GetExtentLength ( IN CONST EXT4_EXTENT *Extent ) { - // If it's an unintialized extent, the true length is ee_len - 2^15 + // If it's an uninitialized extent, the true length is ee_len - 2^15 if (EXT4_EXTENT_IS_UNINITIALIZED (Extent)) { return Extent->ee_len - EXT4_EXTENT_MAX_INITIALIZED; } diff --git a/Ext4Pkg/Ext4Dxe/File.c b/Ext4Pkg/Ext4Dxe/File.c index 04198a53bf..677caf88fb 100644 --- a/Ext4Pkg/Ext4Dxe/File.c +++ b/Ext4Pkg/Ext4Dxe/File.c @@ -105,7 +105,7 @@ Ext4IsLastPathSegment ( @param[in out] File Pointer to the file we're opening. @param[in] OpenMode Mode in which to open the file. - @return True if the open was succesful, false if we don't have + @return True if the open was successful, false if we don't have enough permissions. **/ STATIC @@ -207,6 +207,11 @@ Ext4OpenInternal ( Level = 0; DEBUG ((DEBUG_FS, "[ext4] Ext4OpenInternal %s\n", FileName)); + + if (!Ext4FileIsDir (Current)) { + return EFI_INVALID_PARAMETER; + } + // If the path starts with a backslash, we treat the root directory as the base directory if (FileName[0] == L'\\') { FileName++; @@ -219,6 +224,10 @@ Ext4OpenInternal ( return EFI_ACCESS_DENIED; } + if (!Ext4FileIsDir (Current)) { + return EFI_INVALID_PARAMETER; + } + // Discard leading path separators while (FileName[0] == L'\\') { FileName++; @@ -242,10 +251,6 @@ Ext4OpenInternal ( DEBUG ((DEBUG_FS, "[ext4] Opening %s\n", PathSegment)); - if (!Ext4FileIsDir (Current)) { - return EFI_INVALID_PARAMETER; - } - if (!Ext4IsLastPathSegment (FileName)) { if (!Ext4DirCanLookup (Current)) { return EFI_ACCESS_DENIED; @@ -714,7 +719,11 @@ Ext4GetVolumeName ( VolNameLength = StrLen (VolumeName); } else { - VolumeName = AllocateZeroPool (sizeof (CHAR16)); + VolumeName = AllocateZeroPool (sizeof (CHAR16)); + if (VolumeName == NULL) { + return EFI_OUT_OF_RESOURCES; + } + VolNameLength = 0; } @@ -781,7 +790,9 @@ Ext4GetFilesystemInfo ( Info->VolumeSize = MultU64x32 (TotalBlocks, Part->BlockSize); Info->FreeSpace = MultU64x32 (FreeBlocks, Part->BlockSize); - StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName); + Status = StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName); + + ASSERT_EFI_ERROR (Status); FreePool (VolumeName); diff --git a/Ext4Pkg/Ext4Dxe/Inode.c b/Ext4Pkg/Ext4Dxe/Inode.c index 5ccb4d2bfc..8db051d3c4 100644 --- a/Ext4Pkg/Ext4Dxe/Inode.c +++ b/Ext4Pkg/Ext4Dxe/Inode.c @@ -76,7 +76,7 @@ Ext4CalculateInodeChecksum ( @param[out] Buffer Pointer to the buffer. @param[in] Offset Offset of the read. @param[in out] Length Pointer to the length of the buffer, in bytes. - After a succesful read, it's updated to the number of read bytes. + After a successful read, it's updated to the number of read bytes. @return Status of the read operation. **/ @@ -152,7 +152,7 @@ Ext4Read ( } else { // Uninitialized extents behave exactly the same as file holes, except they have // blocks already allocated to them. - HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) - HoleOff; + HoleLen = MultU64x32 (Ext4GetExtentLength (&Extent), Partition->BlockSize) - HoleOff; } WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen; @@ -166,7 +166,7 @@ Ext4Read ( Partition->BlockSize ); ExtentLengthBytes = Extent.ee_len * Partition->BlockSize; - ExtentLogicalBytes = (UINT64)Extent.ee_block * Partition->BlockSize; + ExtentLogicalBytes = MultU64x32 ((UINT64)Extent.ee_block, Partition->BlockSize); ExtentOffset = CurrentSeek - ExtentLogicalBytes; ExtentMayRead = (UINTN)(ExtentLengthBytes - ExtentOffset); @@ -230,7 +230,7 @@ Ext4AllocateInode ( Inode = AllocateZeroPool (InodeSize); - if (!Inode) { + if (Inode == NULL) { return NULL; } diff --git a/Ext4Pkg/Ext4Dxe/Superblock.c b/Ext4Pkg/Ext4Dxe/Superblock.c index edee051c41..3f56de93c1 100644 --- a/Ext4Pkg/Ext4Dxe/Superblock.c +++ b/Ext4Pkg/Ext4Dxe/Superblock.c @@ -151,7 +151,7 @@ Ext4VerifySuperblockChecksum ( Opens and parses the superblock. @param[out] Partition Partition structure to fill with filesystem details. - @retval EFI_SUCCESS Parsing was succesful and the partition is a + @retval EFI_SUCCESS Parsing was successful and the partition is a valid ext4 partition. **/ EFI_STATUS @@ -203,7 +203,7 @@ Ext4OpenSuperblock ( // Now, check for the feature set of the filesystem // It's essential to check for this to avoid filesystem corruption and to avoid - // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disasterous. + // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disastrous. if (Partition->FeaturesIncompat & ~gSupportedIncompatFeat) { DEBUG (( @@ -220,13 +220,11 @@ Ext4OpenSuperblock ( } // At the time of writing, it's the only supported checksum. - if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM && - (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C)) - { + if (EXT4_HAS_METADATA_CSUM (Partition) && (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C)) { return EFI_UNSUPPORTED; } - if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) != 0) { + if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_CSUM_SEED)) { Partition->InitialSeed = Sb->s_checksum_seed; } else { Partition->InitialSeed = Ext4CalculateChecksum (Partition, Sb->s_uuid, 16, ~0U); @@ -245,6 +243,16 @@ Ext4OpenSuperblock ( DEBUG ((DEBUG_FS, "Read only = %u\n", Partition->ReadOnly)); + if (Sb->s_inodes_per_group == 0) { + DEBUG ((DEBUG_ERROR, "[ext4] Inodes per group can not be zero\n")); + return EFI_VOLUME_CORRUPTED; + } + + if (Sb->s_log_block_size > EXT4_LOG_BLOCK_SIZE_MAX) { + DEBUG ((DEBUG_ERROR, "[ext4] SuperBlock s_log_block_size %lu is too big\n", Sb->s_log_block_size)); + return EFI_UNSUPPORTED; + } + Partition->BlockSize = (UINT32)LShiftU64 (1024, Sb->s_log_block_size); // The size of a block group can also be calculated as 8 * Partition->BlockSize @@ -312,7 +320,7 @@ Ext4OpenSuperblock ( return EFI_OUT_OF_RESOURCES; } - // Note that the cast below is completely safe, because EXT4_FILE is a specialisation of EFI_FILE_PROTOCOL + // Note that the cast below is completely safe, because EXT4_FILE is a specialization of EFI_FILE_PROTOCOL Status = Ext4OpenVolume (&Partition->Interface, (EFI_FILE_PROTOCOL **)&Partition->Root); if (EFI_ERROR (Status)) { diff --git a/Ext4Pkg/Ext4Dxe/Symlink.c b/Ext4Pkg/Ext4Dxe/Symlink.c index 0905417ffb..d80cb9fff9 100644 --- a/Ext4Pkg/Ext4Dxe/Symlink.c +++ b/Ext4Pkg/Ext4Dxe/Symlink.c @@ -1,7 +1,7 @@ /** @file Symbolic links routines - Copyright (c) 2022 Savva Mitrofanov All rights reserved. + Copyright (c) 2022-2023 Savva Mitrofanov All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -155,19 +155,20 @@ Ext4ReadSlowSymlink ( return Status; } - // - // Add null-terminator - // - SymlinkTmp[SymlinkSizeTmp] = '\0'; - if (SymlinkSizeTmp != ReadSize) { DEBUG (( DEBUG_FS, "[ext4] Error! The size of the read block doesn't match the value from the inode!\n" )); + FreePool (SymlinkTmp); return EFI_VOLUME_CORRUPTED; } + // + // Add null-terminator + // + SymlinkTmp[ReadSize] = '\0'; + *AsciiSymlinkSize = SymlinkAllocateSize; *AsciiSymlink = SymlinkTmp; @@ -201,7 +202,7 @@ Ext4ReadSymlink ( CHAR16 *Needle; // - // Assume that we alread read Inode via Ext4ReadInode + // Assume that we already read Inode via Ext4ReadInode // Skip reading, just check encryption flag // if ((File->Inode->i_flags & EXT4_ENCRYPT_FL) != 0) { @@ -242,7 +243,6 @@ Ext4ReadSymlink ( Status )); FreePool (Symlink16Tmp); - FreePool (SymlinkTmp); return Status; }