public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms][PATCH v3 0/1] Ext4Pkg: Code correctness and security improvements
@ 2022-07-29 15:54 Savva Mitrofanov
  2022-07-29 15:54 ` [edk2-platforms][PATCH v3 1/1] " Savva Mitrofanov
  0 siblings, 1 reply; 4+ messages in thread
From: Savva Mitrofanov @ 2022-07-29 15:54 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Hi all,

This is the third version of patch in which I correct commit message and added some
code-style corrections into conditions with bit operations.

REF: https://github.com/savvamitrofanov/edk2-platforms/commits/ext4pkg_security_improvements

Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>

Savva Mitrofanov (1):
  Ext4Pkg: Code correctness and security improvements

 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(-)

-- 
2.37.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [edk2-platforms][PATCH v3 1/1] Ext4Pkg: Code correctness and security improvements
  2022-07-29 15:54 [edk2-platforms][PATCH v3 0/1] Ext4Pkg: Code correctness and security improvements Savva Mitrofanov
@ 2022-07-29 15:54 ` Savva Mitrofanov
  2022-08-06 23:21   ` Pedro Falcato
       [not found]   ` <1708E4D87FD363AC.26691@groups.io>
  0 siblings, 2 replies; 4+ messages in thread
From: Savva Mitrofanov @ 2022-07-29 15:54 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

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);
 
     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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [edk2-platforms][PATCH v3 1/1] Ext4Pkg: Code correctness and security improvements
  2022-07-29 15:54 ` [edk2-platforms][PATCH v3 1/1] " Savva Mitrofanov
@ 2022-08-06 23:21   ` Pedro Falcato
       [not found]   ` <1708E4D87FD363AC.26691@groups.io>
  1 sibling, 0 replies; 4+ messages in thread
From: Pedro Falcato @ 2022-08-06 23:21 UTC (permalink / raw)
  To: Savva Mitrofanov
  Cc: edk2-devel-groups-io, Marvin Häuser, Vitaly Cheptsov

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v3 1/1] Ext4Pkg: Code correctness and security improvements
       [not found]   ` <1708E4D87FD363AC.26691@groups.io>
@ 2022-08-06 23:25     ` Pedro Falcato
  0 siblings, 0 replies; 4+ messages in thread
From: Pedro Falcato @ 2022-08-06 23:25 UTC (permalink / raw)
  To: edk2-devel-groups-io, Pedro Falcato
  Cc: Savva Mitrofanov, Marvin Häuser, Vitaly Cheptsov

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

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.com@groups.io> wrote:

>
>
> 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
> 
>
>

-- 
Pedro Falcato

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-08-06 23:25 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-29 15:54 [edk2-platforms][PATCH v3 0/1] Ext4Pkg: Code correctness and security improvements Savva Mitrofanov
2022-07-29 15:54 ` [edk2-platforms][PATCH v3 1/1] " Savva Mitrofanov
2022-08-06 23:21   ` Pedro Falcato
     [not found]   ` <1708E4D87FD363AC.26691@groups.io>
2022-08-06 23:25     ` [edk2-devel] " Pedro Falcato

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox