* [edk2-platforms][PATCH v2 0/1] Ext4Pkg: Code correctness and security improvements
@ 2022-07-20 5:36 Savva Mitrofanov
2022-07-20 5:36 ` [edk2-platforms][PATCH v2 1/1] " Savva Mitrofanov
0 siblings, 1 reply; 6+ messages in thread
From: Savva Mitrofanov @ 2022-07-20 5:36 UTC (permalink / raw)
To: devel; +Cc: Marvin Häuser, Pedro Falcato, Vitaly Cheptsov
Hi all,
This is second version of patch in which I squashed commits into one.
Here 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 (1):
Ext4Pkg: Code correctness and security improvements
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(-)
--
2.37.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
2022-07-20 5:36 [edk2-platforms][PATCH v2 0/1] Ext4Pkg: Code correctness and security improvements Savva Mitrofanov
@ 2022-07-20 5:36 ` Savva Mitrofanov
2022-07-20 17:54 ` Marvin Häuser
2022-07-24 16:59 ` Pedro Falcato
0 siblings, 2 replies; 6+ messages in thread
From: Savva Mitrofanov @ 2022-07-20 5:36 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 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.
- 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@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/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,
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
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
2022-07-20 5:36 ` [edk2-platforms][PATCH v2 1/1] " Savva Mitrofanov
@ 2022-07-20 17:54 ` Marvin Häuser
2022-07-29 7:47 ` Savva Mitrofanov
2022-07-24 16:59 ` Pedro Falcato
1 sibling, 1 reply; 6+ messages in thread
From: Marvin Häuser @ 2022-07-20 17:54 UTC (permalink / raw)
To: Savva Mitrofanov; +Cc: devel, Pedro Falcato, Vitaly Cheptsov
On 20. Jul 2022, at 07:36, Savva Mitrofanov <savvamtr@gmail.com> 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@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/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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
2022-07-20 5:36 ` [edk2-platforms][PATCH v2 1/1] " Savva Mitrofanov
2022-07-20 17:54 ` Marvin Häuser
@ 2022-07-24 16:59 ` Pedro Falcato
2022-07-29 10:09 ` Savva Mitrofanov
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Falcato @ 2022-07-24 16:59 UTC (permalink / raw)
To: Savva Mitrofanov
Cc: edk2-devel-groups-io, Marvin Häuser, Vitaly Cheptsov
[-- Attachment #1: Type: text/plain, Size: 11751 bytes --]
Hi Savva,
Could you please send a new version of the patch, with a proper encoding
(try 8bit encoding, quoted-printable is screwing up my git am) and
addressing Marvin's concerns?
Take the time and send a new version of the symlink patchset as well.
Thanks,
Pedro
On Wed, Jul 20, 2022 at 6:36 AM Savva Mitrofanov <savvamtr@gmail.com> 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.
> - 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@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/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,
> 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
> + 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
>
>
--
Pedro Falcato
[-- Attachment #2: Type: text/html, Size: 13784 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
2022-07-20 17:54 ` Marvin Häuser
@ 2022-07-29 7:47 ` Savva Mitrofanov
0 siblings, 0 replies; 6+ messages in thread
From: Savva Mitrofanov @ 2022-07-29 7:47 UTC (permalink / raw)
To: Marvin Häuser; +Cc: Pedro Falcato, vit9696, devel
Hi Marvin,
Thanks for comments!
> On 20 Jul 2022, at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> On 20. Jul 2022, at 07:36, Savva Mitrofanov <savvamtr@gmail.com> 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@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/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?
The idea to use such type naming belongs to Pedro, and in recent discussions he insisted on this, due EXT2_BLOCK_NR is more explicit than using UINT32 directly.
So, I'll keep it in v3.
>
>> 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.
As we discussed, in 64-bit fs we have 2^64 maximum blocks. But if we dig deeper, we find that files which are not using extents, but using blockmaps must be placed within the first 2^32 blocks of the filesystem by the spec, so in this case it is safe, and the comment about the 32-bit nature of ext2/3 is improper here.
We should rewrite it like this:
// If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent using the block map
// 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 +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
>>
>
I already took into account all the remarks in v3, and will send it soon.
Best regards,
Savva Mitrofanov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
2022-07-24 16:59 ` Pedro Falcato
@ 2022-07-29 10:09 ` Savva Mitrofanov
0 siblings, 0 replies; 6+ messages in thread
From: Savva Mitrofanov @ 2022-07-29 10:09 UTC (permalink / raw)
To: Pedro Falcato; +Cc: vit9696, Marvin Häuser, devel
[-- Attachment #1: Type: text/plain, Size: 12485 bytes --]
Hi Pedro,
Thanks for pointing this problem, it is really complicated. As we discussed in Element, EDKII uses CRLF in their codebase, so to send it correct we need to use quoted-printable transfer encoding to preserve CRLF, otherwise the patches will be damaged and force applying them will cause CRLF and LF to mix. By default git am tries to use LF encoding, so to change this behaviour it is suggested to use git am with --quoted-cr=nowarn and --keep-cr arguments.
Best regards,
Savva Mitrofanov
> On 24 Jul 2022, at 22:59, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Hi Savva,
>
> Could you please send a new version of the patch, with a proper encoding (try 8bit encoding, quoted-printable is screwing up my git am) and addressing Marvin's concerns?
>
> Take the time and send a new version of the symlink patchset as well.
>
> Thanks,
> Pedro
>
> On Wed, Jul 20, 2022 at 6:36 AM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> 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.
> - 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@posteo.de <mailto:mhaeuser@posteo.de>>
> Cc: Pedro Falcato <pedro.falcato@gmail.com <mailto:pedro.falcato@gmail.com>>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>>
> ---
> 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,
> 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
> + 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
>
>
>
> --
> Pedro Falcato
[-- Attachment #2: Type: text/html, Size: 20237 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-29 10:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-20 5:36 [edk2-platforms][PATCH v2 0/1] Ext4Pkg: Code correctness and security improvements Savva Mitrofanov
2022-07-20 5:36 ` [edk2-platforms][PATCH v2 1/1] " Savva Mitrofanov
2022-07-20 17:54 ` Marvin Häuser
2022-07-29 7:47 ` Savva Mitrofanov
2022-07-24 16:59 ` Pedro Falcato
2022-07-29 10:09 ` Savva Mitrofanov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox