Pushed as c367ec5 On Sun, Aug 7, 2022 at 12:21 AM Pedro Falcato via groups.io wrote: > > > On Fri, Jul 29, 2022 at 4:54 PM Savva Mitrofanov > wrote: > >> This changes tends to improve security of code sections by fixing >> integer overflows, missing alignment checks, unsafe casts, also >> simplified some routines, fixed compiler warnings and corrected some >> code mistakes. >> >> - Set HoleLen to UINT64 to prevent truncation in Ext4Read function >> - Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because >> by specification files using block maps must be placed within the first >> 2^32 blocks of a filesystem >> - Replace UNREACHABLE with ASSERT (FALSE) in case of new checksum >> algorithms, due to it is an invariant violation rather than unreachable >> path >> - Solve compiler warnings. Initialize all fields in gExt4BindingProtocol >> Fix comparison of integer expressions of different signedness >> - Field name_len has type CHAR8, while filename limit is 255 >> (EXT4_NAME_MAX), so because structure EXT4_DIR_ENTRY would be >> unchangeable in future, we could drop this check without any >> assertions >> - Simplify Ext4RemoveDentry logic by using IsNodeInList >> - Fix possible int overflow in Ext4ExtentsMapKeyCompare >> - Return bad block type in Ext4GetBlockpath >> - Adds 4-byte aligned check for superblock group descriptor size field >> >> Cc: Marvin Häuser >> Cc: Pedro Falcato >> Cc: Vitaly Cheptsov >> Signed-off-by: Savva Mitrofanov >> --- >> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 3 +- >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 2 +- >> Features/Ext4Pkg/Ext4Dxe/BlockGroup.c | 4 +-- >> Features/Ext4Pkg/Ext4Dxe/BlockMap.c | 18 ++++++++---- >> Features/Ext4Pkg/Ext4Dxe/Directory.c | 29 ++------------------ >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 10 ++++--- >> Features/Ext4Pkg/Ext4Dxe/Extents.c | 8 ++++-- >> Features/Ext4Pkg/Ext4Dxe/Inode.c | 10 +++---- >> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 15 +++++----- >> 9 files changed, 44 insertions(+), 55 deletions(-) >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h >> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h >> index a55cd2fa68ad..7a19d2f79d53 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h >> @@ -338,7 +338,7 @@ STATIC_ASSERT ( >> #define EXT4_TIND_BLOCK 14 >> #define EXT4_NR_BLOCKS 15 >> >> -#define EXT4_GOOD_OLD_INODE_SIZE 128 >> +#define EXT4_GOOD_OLD_INODE_SIZE 128U >> >> typedef struct _Ext4_I_OSD2_Linux { >> UINT16 l_i_blocks_high; >> @@ -463,6 +463,7 @@ typedef struct { >> #define EXT4_EXTENT_MAX_INITIALIZED (1 << 15) >> >> typedef UINT64 EXT4_BLOCK_NR; >> +typedef UINT32 EXT2_BLOCK_NR; >> typedef UINT32 EXT4_INO_NR; >> >> // 2 is always the root inode number in ext4 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> index b1508482b0a7..b446488b2112 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> @@ -1165,7 +1165,7 @@ EFI_STATUS >> Ext4GetBlocks ( >> IN EXT4_PARTITION *Partition, >> IN EXT4_FILE *File, >> - IN EXT4_BLOCK_NR LogicalBlock, >> + IN EXT2_BLOCK_NR LogicalBlock, >> OUT EXT4_EXTENT *Extent >> ); >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c >> b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c >> index 9a1a41901f36..572e8f60ab92 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c >> @@ -218,9 +218,9 @@ Ext4CalculateBlockGroupDescChecksum ( >> IN UINT32 BlockGroupNum >> ) >> { >> - if (Partition->FeaturesRoCompat & >> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) { >> + if ((Partition->FeaturesRoCompat & >> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) != 0) { >> return Ext4CalculateBlockGroupDescChecksumMetadataCsum (Partition, >> BlockGroupDesc, BlockGroupNum); >> - } else if (Partition->FeaturesRoCompat & >> EXT4_FEATURE_RO_COMPAT_GDT_CSUM) { >> + } else if ((Partition->FeaturesRoCompat & >> EXT4_FEATURE_RO_COMPAT_GDT_CSUM) != 0) { >> return Ext4CalculateBlockGroupDescChecksumGdtCsum (Partition, >> BlockGroupDesc, BlockGroupNum); >> } >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c >> b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c >> index 1a06ac9fbf86..2bc629fe9d38 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c >> @@ -70,7 +70,7 @@ UINTN >> Ext4GetBlockPath ( >> IN CONST EXT4_PARTITION *Partition, >> IN UINT32 LogicalBlock, >> - OUT EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH] >> + OUT EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH] >> ) >> { >> // The logic behind the block map is very much like a page table >> @@ -123,7 +123,7 @@ Ext4GetBlockPath ( >> break; >> default: >> // EXT4_TYPE_BAD_BLOCK >> - return -1; >> + break; >> } >> >> return Type + 1; >> @@ -213,12 +213,12 @@ EFI_STATUS >> Ext4GetBlocks ( >> IN EXT4_PARTITION *Partition, >> IN EXT4_FILE *File, >> - IN EXT4_BLOCK_NR LogicalBlock, >> + IN EXT2_BLOCK_NR LogicalBlock, >> OUT EXT4_EXTENT *Extent >> ) >> { >> EXT4_INODE *Inode; >> - EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]; >> + EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]; >> UINTN BlockPathLength; >> UINTN Index; >> UINT32 *Buffer; >> @@ -230,7 +230,7 @@ Ext4GetBlocks ( >> >> BlockPathLength = Ext4GetBlockPath (Partition, LogicalBlock, >> BlockPath); >> >> - if (BlockPathLength == (UINTN)-1) { >> + if (BlockPathLength - 1 == EXT4_TYPE_BAD_BLOCK) { >> // Bad logical block (out of range) >> return EFI_NO_MAPPING; >> } >> @@ -272,7 +272,13 @@ Ext4GetBlocks ( >> } >> } >> >> - Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof >> (UINT32), BlockPath[BlockPathLength - 1], Extent); >> + Ext4GetExtentInBlockMap ( >> + Buffer, >> + Partition->BlockSize / sizeof (UINT32), >> + BlockPath[BlockPathLength - 1], >> + Extent >> + ); >> + >> FreePool (Buffer); >> >> return EFI_SUCCESS; >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c >> b/Features/Ext4Pkg/Ext4Dxe/Directory.c >> index 682f66ad5525..4441e6d192b6 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c >> @@ -74,7 +74,7 @@ Ext4ValidDirent ( >> } >> >> // Dirent sizes need to be 4 byte aligned >> - if (Dirent->rec_len % 4) { >> + if ((Dirent->rec_len % 4) != 0) { >> return FALSE; >> } >> >> @@ -160,17 +160,6 @@ Ext4RetrieveDirent ( >> return EFI_VOLUME_CORRUPTED; >> } >> >> - // Ignore names bigger than our limit. >> - >> - /* Note: I think having a limit is sane because: >> - 1) It's nicer to work with. >> - 2) Linux and a number of BSDs also have a filename limit of 255. >> - */ >> - if (Entry->name_len > EXT4_NAME_MAX) { >> - BlockOffset += Entry->rec_len; >> - continue; >> - } >> - >> // Unused entry >> if (Entry->inode == 0) { >> BlockOffset += Entry->rec_len; >> @@ -548,20 +537,8 @@ Ext4RemoveDentry ( >> IN OUT EXT4_DENTRY *ToBeRemoved >> ) >> { >> - EXT4_DENTRY *D; >> - LIST_ENTRY *Entry; >> - LIST_ENTRY *NextEntry; >> - >> - BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) { >> - D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry); >> - >> - if (D == ToBeRemoved) { >> - RemoveEntryList (Entry); >> - return; >> - } >> - } >> - >> - DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the >> asked-for dentry\n")); >> + ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent->Children)); >> + RemoveEntryList (&ToBeRemoved->ListNode); >> } >> >> /** >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> index 43b9340d3956..2a4f5a7bd0ef 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> @@ -260,10 +260,12 @@ Ext4Stop ( >> >> EFI_DRIVER_BINDING_PROTOCOL gExt4BindingProtocol = >> { >> - Ext4IsBindingSupported, >> - Ext4Bind, >> - Ext4Stop, >> - EXT4_DRIVER_VERSION >> + .Supported = Ext4IsBindingSupported, >> + .Start = Ext4Bind, >> + .Stop = Ext4Stop, >> + .Version = EXT4_DRIVER_VERSION, >> + .ImageHandle = NULL, >> + .DriverBindingHandle = NULL >> }; >> >> /** >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c >> b/Features/Ext4Pkg/Ext4Dxe/Extents.c >> index c3874df71751..369879e07fe7 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c >> @@ -257,9 +257,11 @@ Ext4GetExtent ( >> return EFI_SUCCESS; >> } >> >> - if (!(Inode->i_flags & EXT4_EXTENTS_FL)) { >> + if ((Inode->i_flags & EXT4_EXTENTS_FL) == 0) { >> // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent >> using the block map >> - Status = Ext4GetBlocks (Partition, File, LogicalBlock, Extent); >> + // By specification files using block maps must be placed within the >> first 2^32 blocks >> + // of a filesystem, so we can safely cast LogicalBlock to uint32 >> + Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, >> Extent); >> > This comment is wrong. It should read something like "Files using block > maps are limited to 2^32 blocks, so we can safely...". I'll fix this up > myself since this is minor and the rest LGTM. > >> >> if (!EFI_ERROR (Status)) { >> Ext4CacheExtents (File, Extent, 1); >> @@ -420,7 +422,7 @@ Ext4ExtentsMapKeyCompare ( >> Extent = UserStruct; >> Block = (UINT32)(UINTN)StandaloneKey; >> >> - if ((Block >= Extent->ee_block) && (Block < Extent->ee_block + >> Ext4GetExtentLength (Extent))) { >> + if ((Block >= Extent->ee_block) && (Block - Extent->ee_block < >> Ext4GetExtentLength (Extent))) { >> return 0; >> } >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c >> b/Features/Ext4Pkg/Ext4Dxe/Inode.c >> index 831f5946e870..7f8be2f02643 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c >> @@ -100,7 +100,7 @@ Ext4Read ( >> EFI_STATUS Status; >> BOOLEAN HasBackingExtent; >> UINT32 HoleOff; >> - UINTN HoleLen; >> + UINT64 HoleLen; >> UINT64 ExtentStartBytes; >> UINT64 ExtentLengthBytes; >> UINT64 ExtentLogicalBytes; >> @@ -155,10 +155,10 @@ Ext4Read ( >> HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) >> - HoleOff; >> } >> >> - WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen; >> + WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen; >> // Potential improvement: In the future, we could get the file >> hole's total >> // size and memset all that >> - SetMem (Buffer, WasRead, 0); >> + ZeroMem (Buffer, WasRead); >> } else { >> ExtentStartBytes = MultU64x32 ( >> LShiftU64 (Extent.ee_start_hi, 32) | >> @@ -291,7 +291,7 @@ Ext4FilePhysicalSpace ( >> >> // If HUGE_FILE is enabled and EXT4_HUGE_FILE_FL is set in the >> inode's flags, each unit >> // in i_blocks corresponds to an actual filesystem block >> - if (File->Inode->i_flags & EXT4_HUGE_FILE_FL) { >> + if ((File->Inode->i_flags & EXT4_HUGE_FILE_FL) != 0) { >> return MultU64x32 (Blocks, File->Partition->BlockSize); >> } >> } >> @@ -431,7 +431,7 @@ Ext4FileCreateTime ( >> Inode = File->Inode; >> >> if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) { >> - SetMem (Time, sizeof (EFI_TIME), 0); >> + ZeroMem (Time, sizeof (EFI_TIME)); >> return; >> } >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> index 47fc3a65507a..c22155ba11b4 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> @@ -220,7 +220,7 @@ Ext4OpenSuperblock ( >> return EFI_UNSUPPORTED; >> } >> >> - if (Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) { >> + if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) != >> 0) { >> Partition->InitialSeed = Sb->s_checksum_seed; >> } else { >> Partition->InitialSeed = Ext4CalculateChecksum (Partition, >> Sb->s_uuid, 16, ~0U); >> @@ -257,16 +257,17 @@ Ext4OpenSuperblock ( >> )); >> >> if (EXT4_IS_64_BIT (Partition)) { >> + // s_desc_size should be 4 byte aligned and >> + // 64 bit filesystems need DescSize to be 64 bytes >> + if (((Sb->s_desc_size % 4) != 0) || (Sb->s_desc_size < >> EXT4_64BIT_BLOCK_DESC_SIZE)) { >> + return EFI_VOLUME_CORRUPTED; >> + } >> + >> Partition->DescSize = Sb->s_desc_size; >> } else { >> Partition->DescSize = EXT4_OLD_BLOCK_DESC_SIZE; >> } >> >> - if ((Partition->DescSize < EXT4_64BIT_BLOCK_DESC_SIZE) && >> EXT4_IS_64_BIT (Partition)) { >> - // 64 bit filesystems need DescSize to be 64 bytes >> - return EFI_VOLUME_CORRUPTED; >> - } >> - >> if (!Ext4VerifySuperblockChecksum (Partition, Sb)) { >> DEBUG ((DEBUG_ERROR, "[ext4] Bad superblock checksum %lx\n", >> Ext4CalculateSuperblockChecksum (Partition, Sb))); >> return EFI_VOLUME_CORRUPTED; >> @@ -342,7 +343,7 @@ Ext4CalculateChecksum ( >> // For some reason, EXT4 really likes non-inverted CRC32C >> checksums, so we stick to that here. >> return ~CalculateCrc32c(Buffer, Length, ~InitialValue); >> default: >> - UNREACHABLE (); >> + ASSERT (FALSE); >> return 0; >> } >> } >> -- >> 2.37.1 >> >> Overall, LGTM. > > Reviewed-by: Pedro Falcato > > -- > Pedro Falcato > > > -- Pedro Falcato