From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by mx.groups.io with SMTP id smtpd.web11.317.1659110068306794314 for ; Fri, 29 Jul 2022 08:54:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=Gb/E5cGq; spf=pass (domain: gmail.com, ip: 209.85.208.172, mailfrom: savvamtr@gmail.com) Received: by mail-lj1-f172.google.com with SMTP id bx8so3195571ljb.3 for ; Fri, 29 Jul 2022 08:54:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=5v0vjpvUf57/S8erSVrQ9dcV8zcifCtb2mQTeVKTBf0=; b=Gb/E5cGq7tN31G8P6Atvr2v2ftqtYTkARiFy9hGA1K+cH96173kB0bhora6WW18lyp s+Qb3qXDoU9SIX+idywFfbPNU1j+uXdQX85Hhawjk3K4iftTQKkDUpZeIO0D7Xl3sRu6 xWdWFsZEG57eK+FyZBLJ7FWhwgje6aL4GAHbsqM7X0Pja6Gy2YGnBkYL3VOY9q9XVwNa OXljfaQYXzpoyyLHjdlYK8z6aM07k8usQ/HnJkIqtdqsil4r1clLA7xuS2/SMa1tUbGE amiGpRSbvmL3FlebyoSrG41kjuNbrXrPe3Nlmx4s2GE+/QVeeGEvjjJsLltI9MJySZaf dtQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=5v0vjpvUf57/S8erSVrQ9dcV8zcifCtb2mQTeVKTBf0=; b=NMgVyoJP8+d1EcTs77lM0DpENKkxGFt7TqGVsgj7UufsLjx27YmEm6rKzhhExwjqxX 3calUE/UAZ3+7ZDtnzBzqMigXWJ4XBcGTv1eZ7qOmM4AfNjj7aQySRIn3jceztsFJVEm Or6ucWJlfFnIn9muw7yL1mRD0K3C+YJ7DORLSEuQ3lGDmaEpSrXjH3Nia0qiDk0SA9lb M7DjgCzsHBn198+FnKP2a35WvtWE6m+htWgvS7JWk0TG58lQBGebl7jf6KBK7DvvLXlR t0cKe1kzGAYBF3sdCXtFtyGaB3PWPRh2XdOXQ0nMgGjlAviMer1emIq+cLFC62z7RrTv /ZGA== X-Gm-Message-State: AJIora99i6eKqPF+RIJL4255eTHCVey9T+QFH+TQsgdCbheAwIb+EzF5 iYc/uiUlLGMcsjt022bb09+VWV8cDgBnZ6DGuZc= X-Google-Smtp-Source: AGRyM1tEMNuXI/IeI2TQQStpVJV/bJAc1z4RVxnDdxkamza1h5MHlaDg1gkIt28Msbo6+g/O+yzpdw== X-Received: by 2002:a2e:9584:0:b0:25d:ff77:da3d with SMTP id w4-20020a2e9584000000b0025dff77da3dmr1287905ljh.383.1659110066056; Fri, 29 Jul 2022 08:54:26 -0700 (PDT) Return-Path: Received: from localhost.localdomain ([207.180.219.167]) by smtp.gmail.com with ESMTPSA id g25-20020a2eb5d9000000b0025e4342f96esm34333ljn.127.2022.07.29.08.54.24 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 29 Jul 2022 08:54:25 -0700 (PDT) From: "Savva Mitrofanov" To: devel@edk2.groups.io Cc: =?UTF-8?q?Marvin=20H=C3=A4user?= , Pedro Falcato , Vitaly Cheptsov Subject: [edk2-platforms][PATCH v3 1/1] Ext4Pkg: Code correctness and security improvements Date: Fri, 29 Jul 2022 21:54:17 +0600 Message-Id: <20220729155417.17255-2-savvamtr@gmail.com> X-Mailer: git-send-email 2.37.1 In-Reply-To: <20220729155417.17255-1-savvamtr@gmail.com> References: <20220729155417.17255-1-savvamtr@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable This changes tends to improve security of code sections by fixing integer overflows, missing alignment checks, unsafe casts, also simplified some routines, fixed compiler warnings and corrected some code mistakes. - Set HoleLen to UINT64 to prevent truncation in Ext4Read function - Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because by specification files using block maps must be placed within the first 2^32 blocks of a filesystem - 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. Initialize 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/BlockGroup.c | 4 +-- Features/Ext4Pkg/Ext4Dxe/BlockMap.c | 18 ++++++++---- Features/Ext4Pkg/Ext4Dxe/Directory.c | 29 ++------------------ Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 10 ++++--- Features/Ext4Pkg/Ext4Dxe/Extents.c | 8 ++++-- Features/Ext4Pkg/Ext4Dxe/Inode.c | 10 +++---- Features/Ext4Pkg/Ext4Dxe/Superblock.c | 15 +++++----- 9 files changed, 44 insertions(+), 55 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=0D #define EXT4_NR_BLOCKS 15=0D =0D -#define EXT4_GOOD_OLD_INODE_SIZE 128=0D +#define EXT4_GOOD_OLD_INODE_SIZE 128U=0D =0D typedef struct _Ext4_I_OSD2_Linux {=0D UINT16 l_i_blocks_high;=0D @@ -463,6 +463,7 @@ typedef struct { #define EXT4_EXTENT_MAX_INITIALIZED (1 << 15)=0D =0D typedef UINT64 EXT4_BLOCK_NR;=0D +typedef UINT32 EXT2_BLOCK_NR;=0D typedef UINT32 EXT4_INO_NR;=0D =0D // 2 is always the root inode number in ext4=0D 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 (=0D IN EXT4_PARTITION *Partition,=0D IN EXT4_FILE *File,=0D - IN EXT4_BLOCK_NR LogicalBlock,=0D + IN EXT2_BLOCK_NR LogicalBlock,=0D OUT EXT4_EXTENT *Extent=0D );=0D =0D diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c b/Features/Ext4Pkg/Ext4D= xe/BlockGroup.c index 9a1a41901f36..572e8f60ab92 100644 --- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c +++ b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c @@ -218,9 +218,9 @@ Ext4CalculateBlockGroupDescChecksum ( IN UINT32 BlockGroupNum=0D )=0D {=0D - if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) = {=0D + if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)= !=3D 0) {=0D return Ext4CalculateBlockGroupDescChecksumMetadataCsum (Partition, Blo= ckGroupDesc, BlockGroupNum);=0D - } else if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM= ) {=0D + } else if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSU= M) !=3D 0) {=0D return Ext4CalculateBlockGroupDescChecksumGdtCsum (Partition, BlockGro= upDesc, BlockGroupNum);=0D }=0D =0D 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 (=0D IN CONST EXT4_PARTITION *Partition,=0D IN UINT32 LogicalBlock,=0D - OUT EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]=0D + OUT EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]=0D )=0D {=0D // The logic behind the block map is very much like a page table=0D @@ -123,7 +123,7 @@ Ext4GetBlockPath ( break;=0D default:=0D // EXT4_TYPE_BAD_BLOCK=0D - return -1;=0D + break;=0D }=0D =0D return Type + 1;=0D @@ -213,12 +213,12 @@ EFI_STATUS Ext4GetBlocks (=0D IN EXT4_PARTITION *Partition,=0D IN EXT4_FILE *File,=0D - IN EXT4_BLOCK_NR LogicalBlock,=0D + IN EXT2_BLOCK_NR LogicalBlock,=0D OUT EXT4_EXTENT *Extent=0D )=0D {=0D EXT4_INODE *Inode;=0D - EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];=0D + EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];=0D UINTN BlockPathLength;=0D UINTN Index;=0D UINT32 *Buffer;=0D @@ -230,7 +230,7 @@ Ext4GetBlocks ( =0D BlockPathLength =3D Ext4GetBlockPath (Partition, LogicalBlock, BlockPath= );=0D =0D - if (BlockPathLength =3D=3D (UINTN)-1) {=0D + if (BlockPathLength - 1 =3D=3D EXT4_TYPE_BAD_BLOCK) {=0D // Bad logical block (out of range)=0D return EFI_NO_MAPPING;=0D }=0D @@ -272,7 +272,13 @@ Ext4GetBlocks ( }=0D }=0D =0D - Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof (UINT32),= BlockPath[BlockPathLength - 1], Extent);=0D + Ext4GetExtentInBlockMap (=0D + Buffer,=0D + Partition->BlockSize / sizeof (UINT32),=0D + BlockPath[BlockPathLength - 1],=0D + Extent=0D + );=0D +=0D FreePool (Buffer);=0D =0D return EFI_SUCCESS;=0D 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 ( }=0D =0D // Dirent sizes need to be 4 byte aligned=0D - if (Dirent->rec_len % 4) {=0D + if ((Dirent->rec_len % 4) !=3D 0) {=0D return FALSE;=0D }=0D =0D @@ -160,17 +160,6 @@ Ext4RetrieveDirent ( return EFI_VOLUME_CORRUPTED;=0D }=0D =0D - // Ignore names bigger than our limit.=0D -=0D - /* Note: I think having a limit is sane because:=0D - 1) It's nicer to work with.=0D - 2) Linux and a number of BSDs also have a filename limit of 255.=0D - */=0D - if (Entry->name_len > EXT4_NAME_MAX) {=0D - BlockOffset +=3D Entry->rec_len;=0D - continue;=0D - }=0D -=0D // Unused entry=0D if (Entry->inode =3D=3D 0) {=0D BlockOffset +=3D Entry->rec_len;=0D @@ -548,20 +537,8 @@ Ext4RemoveDentry ( IN OUT EXT4_DENTRY *ToBeRemoved=0D )=0D {=0D - EXT4_DENTRY *D;=0D - LIST_ENTRY *Entry;=0D - LIST_ENTRY *NextEntry;=0D -=0D - BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {=0D - D =3D EXT4_DENTRY_FROM_DENTRY_LIST (Entry);=0D -=0D - if (D =3D=3D ToBeRemoved) {=0D - RemoveEntryList (Entry);=0D - return;=0D - }=0D - }=0D -=0D - DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for= dentry\n"));=0D + ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent->Children));=0D + RemoveEntryList (&ToBeRemoved->ListNode);=0D }=0D =0D /**=0D 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 ( =0D EFI_DRIVER_BINDING_PROTOCOL gExt4BindingProtocol =3D=0D {=0D - Ext4IsBindingSupported,=0D - Ext4Bind,=0D - Ext4Stop,=0D - EXT4_DRIVER_VERSION=0D + .Supported =3D Ext4IsBindingSupported,=0D + .Start =3D Ext4Bind,=0D + .Stop =3D Ext4Stop,=0D + .Version =3D EXT4_DRIVER_VERSION,=0D + .ImageHandle =3D NULL,=0D + .DriverBindingHandle =3D NULL=0D };=0D =0D /**=0D diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/= Extents.c index c3874df71751..369879e07fe7 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c @@ -257,9 +257,11 @@ Ext4GetExtent ( return EFI_SUCCESS;=0D }=0D =0D - if (!(Inode->i_flags & EXT4_EXTENTS_FL)) {=0D + if ((Inode->i_flags & EXT4_EXTENTS_FL) =3D=3D 0) {=0D // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent usi= ng the block map=0D - Status =3D Ext4GetBlocks (Partition, File, LogicalBlock, Extent);=0D + // By specification files using block maps must be placed within the f= irst 2^32 blocks=0D + // of a filesystem, so we can safely cast LogicalBlock to uint32=0D + Status =3D Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Exten= t);=0D =0D if (!EFI_ERROR (Status)) {=0D Ext4CacheExtents (File, Extent, 1);=0D @@ -420,7 +422,7 @@ Ext4ExtentsMapKeyCompare ( Extent =3D UserStruct;=0D Block =3D (UINT32)(UINTN)StandaloneKey;=0D =0D - if ((Block >=3D Extent->ee_block) && (Block < Extent->ee_block + Ext4Get= ExtentLength (Extent))) {=0D + if ((Block >=3D Extent->ee_block) && (Block - Extent->ee_block < Ext4Get= ExtentLength (Extent))) {=0D return 0;=0D }=0D =0D diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/In= ode.c index 831f5946e870..7f8be2f02643 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c @@ -100,7 +100,7 @@ Ext4Read ( EFI_STATUS Status;=0D BOOLEAN HasBackingExtent;=0D UINT32 HoleOff;=0D - UINTN HoleLen;=0D + UINT64 HoleLen;=0D UINT64 ExtentStartBytes;=0D UINT64 ExtentLengthBytes;=0D UINT64 ExtentLogicalBytes;=0D @@ -155,10 +155,10 @@ Ext4Read ( HoleLen =3D (Ext4GetExtentLength (&Extent) * Partition->BlockSize)= - HoleOff;=0D }=0D =0D - WasRead =3D HoleLen > RemainingRead ? RemainingRead : HoleLen;=0D + WasRead =3D HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen= ;=0D // Potential improvement: In the future, we could get the file hole'= s total=0D // size and memset all that=0D - SetMem (Buffer, WasRead, 0);=0D + ZeroMem (Buffer, WasRead);=0D } else {=0D ExtentStartBytes =3D MultU64x32 (=0D LShiftU64 (Extent.ee_start_hi, 32) |=0D @@ -291,7 +291,7 @@ Ext4FilePhysicalSpace ( =0D // If HUGE_FILE is enabled and EXT4_HUGE_FILE_FL is set in the inode's= flags, each unit=0D // in i_blocks corresponds to an actual filesystem block=0D - if (File->Inode->i_flags & EXT4_HUGE_FILE_FL) {=0D + if ((File->Inode->i_flags & EXT4_HUGE_FILE_FL) !=3D 0) {=0D return MultU64x32 (Blocks, File->Partition->BlockSize);=0D }=0D }=0D @@ -431,7 +431,7 @@ Ext4FileCreateTime ( Inode =3D File->Inode;=0D =0D if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) {=0D - SetMem (Time, sizeof (EFI_TIME), 0);=0D + ZeroMem (Time, sizeof (EFI_TIME));=0D return;=0D }=0D =0D diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4D= xe/Superblock.c index 47fc3a65507a..c22155ba11b4 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c @@ -220,7 +220,7 @@ Ext4OpenSuperblock ( return EFI_UNSUPPORTED;=0D }=0D =0D - if (Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) {=0D + if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) !=3D= 0) {=0D Partition->InitialSeed =3D Sb->s_checksum_seed;=0D } else {=0D Partition->InitialSeed =3D Ext4CalculateChecksum (Partition, Sb->s_uui= d, 16, ~0U);=0D @@ -257,16 +257,17 @@ Ext4OpenSuperblock ( ));=0D =0D if (EXT4_IS_64_BIT (Partition)) {=0D + // s_desc_size should be 4 byte aligned and=0D + // 64 bit filesystems need DescSize to be 64 bytes=0D + if (((Sb->s_desc_size % 4) !=3D 0) || (Sb->s_desc_size < EXT4_64BIT_BL= OCK_DESC_SIZE)) {=0D + return EFI_VOLUME_CORRUPTED;=0D + }=0D +=0D Partition->DescSize =3D Sb->s_desc_size;=0D } else {=0D Partition->DescSize =3D EXT4_OLD_BLOCK_DESC_SIZE;=0D }=0D =0D - if ((Partition->DescSize < EXT4_64BIT_BLOCK_DESC_SIZE) && EXT4_IS_64_BIT= (Partition)) {=0D - // 64 bit filesystems need DescSize to be 64 bytes=0D - return EFI_VOLUME_CORRUPTED;=0D - }=0D -=0D if (!Ext4VerifySuperblockChecksum (Partition, Sb)) {=0D DEBUG ((DEBUG_ERROR, "[ext4] Bad superblock checksum %lx\n", Ext4Calcu= lateSuperblockChecksum (Partition, Sb)));=0D return EFI_VOLUME_CORRUPTED;=0D @@ -342,7 +343,7 @@ Ext4CalculateChecksum ( // For some reason, EXT4 really likes non-inverted CRC32C checksums,= so we stick to that here.=0D return ~CalculateCrc32c(Buffer, Length, ~InitialValue);=0D default:=0D - UNREACHABLE ();=0D + ASSERT (FALSE);=0D return 0;=0D }=0D }=0D --=20 2.37.1