From 92065decc87e531c1701b6dd413fb52ef99a5b43 Mon Sep 17 00:00:00 2001 From: Savva Mitrofanov Date: Wed, 1 Feb 2023 22:26:47 +0600 Subject: [PATCH] Ext4Pkg: Correct integer overflow check logic in DiskUtil MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct multiplication overflow check code and add additional check for emptiness of number of blocks and block number. Cc: Marvin Häuser Cc: Pedro Falcato Cc: Vitaly Cheptsov Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") Signed-off-by: Savva Mitrofanov Reviewed-by: Marvin Häuser Reviewed-by: Pedro Falcato --- Features/Ext4Pkg/Ext4Dxe/DiskUtil.c | 18 ++++++++++++++---- Features/Ext4Pkg/Ext4Dxe/Extents.c | 16 +++++++++++++--- Features/Ext4Pkg/Ext4Pkg.dsc | 2 +- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c index 32da35f7d9f..5df9ce5bafc 100644 --- a/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c +++ b/Features/Ext4Pkg/Ext4Dxe/DiskUtil.c @@ -54,17 +54,20 @@ Ext4ReadBlocks ( UINT64 Offset; UINTN Length; + ASSERT (NumberBlocks != 0); + ASSERT (BlockNumber != EXT4_BLOCK_FILE_HOLE); + Offset = MultU64x32 (BlockNumber, Partition->BlockSize); Length = NumberBlocks * Partition->BlockSize; // Check for overflow on the block -> byte conversions. // Partition->BlockSize is never 0, so we don't need to check for that. - if (Offset > DivU64x32 ((UINT64)-1, Partition->BlockSize)) { + if (DivU64x64Remainder (Offset, BlockNumber, NULL) != Partition->BlockSize) { return EFI_INVALID_PARAMETER; } - if (Length > (UINTN)-1/Partition->BlockSize) { + if (Length / NumberBlocks != Partition->BlockSize) { return EFI_INVALID_PARAMETER; } @@ -92,14 +95,21 @@ Ext4AllocAndReadBlocks ( VOID *Buf; UINTN Length; + // Check that number of blocks isn't empty, because + // this is incorrect condition for opened partition, + // so we just early-exit + if ((NumberBlocks == 0) || (BlockNumber == EXT4_BLOCK_FILE_HOLE)) { + return NULL; + } + Length = NumberBlocks * Partition->BlockSize; - if (Length > (UINTN)-1/Partition->BlockSize) { + // Check for integer overflow + if (Length / NumberBlocks != Partition->BlockSize) { return NULL; } Buf = AllocatePool (Length); - if (Buf == NULL) { return NULL; } diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c index e1001d0a429..9e4364e50d9 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; Inode = File->Inode; Ext = NULL; @@ -288,7 +289,17 @@ Ext4GetExtent ( // Therefore, we can use binary search, and it's actually the standard for doing so // (see FreeBSD). - Index = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); + Index = Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); + BlockNumber = Ext4ExtentIdxLeafBlock (Index); + + // Check that block isn't file hole + if (BlockNumber == EXT4_BLOCK_FILE_HOLE) { + if (Buffer != NULL) { + FreePool (Buffer); + } + + return EFI_VOLUME_CORRUPTED; + } if (Buffer == NULL) { Buffer = AllocatePool (Partition->BlockSize); @@ -298,8 +309,7 @@ Ext4GetExtent ( } // Read the leaf block onto the previously-allocated buffer. - - Status = Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlock (Index)); + Status = Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber); if (EFI_ERROR (Status)) { FreePool (Buffer); return Status; diff --git a/Features/Ext4Pkg/Ext4Pkg.dsc b/Features/Ext4Pkg/Ext4Pkg.dsc index 59bc327ebf6..621c63eaf92 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 - + # # Required for stack protector support #