Hi Savva, Could you please send a new version of the patch, with a proper encoding (try 8bit encoding, quoted-printable is screwing up my git am) and addressing Marvin's concerns? Take the time and send a new version of the symlink patchset as well. Thanks, Pedro On Wed, Jul 20, 2022 at 6:36 AM Savva Mitrofanov wrote: > This changes tends to improve security of code sections by fixing > integer overflows, missing aligment checks, unsafe casts, also > simplified some routines, fixed compiler warnings and corrected some > code mistakes. > > - Set HoleLen to UINT64 to perform safe cast to UINTN in ternary > operator at WasRead assignment. > - Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because > we consider BlockMap is 32-bit fs ext2/3 feature. > - 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. Init 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/BlockMap.c | 18 ++++++++---- > Features/Ext4Pkg/Ext4Dxe/Directory.c | 29 ++------------------ > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 10 ++++--- > Features/Ext4Pkg/Ext4Dxe/Extents.c | 5 ++-- > Features/Ext4Pkg/Ext4Dxe/Inode.c | 8 +++--- > Features/Ext4Pkg/Ext4Dxe/Superblock.c | 13 +++++---- > 8 files changed, 38 insertions(+), 50 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/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..d9ff69f21c14 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c > @@ -259,7 +259,8 @@ Ext4GetExtent ( > > if (!(Inode->i_flags & EXT4_EXTENTS_FL)) { > // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent > using the block map > - Status = Ext4GetBlocks (Partition, File, LogicalBlock, Extent); > + // We cast LogicalBlock to UINT32, considering ext2/3 are 32-bit > + Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, > Extent); > > if (!EFI_ERROR (Status)) { > Ext4CacheExtents (File, Extent, 1); > @@ -420,7 +421,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..4860cf576377 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) | > @@ -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..a57728a9abe6 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c > @@ -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.0 > > -- Pedro Falcato