On 20. Jul 2022, at 07:36, Savva Mitrofanov <savvamtr@...> wrote:

This changes tends to improve security of code sections by fixing
integer overflows, missing aligment checks, unsafe casts, also
simplified some routines, fixed compiler warnings and corrected some
code mistakes.

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

In my opinion, just "to prevent truncation".

- Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because
we consider BlockMap is 32-bit fs ext2/3 feature.
- 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. Init 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@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
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 | 13 +++++----
8 files changed, 38 insertions(+), 50 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,

This looks awkward, using an "EXT2" type in an "Ext4" function. Maybe just use UINT32?

OUT EXT4_EXTENT *Extent
);

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

The comment is misleading, because this is safe for EXT4 as well.

+ Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Extent);

if (!EFI_ERROR (Status)) {
Ext4CacheExtents (File, Extent, 1);
@@ -420,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;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 831f5946e870..4860cf576377 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) |
@@ -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..a57728a9abe6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -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.0