public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements
@ 2022-07-19 12:10 Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 01/10] Ext4Pkg: Replace SetMem(,,0) with ZeroMem Savva Mitrofanov
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Hi all,

This patchset attempts to improve security of code sections by fixing integer overflows,
missing aligment checks, unsafe casts. Also I simplified some routines, fixed compiler warnings and 
corrected some code mistakes.

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 (10):
  Ext4Pkg: Replace SetMem(,,0) with ZeroMem
  Ext4Pkg: Change HoleLen type to UINT64
  Ext4Pkg: Use 32-bit block number in BlockMap
  Ext4Pkg: Use assertion in Ext4CalculateChecksum
  Ext4Pkg: Fix compiler warnings
  Ext4Pkg: Drop dir entry name_len limit extra check
  Ext4Pkg: Simplify Ext4RemoveDentry logic
  Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare
  Ext4Pkg: Return bad block type in Ext4GetBlockpath
  Ext4Pkg: Group descriptor size must be 4-byte aligned

 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 | 12 ++++----
 8 files changed, 37 insertions(+), 50 deletions(-)

-- 
2.37.0


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

* [edk2-platforms][PATCH 01/10] Ext4Pkg: Replace SetMem(,,0) with ZeroMem
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 02/10] Ext4Pkg: Change HoleLen type to UINT64 Savva Mitrofanov
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

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/Inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 831f5946e870..142ee6e3d78a 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -158,7 +158,7 @@ Ext4Read (
       WasRead = HoleLen > RemainingRead ? RemainingRead : 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;
   }
 
-- 
2.37.0


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

* [edk2-platforms][PATCH 02/10] Ext4Pkg: Change HoleLen type to UINT64
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 01/10] Ext4Pkg: Replace SetMem(,,0) with ZeroMem Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 03/10] Ext4Pkg: Use 32-bit block number in BlockMap Savva Mitrofanov
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Set HoleLen to UINT64 to perform safe cast to UINTN in ternary operator
at WasRead assignment

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/Inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 142ee6e3d78a..6416f5388486 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,7 +155,7 @@ 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
       ZeroMem (Buffer, WasRead);
-- 
2.37.0


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

* [edk2-platforms][PATCH 03/10] Ext4Pkg: Use 32-bit block number in BlockMap
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 01/10] Ext4Pkg: Replace SetMem(,,0) with ZeroMem Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 02/10] Ext4Pkg: Change HoleLen type to UINT64 Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 04/10] Ext4Pkg: Use assertion in Ext4CalculateChecksum Savva Mitrofanov
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because we
consider BlockMap is 32-bit fs ext2/3 feature

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 |  1 +
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h  |  2 +-
 Features/Ext4Pkg/Ext4Dxe/BlockMap.c | 14 ++++++++++----
 Features/Ext4Pkg/Ext4Dxe/Extents.c  |  3 ++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index a55cd2fa68ad..3aef6f0e5bb4 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -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..3d9e16035bee 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
@@ -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;
@@ -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/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index c3874df71751..c5951f78aa62 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);
-- 
2.37.0


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

* [edk2-platforms][PATCH 04/10] Ext4Pkg: Use assertion in Ext4CalculateChecksum
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (2 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 03/10] Ext4Pkg: Use 32-bit block number in BlockMap Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 05/10] Ext4Pkg: Fix compiler warnings Savva Mitrofanov
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Replaced UNREACHABLE with ASSERT (FALSE) in case of new checksum
algorithms, due to it is an invariant violation rather than unreachable
path

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/Superblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 47fc3a65507a..42762b6aa780 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -342,7 +342,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


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

* [edk2-platforms][PATCH 05/10] Ext4Pkg: Fix compiler warnings
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (3 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 04/10] Ext4Pkg: Use assertion in Ext4CalculateChecksum Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 06/10] Ext4Pkg: Drop dir entry name_len limit extra check Savva Mitrofanov
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Initialize all fields in gExt4BindingProtocol. Fix comparison of integer
expressions of different signedness.

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 |  2 +-
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c  | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 3aef6f0e5bb4..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;
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
 };
 
 /**
-- 
2.37.0


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

* [edk2-platforms][PATCH 06/10] Ext4Pkg: Drop dir entry name_len limit extra check
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (4 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 05/10] Ext4Pkg: Fix compiler warnings Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 07/10] Ext4Pkg: Simplify Ext4RemoveDentry logic Savva Mitrofanov
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

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

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/Directory.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 682f66ad5525..96c84c24243e 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -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;
-- 
2.37.0


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

* [edk2-platforms][PATCH 07/10] Ext4Pkg: Simplify Ext4RemoveDentry logic
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (5 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 06/10] Ext4Pkg: Drop dir entry name_len limit extra check Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 08/10] Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare Savva Mitrofanov
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

First of all BASE_LIST_FOR_EACH_SAFE doesn't have any sanity checks. So
its usage isn't "safe". We can drop this loop and use just IsNodeInList

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/Directory.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 96c84c24243e..d1038c04926e 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -537,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);
 }
 
 /**
-- 
2.37.0


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

* [edk2-platforms][PATCH 08/10] Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (6 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 07/10] Ext4Pkg: Simplify Ext4RemoveDentry logic Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 09/10] Ext4Pkg: Return bad block type in Ext4GetBlockpath Savva Mitrofanov
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

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/Extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index c5951f78aa62..80427d869bd3 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
@@ -421,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;
   }
 
-- 
2.37.0


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

* [edk2-platforms][PATCH 09/10] Ext4Pkg: Return bad block type in Ext4GetBlockpath
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (7 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 08/10] Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 12:10 ` [edk2-platforms][PATCH 10/10] Ext4Pkg: Group descriptor size must be 4-byte aligned Savva Mitrofanov
  2022-07-19 16:19 ` [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Pedro Falcato
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

Seems that returning maximum uintn in case of bad block was a mistake,
so return just bad block type

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/BlockMap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
index 3d9e16035bee..2bc629fe9d38 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
+++ b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
@@ -123,7 +123,7 @@ Ext4GetBlockPath (
       break;
     default:
       // EXT4_TYPE_BAD_BLOCK
-      return -1;
+      break;
   }
 
   return Type + 1;
@@ -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;
   }
-- 
2.37.0


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

* [edk2-platforms][PATCH 10/10] Ext4Pkg: Group descriptor size must be 4-byte aligned
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (8 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 09/10] Ext4Pkg: Return bad block type in Ext4GetBlockpath Savva Mitrofanov
@ 2022-07-19 12:10 ` Savva Mitrofanov
  2022-07-19 16:19 ` [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Pedro Falcato
  10 siblings, 0 replies; 12+ messages in thread
From: Savva Mitrofanov @ 2022-07-19 12:10 UTC (permalink / raw)
  To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov

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/Directory.c  |  2 +-
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index d1038c04926e..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;
   }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 42762b6aa780..3bf2e3001f26 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -257,16 +257,16 @@ 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;
-- 
2.37.0


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

* Re: [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements
  2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
                   ` (9 preceding siblings ...)
  2022-07-19 12:10 ` [edk2-platforms][PATCH 10/10] Ext4Pkg: Group descriptor size must be 4-byte aligned Savva Mitrofanov
@ 2022-07-19 16:19 ` Pedro Falcato
  10 siblings, 0 replies; 12+ messages in thread
From: Pedro Falcato @ 2022-07-19 16:19 UTC (permalink / raw)
  To: Savva Mitrofanov
  Cc: edk2-devel-groups-io, Marvin Häuser, Vitaly Cheptsov

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

For the record, I suggested to Savva (off-list) to squash the patch set
into one, for v2.

On Tue, Jul 19, 2022 at 1:10 PM Savva Mitrofanov <savvamtr@gmail.com> wrote:

> Hi all,
>
> This patchset attempts to improve security of code sections by fixing
> integer overflows,
> missing aligment checks, unsafe casts. Also I simplified some routines,
> fixed compiler warnings and
> corrected some code mistakes.
>
> 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 (10):
>   Ext4Pkg: Replace SetMem(,,0) with ZeroMem
>   Ext4Pkg: Change HoleLen type to UINT64
>   Ext4Pkg: Use 32-bit block number in BlockMap
>   Ext4Pkg: Use assertion in Ext4CalculateChecksum
>   Ext4Pkg: Fix compiler warnings
>   Ext4Pkg: Drop dir entry name_len limit extra check
>   Ext4Pkg: Simplify Ext4RemoveDentry logic
>   Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare
>   Ext4Pkg: Return bad block type in Ext4GetBlockpath
>   Ext4Pkg: Group descriptor size must be 4-byte aligned
>
>  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 | 12 ++++----
>  8 files changed, 37 insertions(+), 50 deletions(-)
>
> --
> 2.37.0
>
>

-- 
Pedro Falcato

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

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

end of thread, other threads:[~2022-07-19 16:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-19 12:10 [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 01/10] Ext4Pkg: Replace SetMem(,,0) with ZeroMem Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 02/10] Ext4Pkg: Change HoleLen type to UINT64 Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 03/10] Ext4Pkg: Use 32-bit block number in BlockMap Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 04/10] Ext4Pkg: Use assertion in Ext4CalculateChecksum Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 05/10] Ext4Pkg: Fix compiler warnings Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 06/10] Ext4Pkg: Drop dir entry name_len limit extra check Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 07/10] Ext4Pkg: Simplify Ext4RemoveDentry logic Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 08/10] Ext4Pkg: Fix possible int overflow in Ext4ExtentsMapKeyCompare Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 09/10] Ext4Pkg: Return bad block type in Ext4GetBlockpath Savva Mitrofanov
2022-07-19 12:10 ` [edk2-platforms][PATCH 10/10] Ext4Pkg: Group descriptor size must be 4-byte aligned Savva Mitrofanov
2022-07-19 16:19 ` [edk2-platforms][PATCH 00/10] Ext4Pkg: Code security and correctness improvements Pedro Falcato

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