This changes tends to improve security of code sections by fixing
integer overflows, missing alignment checks, unsafe casts, also
simplified some routines, fixed compiler warnings and corrected some
code mistakes.
- Set HoleLen to UINT64 to prevent truncation in Ext4Read function
- Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because
by specification files using block maps must be placed within the first
2^32 blocks of a filesystem
- Replace UNREACHABLE with ASSERT (FALSE) in case of new checksum
algorithms, due to it is an invariant violation rather than unreachable
path
- Solve compiler warnings. Initialize all fields in gExt4BindingProtocol
Fix comparison of integer expressions of different signedness
- Field name_len has type CHAR8, while filename limit is 255
(EXT4_NAME_MAX), so because structure EXT4_DIR_ENTRY would be
unchangeable in future, we could drop this check without any
assertions
- Simplify Ext4RemoveDentry logic by using IsNodeInList
- Fix possible int overflow in Ext4ExtentsMapKeyCompare
- Return bad block type in Ext4GetBlockpath
- Adds 4-byte aligned check for superblock group descriptor size field
Cc: Marvin Häuser <mhaeu...@posteo.de>
Cc: Pedro Falcato <pedro.falc...@gmail.com>
Cc: Vitaly Cheptsov <vit9...@protonmail.com>
Signed-off-by: Savva Mitrofanov <savva...@gmail.com>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 3 +-
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 2 +-
Features/Ext4Pkg/Ext4Dxe/BlockGroup.c | 4 +--
Features/Ext4Pkg/Ext4Dxe/BlockMap.c | 18 ++++++++----
Features/Ext4Pkg/Ext4Dxe/Directory.c | 29 ++------------------
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 10 ++++---
Features/Ext4Pkg/Ext4Dxe/Extents.c | 8 ++++--
Features/Ext4Pkg/Ext4Dxe/Inode.c | 10 +++----
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 15 +++++-----
9 files changed, 44 insertions(+), 55 deletions(-)
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index a55cd2fa68ad..7a19d2f79d53 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -338,7 +338,7 @@ STATIC_ASSERT (
#define EXT4_TIND_BLOCK 14
#define EXT4_NR_BLOCKS 15
-#define EXT4_GOOD_OLD_INODE_SIZE 128
+#define EXT4_GOOD_OLD_INODE_SIZE 128U
typedef struct _Ext4_I_OSD2_Linux {
UINT16 l_i_blocks_high;
@@ -463,6 +463,7 @@ typedef struct {
#define EXT4_EXTENT_MAX_INITIALIZED (1 << 15)
typedef UINT64 EXT4_BLOCK_NR;
+typedef UINT32 EXT2_BLOCK_NR;
typedef UINT32 EXT4_INO_NR;
// 2 is always the root inode number in ext4
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index b1508482b0a7..b446488b2112 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1165,7 +1165,7 @@ EFI_STATUS
Ext4GetBlocks (
IN EXT4_PARTITION *Partition,
IN EXT4_FILE *File,
- IN EXT4_BLOCK_NR LogicalBlock,
+ IN EXT2_BLOCK_NR LogicalBlock,
OUT EXT4_EXTENT *Extent
);
diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
index 9a1a41901f36..572e8f60ab92 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
+++ b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
@@ -218,9 +218,9 @@ Ext4CalculateBlockGroupDescChecksum (
IN UINT32 BlockGroupNum
)
{
- if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
+ if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) !=
0) {
return Ext4CalculateBlockGroupDescChecksumMetadataCsum (Partition,
BlockGroupDesc, BlockGroupNum);
- } else if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
+ } else if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
!= 0) {
return Ext4CalculateBlockGroupDescChecksumGdtCsum (Partition,
BlockGroupDesc, BlockGroupNum);
}
diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
index 1a06ac9fbf86..2bc629fe9d38 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
+++ b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
@@ -70,7 +70,7 @@ UINTN
Ext4GetBlockPath (
IN CONST EXT4_PARTITION *Partition,
IN UINT32 LogicalBlock,
- OUT EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]
+ OUT EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]
)
{
// The logic behind the block map is very much like a page table
@@ -123,7 +123,7 @@ Ext4GetBlockPath (
break;
default:
// EXT4_TYPE_BAD_BLOCK
- return -1;
+ break;
}
return Type + 1;
@@ -213,12 +213,12 @@ EFI_STATUS
Ext4GetBlocks (
IN EXT4_PARTITION *Partition,
IN EXT4_FILE *File,
- IN EXT4_BLOCK_NR LogicalBlock,
+ IN EXT2_BLOCK_NR LogicalBlock,
OUT EXT4_EXTENT *Extent
)
{
EXT4_INODE *Inode;
- EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];
+ EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];
UINTN BlockPathLength;
UINTN Index;
UINT32 *Buffer;
@@ -230,7 +230,7 @@ Ext4GetBlocks (
BlockPathLength = Ext4GetBlockPath (Partition, LogicalBlock, BlockPath);
- if (BlockPathLength == (UINTN)-1) {
+ if (BlockPathLength - 1 == EXT4_TYPE_BAD_BLOCK) {
// Bad logical block (out of range)
return EFI_NO_MAPPING;
}
@@ -272,7 +272,13 @@ Ext4GetBlocks (
}
}
- Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof (UINT32),
BlockPath[BlockPathLength - 1], Extent);
+ Ext4GetExtentInBlockMap (
+ Buffer,
+ Partition->BlockSize / sizeof (UINT32),
+ BlockPath[BlockPathLength - 1],
+ Extent
+ );
+
FreePool (Buffer);
return EFI_SUCCESS;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c
b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 682f66ad5525..4441e6d192b6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -74,7 +74,7 @@ Ext4ValidDirent (
}
// Dirent sizes need to be 4 byte aligned
- if (Dirent->rec_len % 4) {
+ if ((Dirent->rec_len % 4) != 0) {
return FALSE;
}
@@ -160,17 +160,6 @@ Ext4RetrieveDirent (
return EFI_VOLUME_CORRUPTED;
}
- // Ignore names bigger than our limit.
-
- /* Note: I think having a limit is sane because:
- 1) It's nicer to work with.
- 2) Linux and a number of BSDs also have a filename limit of 255.
- */
- if (Entry->name_len > EXT4_NAME_MAX) {
- BlockOffset += Entry->rec_len;
- continue;
- }
-
// Unused entry
if (Entry->inode == 0) {
BlockOffset += Entry->rec_len;
@@ -548,20 +537,8 @@ Ext4RemoveDentry (
IN OUT EXT4_DENTRY *ToBeRemoved
)
{
- EXT4_DENTRY *D;
- LIST_ENTRY *Entry;
- LIST_ENTRY *NextEntry;
-
- BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {
- D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
-
- if (D == ToBeRemoved) {
- RemoveEntryList (Entry);
- return;
- }
- }
-
- DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for
dentry\n"));
+ ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent->Children));
+ RemoveEntryList (&ToBeRemoved->ListNode);
}
/**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index 43b9340d3956..2a4f5a7bd0ef 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -260,10 +260,12 @@ Ext4Stop (
EFI_DRIVER_BINDING_PROTOCOL gExt4BindingProtocol =
{
- Ext4IsBindingSupported,
- Ext4Bind,
- Ext4Stop,
- EXT4_DRIVER_VERSION
+ .Supported = Ext4IsBindingSupported,
+ .Start = Ext4Bind,
+ .Stop = Ext4Stop,
+ .Version = EXT4_DRIVER_VERSION,
+ .ImageHandle = NULL,
+ .DriverBindingHandle = NULL
};
/**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c
b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index c3874df71751..369879e07fe7 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
@@ -257,9 +257,11 @@ Ext4GetExtent (
return EFI_SUCCESS;
}
- if (!(Inode->i_flags & EXT4_EXTENTS_FL)) {
+ if ((Inode->i_flags & EXT4_EXTENTS_FL) == 0) {
// If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent using
the block map
- Status = Ext4GetBlocks (Partition, File, LogicalBlock, Extent);
+ // By specification files using block maps must be placed within the first
2^32 blocks
+ // of a filesystem, so we can safely cast LogicalBlock to uint32
+ Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Extent);
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91976): https://edk2.groups.io/g/devel/message/91976
Mute This Topic: https://groups.io/mt/92693958/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
