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.25825.1683615934755879599 for ; Tue, 09 May 2023 00:05:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=OeYKezyD; 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 388A4240284 for ; Tue, 9 May 2023 09:05:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1683615932; bh=y3OyBGOZKA1xWpjx7mlfiliIKfnPtnS7HisCKGJv4cc=; h=From:Subject:Date:Cc:To:From; b=OeYKezyDbvjHPlCpuT0PI3cgmLlGE4AMVsnF4EnxOVQixkbWV5W2+6yOE1nW84WYP qgWBBniKsdAinwX35Lw8y6680LCY8RmuUoPipWTtRPPXsORvBbhnX9NsMwnXrFC4Pb cVZhxacMDKBCuwLSAZxRVkV7NFLyn+LYxp0LCfLKwt/44mV7GWPFGL04LgAVa5PHfw wwWn8slrTMsWXA9Se4tKw1QARqOYTIUC7Hd8QI9dWULqwjPoGoEILdRAXhtoe/f4gO SU+9j1cwuEz2lxjf6NwrhxXaDsAAX1udFit7PwRhdp0S4BcHSq8/sKmyqYku0rZ/YD rS19rd4wP6a9g== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4QFq1R1Fkzz9rxG; Tue, 9 May 2023 09:05:31 +0200 (CEST) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 1/2] Ext4Pkg: Improve extent node validation on the number of entries Date: Tue, 9 May 2023 07:05:30 +0000 Message-Id: References: <20230509004202.243859-1-pedro.falcato@gmail.com> Cc: devel@edk2.groups.io, Savva Mitrofanov In-Reply-To: <20230509004202.243859-1-pedro.falcato@gmail.com> To: Pedro Falcato Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 9. May 2023, at 02:42, Pedro Falcato wrote: >=20 > =EF=BB=BFImprove the extent tree node validation by validating the number o= f > entries the node advertises against the theoretical max (derived from > the size of on-disk structs and the block size (or i_data, if inline > extents). >=20 > Previously, we did not validate the number of entries. This could be > exploited for out-of-bounds reads and crashes. >=20 > Cc: Marvin H=C3=A4user > Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") > Reported-by: Savva Mitrofanov > Signed-off-by: Pedro Falcato > --- > Features/Ext4Pkg/Ext4Dxe/Extents.c | 47 +++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 17 deletions(-) >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe= /Extents.c > index 9e4364e50d99..66d085938549 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c > @@ -1,7 +1,7 @@ > /** @file > Extent related routines >=20 > - Copyright (c) 2021 - 2022 Pedro Falcato All rights reserved. > + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ >=20 > @@ -80,13 +80,15 @@ Ext4GetInoExtentHeader ( > /** > Checks if an extent header is valid. > @param[in] Header Pointer to the EXT4_EXTENT_HEADER struct= ure. > + @param[in] MaxEntries Maximum number of entries possible for t= his tree node. >=20 > @return TRUE if valid, FALSE if not. > **/ > STATIC > BOOLEAN > Ext4ExtentHeaderValid ( > - IN CONST EXT4_EXTENT_HEADER *Header > + IN CONST EXT4_EXTENT_HEADER *Header, > + IN UINT16 MaxEntries > ) > { > if (Header->eh_depth > EXT4_EXTENT_TREE_MAX_DEPTH) { > @@ -99,6 +101,18 @@ Ext4ExtentHeaderValid ( > return FALSE; > } >=20 > + if ((Header->eh_max > MaxEntries) || (Header->eh_entries > MaxEntries))= { For eh_entries, this is implicit via the check below. > + DEBUG (( > + DEBUG_ERROR, > + "[ext4] Invalid extent header entries (extent header: %u max," > + " %u entries, theoretical max: %u) (larger than permitted)\n", > + Header->eh_max, > + Header->eh_entries, > + MaxEntries > + )); > + return FALSE; > + } > + > if (Header->eh_max < Header->eh_entries) { > DEBUG (( > DEBUG_ERROR, > @@ -212,6 +226,9 @@ Ext4ExtentIdxLeafBlock ( > return LShiftU64 (Index->ei_leaf_hi, 32) | Index->ei_leaf_lo; > } >=20 > +// Results of sizeof(i_data) / sizeof(extent) - 1 =3D 4 > +#define EXT4_NR_INLINE_EXTENTS 4 > + > /** > Retrieves an extent from an EXT4 inode. > @param[in] Partition Pointer to the opened EXT4 partition. > @@ -237,7 +254,7 @@ Ext4GetExtent ( > EXT4_EXTENT_HEADER *ExtHeader; > EXT4_EXTENT_INDEX *Index; > EFI_STATUS Status; > - EXT4_BLOCK_NR BlockNumber; > + UINT32 MaxExtentsPerNode; >=20 > Inode =3D File->Inode; > Ext =3D NULL; > @@ -275,12 +292,17 @@ Ext4GetExtent ( >=20 > ExtHeader =3D Ext4GetInoExtentHeader (Inode); >=20 > - if (!Ext4ExtentHeaderValid (ExtHeader)) { > + if (!Ext4ExtentHeaderValid (ExtHeader, EXT4_NR_INLINE_EXTENTS)) { > return EFI_VOLUME_CORRUPTED; > } >=20 > CurrentDepth =3D ExtHeader->eh_depth; >=20 > + // A single node fits into a single block, so we can only have (BlockSi= ze / sizeof(EXT4_EXTENT)) - 1 > + // extents in a single node. Note the -1, because both leaf and interna= l node headers are 12 bytes, > + // and so are individual entries. > + MaxExtentsPerNode =3D (Partition->BlockSize / sizeof (EXT4_EXTENT)) - 1= ; > + > while (ExtHeader->eh_depth !=3D 0) { > CurrentDepth--; > // While depth !=3D 0, we're traversing the tree itself and not any le= aves > @@ -289,17 +311,7 @@ Ext4GetExtent ( > // Therefore, we can use binary search, and it's actually the standard= for doing so > // (see FreeBSD). >=20 > - Index =3D Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); > - BlockNumber =3D Ext4ExtentIdxLeafBlock (Index); > - > - // Check that block isn't file hole > - if (BlockNumber =3D=3D EXT4_BLOCK_FILE_HOLE) Why is this no longer possible? Best regards, Marvin > { > - if (Buffer !=3D NULL) { > - FreePool (Buffer); > - } > - > - return EFI_VOLUME_CORRUPTED; > - } > + Index =3D Ext4BinsearchExtentIndex (ExtHeader, LogicalBlock); >=20 > if (Buffer =3D=3D NULL) { > Buffer =3D AllocatePool (Partition->BlockSize); > @@ -309,7 +321,8 @@ Ext4GetExtent ( > } >=20 > // Read the leaf block onto the previously-allocated buffer. > - Status =3D Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber); > + > + Status =3D Ext4ReadBlocks (Partition, Buffer, 1, Ext4ExtentIdxLeafBlo= ck (Index)); > if (EFI_ERROR (Status)) { > FreePool (Buffer); > return Status; > @@ -317,7 +330,7 @@ Ext4GetExtent ( >=20 > ExtHeader =3D Buffer; >=20 > - if (!Ext4ExtentHeaderValid (ExtHeader)) { > + if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) { > FreePool (Buffer); > return EFI_VOLUME_CORRUPTED; > } > --=20 > 2.40.1 >=20