From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vk1-f169.google.com (mail-vk1-f169.google.com [209.85.221.169]) by mx.groups.io with SMTP id smtpd.web10.18566.1658681980983437633 for ; Sun, 24 Jul 2022 09:59:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=BcHdW3aP; spf=pass (domain: gmail.com, ip: 209.85.221.169, mailfrom: pedro.falcato@gmail.com) Received: by mail-vk1-f169.google.com with SMTP id b81so4169675vkf.1 for ; Sun, 24 Jul 2022 09:59:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=UA2jn6rqwOtCr8sQVtCB8aEzMWSHwjXhvhRjdExoD90=; b=BcHdW3aPhOJMpPXKUxrm/OgtW7HY6QQ0G4tiPau/s33y1/UY9cuxtckWPL4CPCSFZa 7xpsVQHKPYzqc/anuwiFsdftrm0IGTFdJKUuyz0ycLF75W2aGof/K/BfiF+Ou1zOyCIZ q12nrn715PV/kZvewkNYCTxqwYH3pZ5Vv/e/cUoXCUb8mEFBEUF8Q+6zYbZOxJO0jqBX jg0xD6bNqNlMJgK671gR+k56HIXi7/joXC3aKzWw5xmTyOOFkI+uyyGWyfxbyhYc2CVe K4UBmzm5fJ5N0seD1QLXsUT3zvyRIPXld9q3XT64EbokHQBRNy/6mfN8CUp/zlRqQ79e 0Dag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=UA2jn6rqwOtCr8sQVtCB8aEzMWSHwjXhvhRjdExoD90=; b=TzhTN16g2MQsj/WEPPRvRl+YaI5eRkU4YEpgxzRfdIWOpuzf1Tw9gPCIiL+tKfx7kZ zo1rDypENDBTNpcPFrbZiJVRo4ogPAc6P8U19LINS1iAoFnZo8AQp9yiX/ejtlexChnq jZA0VBATTDVq388h5pgzKgsx6H/3cSb1+tlIFHVIkP0KvLjLNzhJ8BYy8ezmhOVTZvPj czEIv2GFFlc3HjuhlHiN8MFQp4mLwCh5iU0z6yR8LuhqdAneHl3dXWJ41SLZ4znajGdP I4lFAzMqLYYvElCBNCkf/MxXaSoQLqzabdzRGiSFccgK3zKtVUX/ZALIqq+h9EWy6sS0 2mhQ== X-Gm-Message-State: AJIora9GDCPSdvzS8aphvEX4oRc8tgAgsabEW4+8ue5n6MdaaSYXK/mM PU16a6BLs0Y1qsoFBJlXBEly3crruoLabRecGwM= X-Google-Smtp-Source: AGRyM1uqokwMMGQAQ+6XOVKfttCZKDXm6PdmFxSc7FuVMSijSOvI8VquvaH7lqDts8P55jET8hhMPyvS22B8ErIgj7Y= X-Received: by 2002:ac5:cb69:0:b0:376:f2f:563e with SMTP id l9-20020ac5cb69000000b003760f2f563emr2319934vkn.23.1658681979901; Sun, 24 Jul 2022 09:59:39 -0700 (PDT) MIME-Version: 1.0 References: <20220720053606.38975-1-savvamtr@gmail.com> <20220720053606.38975-2-savvamtr@gmail.com> In-Reply-To: <20220720053606.38975-2-savvamtr@gmail.com> From: "Pedro Falcato" Date: Sun, 24 Jul 2022 17:59:28 +0100 Message-ID: Subject: Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements To: Savva Mitrofanov Cc: edk2-devel-groups-io , =?UTF-8?Q?Marvin_H=C3=A4user?= , Vitaly Cheptsov Content-Type: multipart/alternative; boundary="000000000000351b5805e48ffe6b" --000000000000351b5805e48ffe6b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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=C3=A4user > Cc: Pedro Falcato > Cc: Vitaly Cheptsov > Signed-off-by: Savva Mitrofanov > --- > 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 =3D Ext4GetBlockPath (Partition, LogicalBlock, BlockPa= th); > > - if (BlockPathLength =3D=3D (UINTN)-1) { > + if (BlockPathLength - 1 =3D=3D 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) !=3D 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 +=3D Entry->rec_len; > - continue; > - } > - > // Unused entry > if (Entry->inode =3D=3D 0) { > BlockOffset +=3D 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 =3D EXT4_DENTRY_FROM_DENTRY_LIST (Entry); > - > - if (D =3D=3D 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 =3D > { > - Ext4IsBindingSupported, > - Ext4Bind, > - Ext4Stop, > - EXT4_DRIVER_VERSION > + .Supported =3D Ext4IsBindingSupported, > + .Start =3D Ext4Bind, > + .Stop =3D Ext4Stop, > + .Version =3D EXT4_DRIVER_VERSION, > + .ImageHandle =3D NULL, > + .DriverBindingHandle =3D 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 =3D Ext4GetBlocks (Partition, File, LogicalBlock, Extent); > + // We cast LogicalBlock to UINT32, considering ext2/3 are 32-bit > + Status =3D Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, > Extent); > > if (!EFI_ERROR (Status)) { > Ext4CacheExtents (File, Extent, 1); > @@ -420,7 +421,7 @@ Ext4ExtentsMapKeyCompare ( > Extent =3D UserStruct; > Block =3D (UINT32)(UINTN)StandaloneKey; > > - if ((Block >=3D Extent->ee_block) && (Block < Extent->ee_block + > Ext4GetExtentLength (Extent))) { > + if ((Block >=3D 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 =3D (Ext4GetExtentLength (&Extent) * Partition->BlockSiz= e) > - HoleOff; > } > > - WasRead =3D HoleLen > RemainingRead ? RemainingRead : HoleLen; > + WasRead =3D HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleL= en; > // 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 =3D MultU64x32 ( > LShiftU64 (Extent.ee_start_hi, 32) | > @@ -431,7 +431,7 @@ Ext4FileCreateTime ( > Inode =3D 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) !=3D 0) || (Sb->s_desc_size < > EXT4_64BIT_BLOCK_DESC_SIZE)) { > + return EFI_VOLUME_CORRUPTED; > + } > + > Partition->DescSize =3D Sb->s_desc_size; > } else { > Partition->DescSize =3D 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 > > --=20 Pedro Falcato --000000000000351b5805e48ffe6b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Savva,

Could you please s= end 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 conc= erns?

Take the time and send a new version of the = symlink patchset as well.

Thanks,
Pedro<= br>

On Wed, Jul 20, 2022 at 6:36 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
This changes tends to improve se= curity 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=C3=A4user <mhaeuser@posteo.de>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
---
=C2=A0Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h=C2=A0 =C2=A0|=C2=A0 3 +-
=C2=A0Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h=C2=A0 =C2=A0 |=C2=A0 2 +-
=C2=A0Features/Ext4Pkg/Ext4Dxe/BlockMap.c=C2=A0 =C2=A0| 18 ++++++++----
=C2=A0Features/Ext4Pkg/Ext4Dxe/Directory.c=C2=A0 | 29 ++------------------<= br> =C2=A0Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c=C2=A0 =C2=A0 | 10 ++++---
=C2=A0Features/Ext4Pkg/Ext4Dxe/Extents.c=C2=A0 =C2=A0 |=C2=A0 5 ++--
=C2=A0Features/Ext4Pkg/Ext4Dxe/Inode.c=C2=A0 =C2=A0 =C2=A0 |=C2=A0 8 +++---=
=C2=A0Features/Ext4Pkg/Ext4Dxe/Superblock.c | 13 +++++----
=C2=A08 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 (
=C2=A0#define EXT4_TIND_BLOCK=C2=A0 14
=C2=A0#define EXT4_NR_BLOCKS=C2=A0 =C2=A015

-#define EXT4_GOOD_OLD_INODE_SIZE=C2=A0 128
+#define EXT4_GOOD_OLD_INODE_SIZE=C2=A0 128U

=C2=A0typedef struct _Ext4_I_OSD2_Linux {
=C2=A0 =C2=A0UINT16=C2=A0 =C2=A0 l_i_blocks_high;
@@ -463,6 +463,7 @@ typedef struct {
=C2=A0#define EXT4_EXTENT_MAX_INITIALIZED=C2=A0 (1 << 15)

=C2=A0typedef UINT64=C2=A0 EXT4_BLOCK_NR;
+typedef UINT32=C2=A0 EXT2_BLOCK_NR;
=C2=A0typedef UINT32=C2=A0 EXT4_INO_NR;

=C2=A0// 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
=C2=A0Ext4GetBlocks (
=C2=A0 =C2=A0IN=C2=A0 EXT4_PARTITION=C2=A0 *Partition,
=C2=A0 =C2=A0IN=C2=A0 EXT4_FILE=C2=A0 =C2=A0 =C2=A0 =C2=A0*File,
-=C2=A0 IN=C2=A0 EXT4_BLOCK_NR=C2=A0 =C2=A0LogicalBlock,
+=C2=A0 IN=C2=A0 EXT2_BLOCK_NR=C2=A0 =C2=A0LogicalBlock,
=C2=A0 =C2=A0OUT EXT4_EXTENT=C2=A0 =C2=A0 =C2=A0*Extent
=C2=A0 =C2=A0);

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
=C2=A0Ext4GetBlockPath (
=C2=A0 =C2=A0IN=C2=A0 CONST EXT4_PARTITION=C2=A0 *Partition,
=C2=A0 =C2=A0IN=C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 LogicalBlock,
-=C2=A0 OUT EXT4_BLOCK_NR=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BlockPath[EXT4_M= AX_BLOCK_PATH]
+=C2=A0 OUT EXT2_BLOCK_NR=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BlockPath[EXT4_M= AX_BLOCK_PATH]
=C2=A0 =C2=A0)
=C2=A0{
=C2=A0 =C2=A0// The logic behind the block map is very much like a page tab= le
@@ -123,7 +123,7 @@ Ext4GetBlockPath (
=C2=A0 =C2=A0 =C2=A0 =C2=A0break;
=C2=A0 =C2=A0 =C2=A0default:
=C2=A0 =C2=A0 =C2=A0 =C2=A0// EXT4_TYPE_BAD_BLOCK
-=C2=A0 =C2=A0 =C2=A0 return -1;
+=C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0}

=C2=A0 =C2=A0return Type + 1;
@@ -213,12 +213,12 @@ EFI_STATUS
=C2=A0Ext4GetBlocks (
=C2=A0 =C2=A0IN=C2=A0 EXT4_PARTITION=C2=A0 *Partition,
=C2=A0 =C2=A0IN=C2=A0 EXT4_FILE=C2=A0 =C2=A0 =C2=A0 =C2=A0*File,
-=C2=A0 IN=C2=A0 EXT4_BLOCK_NR=C2=A0 =C2=A0LogicalBlock,
+=C2=A0 IN=C2=A0 EXT2_BLOCK_NR=C2=A0 =C2=A0LogicalBlock,
=C2=A0 =C2=A0OUT EXT4_EXTENT=C2=A0 =C2=A0 =C2=A0*Extent
=C2=A0 =C2=A0)
=C2=A0{
=C2=A0 =C2=A0EXT4_INODE=C2=A0 =C2=A0 =C2=A0*Inode;
-=C2=A0 EXT4_BLOCK_NR=C2=A0 BlockPath[EXT4_MAX_BLOCK_PATH];
+=C2=A0 EXT2_BLOCK_NR=C2=A0 BlockPath[EXT4_MAX_BLOCK_PATH];
=C2=A0 =C2=A0UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BlockPathLength;
=C2=A0 =C2=A0UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Index;
=C2=A0 =C2=A0UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*Buffer;
@@ -230,7 +230,7 @@ Ext4GetBlocks (

=C2=A0 =C2=A0BlockPathLength =3D Ext4GetBlockPath (Partition, LogicalBlock,= BlockPath);

-=C2=A0 if (BlockPathLength =3D=3D (UINTN)-1) {
+=C2=A0 if (BlockPathLength - 1 =3D=3D EXT4_TYPE_BAD_BLOCK) {
=C2=A0 =C2=A0 =C2=A0// Bad logical block (out of range)
=C2=A0 =C2=A0 =C2=A0return EFI_NO_MAPPING;
=C2=A0 =C2=A0}
@@ -272,7 +272,13 @@ Ext4GetBlocks (
=C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0}

-=C2=A0 Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof (= UINT32), BlockPath[BlockPathLength - 1], Extent);
+=C2=A0 Ext4GetExtentInBlockMap (
+=C2=A0 =C2=A0 Buffer,
+=C2=A0 =C2=A0 Partition->BlockSize / sizeof (UINT32),
+=C2=A0 =C2=A0 BlockPath[BlockPathLength - 1],
+=C2=A0 =C2=A0 Extent
+=C2=A0 =C2=A0 );
+
=C2=A0 =C2=A0FreePool (Buffer);

=C2=A0 =C2=A0return EFI_SUCCESS;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dx= e/Directory.c
index 682f66ad5525..4441e6d192b6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -74,7 +74,7 @@ Ext4ValidDirent (
=C2=A0 =C2=A0}

=C2=A0 =C2=A0// Dirent sizes need to be 4 byte aligned
-=C2=A0 if (Dirent->rec_len % 4) {
+=C2=A0 if ((Dirent->rec_len % 4) !=3D 0) {
=C2=A0 =C2=A0 =C2=A0return FALSE;
=C2=A0 =C2=A0}

@@ -160,17 +160,6 @@ Ext4RetrieveDirent (
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return EFI_VOLUME_CORRUPTED;
=C2=A0 =C2=A0 =C2=A0 =C2=A0}

-=C2=A0 =C2=A0 =C2=A0 // Ignore names bigger than our limit.
-
-=C2=A0 =C2=A0 =C2=A0 /* Note: I think having a limit is sane because:
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 1) It's nicer to work with.
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 2) Linux and a number of BSDs also have a file= name limit of 255.
-=C2=A0 =C2=A0 =C2=A0 */
-=C2=A0 =C2=A0 =C2=A0 if (Entry->name_len > EXT4_NAME_MAX) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 BlockOffset +=3D Entry->rec_len;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 continue;
-=C2=A0 =C2=A0 =C2=A0 }
-
=C2=A0 =C2=A0 =C2=A0 =C2=A0// Unused entry
=C2=A0 =C2=A0 =C2=A0 =C2=A0if (Entry->inode =3D=3D 0) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BlockOffset +=3D Entry->rec_len;
@@ -548,20 +537,8 @@ Ext4RemoveDentry (
=C2=A0 =C2=A0IN OUT EXT4_DENTRY=C2=A0 *ToBeRemoved
=C2=A0 =C2=A0)
=C2=A0{
-=C2=A0 EXT4_DENTRY=C2=A0 *D;
-=C2=A0 LIST_ENTRY=C2=A0 =C2=A0*Entry;
-=C2=A0 LIST_ENTRY=C2=A0 =C2=A0*NextEntry;
-
-=C2=A0 BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children= ) {
-=C2=A0 =C2=A0 D =3D EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
-
-=C2=A0 =C2=A0 if (D =3D=3D ToBeRemoved) {
-=C2=A0 =C2=A0 =C2=A0 RemoveEntryList (Entry);
-=C2=A0 =C2=A0 =C2=A0 return;
-=C2=A0 =C2=A0 }
-=C2=A0 }
-
-=C2=A0 DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the= asked-for dentry\n"));
+=C2=A0 ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent-&g= t;Children));
+=C2=A0 RemoveEntryList (&ToBeRemoved->ListNode);
=C2=A0}

=C2=A0/**
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 (

=C2=A0EFI_DRIVER_BINDING_PROTOCOL=C2=A0 gExt4BindingProtocol =3D
=C2=A0{
-=C2=A0 Ext4IsBindingSupported,
-=C2=A0 Ext4Bind,
-=C2=A0 Ext4Stop,
-=C2=A0 EXT4_DRIVER_VERSION
+=C2=A0 .Supported=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D Ext4IsBindin= gSupported,
+=C2=A0 .Start=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D Ex= t4Bind,
+=C2=A0 .Stop=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D Ex= t4Stop,
+=C2=A0 .Version=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D EXT4_DR= IVER_VERSION,
+=C2=A0 .ImageHandle=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D NULL,
+=C2=A0 .DriverBindingHandle =3D NULL
=C2=A0};

=C2=A0/**
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 (

=C2=A0 =C2=A0if (!(Inode->i_flags & EXT4_EXTENTS_FL)) {
=C2=A0 =C2=A0 =C2=A0// If this is an older ext2/ext3 filesystem, emulate Ex= t4GetExtent using the block map
-=C2=A0 =C2=A0 Status =3D Ext4GetBlocks (Partition, File, LogicalBlock, Ext= ent);
+=C2=A0 =C2=A0 // We cast LogicalBlock to UINT32, considering ext2/3 are 32= -bit
+=C2=A0 =C2=A0 Status =3D Ext4GetBlocks (Partition, File, (UINT32)LogicalBl= ock, Extent);

=C2=A0 =C2=A0 =C2=A0if (!EFI_ERROR (Status)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0Ext4CacheExtents (File, Extent, 1);
@@ -420,7 +421,7 @@ Ext4ExtentsMapKeyCompare (
=C2=A0 =C2=A0Extent =3D UserStruct;
=C2=A0 =C2=A0Block=C2=A0 =3D (UINT32)(UINTN)StandaloneKey;

-=C2=A0 if ((Block >=3D Extent->ee_block) && (Block < Exte= nt->ee_block + Ext4GetExtentLength (Extent))) {
+=C2=A0 if ((Block >=3D Extent->ee_block) && (Block - Extent-= >ee_block < Ext4GetExtentLength (Extent))) {
=C2=A0 =C2=A0 =C2=A0return 0;
=C2=A0 =C2=A0}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/In= ode.c
index 831f5946e870..4860cf576377 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -100,7 +100,7 @@ Ext4Read (
=C2=A0 =C2=A0EFI_STATUS=C2=A0 =C2=A0Status;
=C2=A0 =C2=A0BOOLEAN=C2=A0 =C2=A0 =C2=A0 HasBackingExtent;
=C2=A0 =C2=A0UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0HoleOff;
-=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 HoleLen;
+=C2=A0 UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0HoleLen;
=C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0ExtentStartBytes;
=C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0ExtentLengthBytes;
=C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0ExtentLogicalBytes;
@@ -155,10 +155,10 @@ Ext4Read (
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0HoleLen =3D (Ext4GetExtentLength (&Ex= tent) * Partition->BlockSize) - HoleOff;
=C2=A0 =C2=A0 =C2=A0 =C2=A0}

-=C2=A0 =C2=A0 =C2=A0 WasRead =3D HoleLen > RemainingRead ? RemainingRea= d : HoleLen;
+=C2=A0 =C2=A0 =C2=A0 WasRead =3D HoleLen > RemainingRead ? RemainingRea= d : (UINTN)HoleLen;
=C2=A0 =C2=A0 =C2=A0 =C2=A0// Potential improvement: In the future, we coul= d get the file hole's total
=C2=A0 =C2=A0 =C2=A0 =C2=A0// size and memset all that
-=C2=A0 =C2=A0 =C2=A0 SetMem (Buffer, WasRead, 0);
+=C2=A0 =C2=A0 =C2=A0 ZeroMem (Buffer, WasRead);
=C2=A0 =C2=A0 =C2=A0} else {
=C2=A0 =C2=A0 =C2=A0 =C2=A0ExtentStartBytes =3D MultU64x32 (
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 LShiftU64 (Extent.ee_start_hi, 32) |
@@ -431,7 +431,7 @@ Ext4FileCreateTime (
=C2=A0 =C2=A0Inode =3D File->Inode;

=C2=A0 =C2=A0if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) {
-=C2=A0 =C2=A0 SetMem (Time, sizeof (EFI_TIME), 0);
+=C2=A0 =C2=A0 ZeroMem (Time, sizeof (EFI_TIME));
=C2=A0 =C2=A0 =C2=A0return;
=C2=A0 =C2=A0}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4D= xe/Superblock.c
index 47fc3a65507a..a57728a9abe6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -257,16 +257,17 @@ Ext4OpenSuperblock (
=C2=A0 =C2=A0 =C2=A0));

=C2=A0 =C2=A0if (EXT4_IS_64_BIT (Partition)) {
+=C2=A0 =C2=A0 // s_desc_size should be 4 byte aligned and
+=C2=A0 =C2=A0 // 64 bit filesystems need DescSize to be 64 bytes
+=C2=A0 =C2=A0 if (((Sb->s_desc_size % 4) !=3D 0) || (Sb->s_desc_size= < EXT4_64BIT_BLOCK_DESC_SIZE)) {
+=C2=A0 =C2=A0 =C2=A0 return EFI_VOLUME_CORRUPTED;
+=C2=A0 =C2=A0 }
+
=C2=A0 =C2=A0 =C2=A0Partition->DescSize =3D Sb->s_desc_size;
=C2=A0 =C2=A0} else {
=C2=A0 =C2=A0 =C2=A0Partition->DescSize =3D EXT4_OLD_BLOCK_DESC_SIZE; =C2=A0 =C2=A0}

-=C2=A0 if ((Partition->DescSize < EXT4_64BIT_BLOCK_DESC_SIZE) &&= amp; EXT4_IS_64_BIT (Partition)) {
-=C2=A0 =C2=A0 // 64 bit filesystems need DescSize to be 64 bytes
-=C2=A0 =C2=A0 return EFI_VOLUME_CORRUPTED;
-=C2=A0 }
-
=C2=A0 =C2=A0if (!Ext4VerifySuperblockChecksum (Partition, Sb)) {
=C2=A0 =C2=A0 =C2=A0DEBUG ((DEBUG_ERROR, "[ext4] Bad superblock checks= um %lx\n", Ext4CalculateSuperblockChecksum (Partition, Sb)));
=C2=A0 =C2=A0 =C2=A0return EFI_VOLUME_CORRUPTED;
@@ -342,7 +343,7 @@ Ext4CalculateChecksum (
=C2=A0 =C2=A0 =C2=A0 =C2=A0// For some reason, EXT4 really likes non-invert= ed CRC32C checksums, so we stick to that here.
=C2=A0 =C2=A0 =C2=A0 =C2=A0return ~CalculateCrc32c(Buffer, Length, ~Initial= Value);
=C2=A0 =C2=A0 =C2=A0default:
-=C2=A0 =C2=A0 =C2=A0 UNREACHABLE ();
+=C2=A0 =C2=A0 =C2=A0 ASSERT (FALSE);
=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
=C2=A0 =C2=A0}
=C2=A0}
--
2.37.0



--
Pedro Falcato
--000000000000351b5805e48ffe6b--