From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by mx.groups.io with SMTP id smtpd.web08.6325.1659089386269022743 for ; Fri, 29 Jul 2022 03:09:46 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=CVLLXvFb; spf=pass (domain: gmail.com, ip: 209.85.208.181, mailfrom: savvamtr@gmail.com) Received: by mail-lj1-f181.google.com with SMTP id e11so4649825ljl.4 for ; Fri, 29 Jul 2022 03:09:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=uIpgNwYQGWprKBv7Bg3ARyiCROj2Bb4KjlMcOfEM85Y=; b=CVLLXvFbM6FnniZibwLBOh7oWTfFWlJs2oSociktWqD6DYleVl2QTdrmpBwmfM3he+ Sn0MxDGKaprV00E81XBgjxdphr4yJrNIZDD68R8UPesp9vwjqH3uBxPWHC6ebKKK/hlN vQNRML/W5eGMDWX2u8GVJYxd4lVa11n0kCs6h62wymxO8ze77nd4vsQTQT2d/+VFWaNv iH7Ko33SVwUeqQshjwLOx9a9dbOX2JVbKR1v+BLSCftEYKh2ptGT0NeeI9setdZBoY18 IhiTLDq36A3ycR+Y2VLIVgk5LIP346lEfwhYC32a41uUoj7JImOllNSOYsvneF2SyRVH qqbw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=uIpgNwYQGWprKBv7Bg3ARyiCROj2Bb4KjlMcOfEM85Y=; b=t1X2kZXrCBWtr0G5G9kD9gvskauGX6unWUdLWH9pprHYEKYuhfQEJ24sEaAbTrgP4w 5wLVD+G+SL6JHEYzycQlDpIdWC51wAPBzbM+5GhiQTQEqABpjLgeAP5C2PJYcOKZmLam dkirqVJD2+91nRHpBRCyxTzTHxlOFqO59KqyakYR0QUnEaMHYT4a/p815EBLq6a9OqLD +jM8J75QsENRiih3N2l3wgIdpUAF6JrM/nAtxIuLrKxrLowKIFGWpC+Bw/FLXZB4PaiT qhszqy/PntnQlXVgv1fh9hXeAyez4MBjRXEn7Qs8p5sP5yCohE13mG/gnCNkEzrRo8/j 2gEg== X-Gm-Message-State: AJIora+jhMfLfD8Fk9IPaZUVcR0fFWHfsBem4A40L6YjFmkakU1dSjDt kNp93/4IFLicXRiY0cVisaA= X-Google-Smtp-Source: AGRyM1s0XNHJ67GgsMHDr5YOUTsmehzdnG3cAEozaiSgwDhRTDQnSc8PfoIUEURDxvZ71Co+/wIBQw== X-Received: by 2002:a2e:934f:0:b0:250:a7bc:2b8f with SMTP id m15-20020a2e934f000000b00250a7bc2b8fmr872755ljh.512.1659089384289; Fri, 29 Jul 2022 03:09:44 -0700 (PDT) Return-Path: Received: from smtpclient.apple ([207.180.219.167]) by smtp.gmail.com with ESMTPSA id v23-20020ac25597000000b0047f72bf3c32sm607553lfg.77.2022.07.29.03.09.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 29 Jul 2022 03:09:43 -0700 (PDT) From: "Savva Mitrofanov" Message-Id: <1812C02F-1381-4643-9DB0-625A15377A92@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\)) Subject: Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements Date: Fri, 29 Jul 2022 16:09:41 +0600 In-Reply-To: Cc: vit9696@protonmail.com, =?utf-8?Q?Marvin_H=C3=A4user?= , devel@edk2.groups.io To: Pedro Falcato References: <20220720053606.38975-1-savvamtr@gmail.com> <20220720053606.38975-2-savvamtr@gmail.com> X-Mailer: Apple Mail (2.3654.120.0.1.13) Content-Type: multipart/alternative; boundary="Apple-Mail=_D79FAC0E-3692-4DD4-8AF5-FC19CF551591" --Apple-Mail=_D79FAC0E-3692-4DD4-8AF5-FC19CF551591 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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=3Dnowarn and --keep-cr arguments. Best regards, Savva Mitrofanov > On 24 Jul 2022, at 22:59, Pedro Falcato = wrote: >=20 > Hi Savva, >=20 > 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? >=20 > Take the time and send a new version of the symlink patchset as well. >=20 > Thanks, > Pedro >=20 > 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. >=20 > - 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 >=20 > 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(-) >=20 > 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 >=20 > -#define EXT4_GOOD_OLD_INODE_SIZE 128 > +#define EXT4_GOOD_OLD_INODE_SIZE 128U >=20 > typedef struct _Ext4_I_OSD2_Linux { > UINT16 l_i_blocks_high; > @@ -463,6 +463,7 @@ typedef struct { > #define EXT4_EXTENT_MAX_INITIALIZED (1 << 15) >=20 > typedef UINT64 EXT4_BLOCK_NR; > +typedef UINT32 EXT2_BLOCK_NR; > typedef UINT32 EXT4_INO_NR; >=20 > // 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 > ); >=20 > 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; > } >=20 > 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 ( >=20 > BlockPathLength =3D Ext4GetBlockPath (Partition, LogicalBlock, = BlockPath); >=20 > - 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 ( > } > } >=20 > - Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof = (UINT32), BlockPath[BlockPathLength - 1], Extent); > + Ext4GetExtentInBlockMap ( > + Buffer, > + Partition->BlockSize / sizeof (UINT32), > + BlockPath[BlockPathLength - 1], > + Extent > + ); > + > FreePool (Buffer); >=20 > 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 ( > } >=20 > // Dirent sizes need to be 4 byte aligned > - if (Dirent->rec_len % 4) { > + if ((Dirent->rec_len % 4) !=3D 0) { > return FALSE; > } >=20 > @@ -160,17 +160,6 @@ Ext4RetrieveDirent ( > return EFI_VOLUME_CORRUPTED; > } >=20 > - // 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); > } >=20 > /** > 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 ( >=20 > 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 > }; >=20 > /** > 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 ( >=20 > 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); >=20 > if (!EFI_ERROR (Status)) { > Ext4CacheExtents (File, Extent, 1); > @@ -420,7 +421,7 @@ Ext4ExtentsMapKeyCompare ( > Extent =3D UserStruct; > Block =3D (UINT32)(UINTN)StandaloneKey; >=20 > - 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; > } >=20 > 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->BlockSize) - HoleOff; > } >=20 > - WasRead =3D HoleLen > RemainingRead ? RemainingRead : HoleLen; > + WasRead =3D 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 =3D MultU64x32 ( > LShiftU64 (Extent.ee_start_hi, 32) | > @@ -431,7 +431,7 @@ Ext4FileCreateTime ( > Inode =3D File->Inode; >=20 > if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) { > - SetMem (Time, sizeof (EFI_TIME), 0); > + ZeroMem (Time, sizeof (EFI_TIME)); > return; > } >=20 > 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 ( > )); >=20 > 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; > } >=20 > - 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; > } > } > --=20 > 2.37.0 >=20 >=20 >=20 > --=20 > Pedro Falcato --Apple-Mail=_D79FAC0E-3692-4DD4-8AF5-FC19CF551591 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
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=3Dnowarn 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> 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 <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 =3D Ext4GetBlockPath (Partition, = LogicalBlock, BlockPath);

-  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->BlockSize) - HoleOff;
       }

-      WasRead =3D HoleLen > RemainingRead ? = RemainingRead : HoleLen;
+      WasRead =3D 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 =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



--
Pedro Falcato

= --Apple-Mail=_D79FAC0E-3692-4DD4-8AF5-FC19CF551591--