[edk2-devel] [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements
Marvin Häuser
mhaeuser at posteo.de
Wed Jul 20 17:54:09 UTC 2022
On 20. Jul 2022, at 07:36, Savva Mitrofanov <savvamtr at 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 at posteo.de>
> Cc: Pedro Falcato <pedro.falcato at gmail.com>
> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
> Signed-off-by: Savva Mitrofanov <savvamtr at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91666): https://edk2.groups.io/g/devel/message/91666
Mute This Topic: https://groups.io/mt/92531474/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list