From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web08.363.1658339652998516003 for ; Wed, 20 Jul 2022 10:54:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=DX+n3/l6; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 28267240026 for ; Wed, 20 Jul 2022 19:54:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1658339651; bh=mCk2RVze8hpgRBVYUfmYGsWfNp3vBXB6M+dgjEuYsiY=; h=Subject:From:Date:Cc:To:From; b=DX+n3/l6mLRlsoSQ0LUDE9XTNvl6/6uQLalRWFiNzLy5IPdNSnvhVOvoRpUpr7nCO NF5JZ15IobULV35k6OLPEus2ed2CzlLwEq81Vg5a4QqZIeTR0nG7AIVbh6T1gFXUN9 mS9u95+Bd4fu+F1iTHTbk27F65+JrrMSy8cU/JbsQ3VFKiBMrDwpUUYadT099ydJ9E /M6p9JSEr3ugF2sTBZLhQRJfA7qXH/Pfomuj3BxFLEX+tPK0+WxjYOLhggMIMGAjRa eD4AMSpH0O7oFJvxQZzzEpIzi0fwnoIp/oy313zvyNvb5yFrfKv4rqQPq0i/6u2SGE oDd2RoPwR029A== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Lp3H56PmFz6tmH; Wed, 20 Jul 2022 19:54:09 +0200 (CEST) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Subject: Re: [edk2-platforms][PATCH v2 1/1] Ext4Pkg: Code correctness and security improvements From: =?utf-8?Q?Marvin_H=C3=A4user?= In-Reply-To: <20220720053606.38975-2-savvamtr@gmail.com> Date: Wed, 20 Jul 2022 17:54:09 +0000 Cc: devel@edk2.groups.io, Pedro Falcato , Vitaly Cheptsov Message-Id: <59EBE1F6-9ACD-46FF-8F73-F11AC11DCB60@posteo.de> References: <20220720053606.38975-1-savvamtr@gmail.com> <20220720053606.38975-2-savvamtr@gmail.com> To: Savva Mitrofanov Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 20. Jul 2022, at 07:36, Savva Mitrofanov wrote: >=20 > 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. 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 >=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, This looks awkward, using an "EXT2" type in an "Ext4" function. Maybe = just use UINT32? > 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 The comment is misleading, because this is safe for EXT4 as well. > + 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