Re: [edk2-platforms][PATCH v3 1/1] Ext4Pkg: Code correctness and security improvements

["Pedro Falcato" <pedro.falcato@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13770 bytes --]

On Fri, Jul 29, 2022 at 4:54 PM Savva Mitrofanov <savvamtr@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 <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr@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.falcato@gmail.com>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 16186 bytes --]