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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92178): https://edk2.groups.io/g/devel/message/92178
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]
-=-=-=-=-=-=-=-=-=-=-=-
