From nobody Thu Mar 28 10:23:14 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) client-ip=66.175.222.108; envelope-from=bounce+27952+91976+1787277+3901457@groups.io; helo=mail02.groups.io; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce+27952+91976+1787277+3901457@groups.io; dmarc=fail(p=none dis=none) header.from=gmail.com ARC-Seal: i=1; a=rsa-sha256; t=1659110070; cv=none; d=zohomail.com; s=zohoarc; b=NK1XcMDorqRNYP/8DHKNchSOsNubK2EW18xMJZV1W+xaTpLxOnSAapCWBlkD4CuN2JbVpdEQdNPfoDYL3kTR7xQEllO8QHdFYRuuJTHEsgUEJs1nHi1qjRJaKkuzeTwjzk50zyv/Ed+iuuC96SNF3cSSt+ETZ6qdJzGCRYwqQEQ= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1659110070; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Id:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:References:Sender:Subject:To; bh=pQx0xk4la8Z0azlUKaF34oXK1rODhDFV4yzKVxHyq20=; b=AanQxkdZ8NAL6xJmSP14T258CFhi/B+F2b2BH2sGXGtUWeWXJBvEYey8pSXZAt8JKzI9D3ref9yS/1S+r1bDL2xdY0yLq7o+aX0tm5/jvwYu4Gf6b36D5l6e3SZgqyTbO/xo2+zv6288+M4rQQIfWtxaMwSG+b4qdlxyCvjZjQg= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce+27952+91976+1787277+3901457@groups.io; dmarc=fail header.from= (p=none dis=none) Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by mx.zohomail.com with SMTPS id 1659110070627463.16121098545693; Fri, 29 Jul 2022 08:54:30 -0700 (PDT) Return-Path: X-Received: by 127.0.0.2 with SMTP id Q7x2YY1788612xweomtpQK68; Fri, 29 Jul 2022 08:54:29 -0700 X-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 X-Received: by mail-lj1-f172.google.com with SMTP id bx8so3195571ljb.3 for ; Fri, 29 Jul 2022 08:54:28 -0700 (PDT) X-Gm-Message-State: 4YC7fTS7UaEtJbC8ui8Vn8Ljx1787277AA= 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) X-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-devel] [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> In-Reply-To: <20220729155417.17255-1-savvamtr@gmail.com> References: <20220729155417.17255-1-savvamtr@gmail.com> MIME-Version: 1.0 Precedence: Bulk List-Unsubscribe: List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,savvamtr@gmail.com Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=groups.io; q=dns/txt; s=20140610; t=1659110069; bh=pakCLOTt15E2laFEo1Tw8erzITlTuE/YAnFWvduY/5M=; h=Cc:Content-Type:Date:From:Reply-To:Subject:To; b=T/bKTAYK+gxWLSyO1kAF6rX4XQ0rMi8h6E4A6Gi6WgLusiZdhxN3eaVb+1qqTr5fvVF /6vzZ6bpAzHbDL7xqZHtuRypYJRvabru8BNi/H9BmoXx4xUvb6cBpPjlyecceTAMkTLwk tceqY0LVQxcmVCwjsMPHACVJhg2ce14FHf0= X-ZohoMail-DKIM: pass (identity @groups.io) X-ZM-MESSAGEID: 1659110072412100007 Content-Type: text/plain; charset="utf-8" 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 Reviewed-by: Pedro Falcato --- 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 #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/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 ) { - if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) { + if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)= !=3D 0) { return Ext4CalculateBlockGroupDescChecksumMetadataCsum (Partition, Blo= ckGroupDesc, BlockGroupNum); - } else if (Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM= ) { + } else if ((Partition->FeaturesRoCompat & EXT4_FEATURE_RO_COMPAT_GDT_CSU= M) !=3D 0) { return Ext4CalculateBlockGroupDescChecksumGdtCsum (Partition, BlockGro= upDesc, BlockGroupNum); } =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/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 ( } =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..369879e07fe7 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c @@ -257,9 +257,11 @@ Ext4GetExtent ( return EFI_SUCCESS; } =20 - if (!(Inode->i_flags & EXT4_EXTENTS_FL)) { + if ((Inode->i_flags & EXT4_EXTENTS_FL) =3D=3D 0) { // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent usi= ng the block map - Status =3D Ext4GetBlocks (Partition, File, LogicalBlock, Extent); + // By specification files using block maps must be placed within the f= irst 2^32 blocks + // of a filesystem, so we can safely cast LogicalBlock to uint32 + Status =3D Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Exten= t); =20 if (!EFI_ERROR (Status)) { Ext4CacheExtents (File, Extent, 1); @@ -420,7 +422,7 @@ Ext4ExtentsMapKeyCompare ( Extent =3D UserStruct; Block =3D (UINT32)(UINTN)StandaloneKey; =20 - if ((Block >=3D Extent->ee_block) && (Block < Extent->ee_block + Ext4Get= ExtentLength (Extent))) { + if ((Block >=3D Extent->ee_block) && (Block - Extent->ee_block < Ext4Get= ExtentLength (Extent))) { return 0; } =20 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; 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) | @@ -291,7 +291,7 @@ Ext4FilePhysicalSpace ( =20 // If HUGE_FILE is enabled and EXT4_HUGE_FILE_FL is set in the inode's= flags, each unit // in i_blocks corresponds to an actual filesystem block - if (File->Inode->i_flags & EXT4_HUGE_FILE_FL) { + if ((File->Inode->i_flags & EXT4_HUGE_FILE_FL) !=3D 0) { return MultU64x32 (Blocks, File->Partition->BlockSize); } } @@ -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/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; } =20 - if (Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) { + if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) !=3D= 0) { Partition->InitialSeed =3D Sb->s_checksum_seed; } else { Partition->InitialSeed =3D Ext4CalculateChecksum (Partition, Sb->s_uui= d, 16, ~0U); @@ -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_BL= OCK_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", Ext4Calcu= lateSuperblockChecksum (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.1 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#91976): https://edk2.groups.io/g/devel/message/91976 Mute This Topic: https://groups.io/mt/92693958/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-