Pushed as c367ec5
<https://github.com/tianocore/edk2-platforms/commit/c367ec54f7bfce341732c6eb542809bcd77fc618>
<https://github.com/tianocore/edk2-platforms/commit/c367ec54f7bfce341732c6eb542809bcd77fc618>
<https://github.com/tianocore/edk2-platforms/commit/c367ec54f7bfce341732c6eb542809bcd77fc618>
On Sun, Aug 7, 2022 at 12:21 AM Pedro Falcato via groups.io <pedro.falcato=
gmail....@groups.io> wrote:
>
>
> On Fri, Jul 29, 2022 at 4:54 PM Savva Mitrofanov <savva...@gmail.com>
> 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 <mhaeu...@posteo.de>
>> Cc: Pedro Falcato <pedro.falc...@gmail.com>
>> Cc: Vitaly Cheptsov <vit9...@protonmail.com>
>> Signed-off-by: Savva Mitrofanov <savva...@gmail.com>
>> ---
>> 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.falc...@gmail.com>
>
> --
> Pedro Falcato
>
>
>
--
Pedro Falcato
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92179): https://edk2.groups.io/g/devel/message/92179
Mute This Topic: https://groups.io/mt/92693958/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
