From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web11.11637.1675333896442200984 for ; Thu, 02 Feb 2023 02:31:36 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=mN2ok9tY; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id D76C12406C2 for ; Thu, 2 Feb 2023 11:31:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675333894; bh=h5xu5Qr2YYn74PNBLebYUeqEpIhJITMV4+OgCuu+Uqk=; h=From:Subject:Date:Cc:To:From; b=mN2ok9tYdX9/jSoqXOhyO/1LtGB96exhNh32pca2Ov+C59M1DUpWVapD3F8EP/YXt NoQAfmX1PFfLm5khVGULWvOQZBZeug8yMusYa+hc6c/BYtgSkAk4udbykwVI9T4kl7 51WhuSAZJ94lKM4rddU16HLwOR/1PA5BwdFNPGXXGF796mnyM83XqV9uUmUD8q+Ag0 gXYsuXHKrrVEmINLslg4E/SnP9YRdTLEF+d8+Il6mLGAQEyEhtX7N4uveZl8ASmOFg wVatqSvF+bp6xObhoOjlscQncMZi9M+8j9baBI0soVUD68L5yCg4T8/Di4JNW6GP10 MlRlXcQ7oBF9A== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P6w7V26DHz6tm4; Thu, 2 Feb 2023 11:31:34 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-platforms][PATCH v4 06/12] Ext4Pkg: Corrects integer overflow check logic in DiskUtil Date: Thu, 2 Feb 2023 10:31:33 +0000 Message-Id: References: <20230202102133.51606-7-savvamtr@gmail.com> Cc: devel@edk2.groups.io, Pedro Falcato , Vitaly Cheptsov In-Reply-To: <20230202102133.51606-7-savvamtr@gmail.com> To: Savva Mitrofanov Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Reviewed-by: Marvin H=C3=A4user > On 2. Feb 2023, at 11:21, Savva Mitrofanov wrote: >=20 > =EF=BB=BFCorrects multiplication overflow check code and adds additional c= heck > for emptiness of number of blocks and block number >=20 > Cc: Marvin H=C3=A4user > Cc: Pedro Falcato > Cc: Vitaly Cheptsov > Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") > Signed-off-by: Savva Mitrofanov > --- > Features/Ext4Pkg/Ext4Pkg.dsc | 2 +- > Features/Ext4Pkg/Ext4Dxe/DiskUtil.c | 18 ++++++++++++++---- > Features/Ext4Pkg/Ext4Dxe/Extents.c | 15 ++++++++++++--- > 3 files changed, 27 insertions(+), 8 deletions(-) >=20 > diff --git a/Features/Ext4Pkg/Ext4Pkg.dsc b/Features/Ext4Pkg/Ext4Pkg.dsc > index 59bc327ebf6e..621c63eaf92d 100644 > --- a/Features/Ext4Pkg/Ext4Pkg.dsc > +++ b/Features/Ext4Pkg/Ext4Pkg.dsc > @@ -46,7 +46,7 @@ > DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf > OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib= /BaseOrderedCollectionRedBlackTreeLib.inf > BaseUcs2Utf8Lib|RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf > - =20 > + > # > # Required for stack protector support > # > diff --git a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c b/Features/Ext4Pkg/Ext4Dx= e/DiskUtil.c > index 32da35f7d9f5..5df9ce5bafcf 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c > +++ b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c > @@ -54,17 +54,20 @@ Ext4ReadBlocks ( > UINT64 Offset; > UINTN Length; >=20 > + ASSERT (NumberBlocks !=3D 0); > + ASSERT (BlockNumber !=3D EXT4_BLOCK_FILE_HOLE); > + > Offset =3D MultU64x32 (BlockNumber, Partition->BlockSize); > Length =3D NumberBlocks * Partition->BlockSize; >=20 > // Check for overflow on the block -> byte conversions. > // Partition->BlockSize is never 0, so we don't need to check for that. >=20 > - if (Offset > DivU64x32 ((UINT64)-1, Partition->BlockSize)) { > + if (DivU64x64Remainder (Offset, BlockNumber, NULL) !=3D Partition->Bloc= kSize) { > return EFI_INVALID_PARAMETER; > } >=20 > - if (Length > (UINTN)-1/Partition->BlockSize) { > + if (Length / NumberBlocks !=3D Partition->BlockSize) { > return EFI_INVALID_PARAMETER; > } >=20 > @@ -92,14 +95,21 @@ Ext4AllocAndReadBlocks ( > VOID *Buf; > UINTN Length; >=20 > + // Check that number of blocks isn't empty, because > + // this is incorrect condition for opened partition, > + // so we just early-exit > + if ((NumberBlocks =3D=3D 0) || (BlockNumber =3D=3D EXT4_BLOCK_FILE_HOLE= )) { > + return NULL; > + } > + > Length =3D NumberBlocks * Partition->BlockSize; >=20 > - if (Length > (UINTN)-1/Partition->BlockSize) { > + // Check for integer overflow > + if (Length / NumberBlocks !=3D Partition->BlockSize) { > return NULL; > } >=20 > Buf =3D AllocatePool (Length); > - > if (Buf =3D=3D NULL) { > return NULL; > } > diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe= /Extents.c > index e1001d0a4292..99cb0f204fc2 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c > @@ -237,6 +237,7 @@ Ext4GetExtent ( > EXT4_EXTENT_HEADER *ExtHeader; > EXT4_EXTENT_INDEX *Index; > EFI_STATUS Status; > + EXT4_BLOCK_NR BlockNumber; >=20 > Inode =3D File->Inode; > Ext =3D NULL; > @@ -288,7 +289,16 @@ Ext4GetExtent ( > // Therefore, we can use binary search, and it's actually the standard= for doing so > // (see FreeBSD). >=20 > - Index =3D Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); > + Index =3D Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); > + BlockNumber =3D Ext4ExtentIdxLeafBlock (Index); > + > + // Check that block isn't file hole > + if (BlockNumber =3D=3D EXT4_BLOCK_FILE_HOLE) { > + if (Buffer !=3D NULL) { > + FreePool (Buffer); > + } > + return EFI_NO_MAPPING; > + } >=20 > if (Buffer =3D=3D NULL) { > Buffer =3D AllocatePool (Partition->BlockSize); > @@ -298,8 +308,7 @@ Ext4GetExtent ( > } >=20 > // Read the leaf block onto the previously-allocated buffer. > - > - Status =3D Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlo= ck (Index)); > + Status =3D Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber); > if (EFI_ERROR (Status)) { > FreePool (Buffer); > return Status; > --=20 > 2.39.1 >=20