From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com [209.85.167.42]) by mx.groups.io with SMTP id smtpd.web09.5533.1659080863352168231 for ; Fri, 29 Jul 2022 00:47:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=MkZjRC+L; spf=pass (domain: gmail.com, ip: 209.85.167.42, mailfrom: savvamtr@gmail.com) Received: by mail-lf1-f42.google.com with SMTP id w15so6145745lft.11 for ; Fri, 29 Jul 2022 00:47:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=OspQFkKMtccg6GtA3y9K32OrmypztzcULMMyEW3JdQ0=; b=MkZjRC+LWkUYxuUZtGiY9ElFVhtBZ453R88OmamAP9Z7d3iT7CVz2smstXm/wJjQk0 CMPgsIcAgNlT+d47cHspZQeBeq5J0h7HNBFZFMbdF/UedzqRsB3KbWSiNWmTZ7PJ0cMu jpA5KM8mJVq+EBag3XpAQLENC34VQQbfYdJecJ4TUM/NoRRMg60mrJnMBWGZc1wcacQ/ QAgYMjJ4LBdR2EEgRjf/bQ85CVc7e7RLv9QUZac1cpFMNpxgjnrDl89xVhmpo0b/E1hS wXGZ3XJ7hJ/1sJHnkyJxE3k1wLJOVNmXXCNfUWdQNnbFURPUQu8KgPD88RsD36YdOj3e yoTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=OspQFkKMtccg6GtA3y9K32OrmypztzcULMMyEW3JdQ0=; b=qKES8ORinVFThJ6J1c6qzFF1RD9tHHXjRrO6zyldQM3KJ7SeWjNeRjJKJGx7tZ7eP0 ws/NQNBAHYWHDUM+h/SfIa8As1ISGQPSSgS90PtpO6gsWdUSHUjkrbhze9VtYT5RT40S hQZRtRMNF8GujuxE1SvnbgohbY3HBikJEOQ6Z82YFqMJdEBexEETnT7ylYRBTKbGtotW uFdJriqt64g9xg67kHatBmyVeeSeijwGfZe122renlEZVcQLfSar0DBmf/3weE6R32jk GZVMeA73sSjJhShEDsbQRkmm05TdWFKtSQnhhJjShD93UQ+/82Z+hMF1JR8K39hKlvQ4 sBuA== X-Gm-Message-State: AJIora9BgDYpc8AvJmxSGFN4Msk2mibqJsK/r41en8C624cF6fQsP9LG sahQu1EGve1Cqjb3WmdjYMY= X-Google-Smtp-Source: AGRyM1tQzFjI4Ko9vbA2Qe8KtQRA4sxN/D+EciGyLkEQJCujNViseSpiK1xtQLKIPIxITJVgdA/z9g== X-Received: by 2002:a05:6512:3406:b0:489:deab:e516 with SMTP id i6-20020a056512340600b00489deabe516mr758573lfr.609.1659080861219; Fri, 29 Jul 2022 00:47:41 -0700 (PDT) Return-Path: Received: from smtpclient.apple ([207.180.219.167]) by smtp.gmail.com with ESMTPSA id c4-20020a197604000000b0048ad126ce34sm367632lff.290.2022.07.29.00.47.39 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 29 Jul 2022 00:47:40 -0700 (PDT) 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 From: "Savva Mitrofanov" In-Reply-To: <59EBE1F6-9ACD-46FF-8F73-F11AC11DCB60@posteo.de> Date: Fri, 29 Jul 2022 13:47:37 +0600 Cc: Pedro Falcato , vit9696@protonmail.com, devel@edk2.groups.io Message-Id: <28B7ED97-7613-45DB-ADF5-5933FAF7CC08@gmail.com> References: <20220720053606.38975-1-savvamtr@gmail.com> <20220720053606.38975-2-savvamtr@gmail.com> <59EBE1F6-9ACD-46FF-8F73-F11AC11DCB60@posteo.de> To: =?utf-8?Q?Marvin_H=C3=A4user?= X-Mailer: Apple Mail (2.3654.120.0.1.13) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Marvin, Thanks for comments!=20 > On 20 Jul 2022, at 23:54, Marvin H=C3=A4user = wrote: >=20 > 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. >=20 > In my opinion, just "to prevent truncation". >=20 >> - 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, >=20 > 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. >=20 >> 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 >=20 > 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 >=20 >> + 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 I already took into account all the remarks in v3, and will send it = soon. Best regards, Savva Mitrofanov