From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) by mx.groups.io with SMTP id smtpd.web10.38252.1683651026183109487 for ; Tue, 09 May 2023 09:50:26 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@gmail.com header.s=20221208 header.b=r5OUOE7a; spf=pass (domain: gmail.com, ip: 209.85.210.174, mailfrom: pedro.falcato@gmail.com) Received: by mail-pf1-f174.google.com with SMTP id d2e1a72fcca58-6434e263962so4612772b3a.2 for ; Tue, 09 May 2023 09:50:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683651025; x=1686243025; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=5eLUQt5UK0i3q3ogj8S4ivE5FgSr/FnD9JV8OvR1T9w=; b=r5OUOE7aWqCvy8TA2BfBf2DeKzDyrgycjKkRwzzpNtgox/Hc7WtC+0NNeDpmSl3K6S e1folLaNbXL84vXucimsYuurQHtHrRXAQgPTze1wErXvWB9MuYFDExLmBArEdE9xsXvg TsVSMX1Oauxx/9Ld6GxoZLVgXKOIKFTHiF1Cvc0RvphwnsN/Hb4N5w6gyD0EwjlwI9d4 Az5kLteIiO9gugCsonXFLc0wc5zXXSpM/otduKtPG+xg3LJLoUKL55aCBbNP1jpwTAR2 Qes8PGe9aQKtpmAgzfm5NPHmvKpAx2I4xBni6ZMuazZgkAmJufVnzYfRLmjWas1+7eXe MqlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683651025; x=1686243025; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5eLUQt5UK0i3q3ogj8S4ivE5FgSr/FnD9JV8OvR1T9w=; b=Xm3BrNwYmKH3WBZHpH+ALfnYS40sztO/q+b0jf4KIRSh44V6+Z/3UdJYzvHp2AKAwX jj39WQLdEIWJENu4XpLvguRF0cEA3MK995herkMSjAWEWDhg1cMKa8paNexYE7laeotN 0YF/26aBv2WKvHhvB3TAENDv/Iq4EURAawDkI4mR0JFDk2IWm0rC9D5LlzsQxG49r4n6 n7bwzMGid0WIu7sPIbjZbKLtSP9mbKanRSlJQpncMZk/B73k2VdwpNdjz5+ZxqpJW3ba L55mFDtWNPJ/6VFsNXxzIZyMJ/Ni4+zrrg6zQ43FF3xeDCTaJ6tXO6jzYIdZSfiUZaUE /tUQ== X-Gm-Message-State: AC+VfDzsBNJk/pI8c+R0VkGX9ni0yP8ideUOLt6Us1buyPS2AGOx0+Qf 8KN5PlEaCugaodN/lotIa6N4/4GY3h7XNzqlCFM= X-Google-Smtp-Source: ACHHUZ5GmUTUi53pdY0fbbJiQHsHqoKQ79A8Pz8tuA9Dlbv27SIYlBONmqYvJ7l0IoOTuzHGmNUV3rt1lFO3R3AClY4= X-Received: by 2002:a05:6a20:2591:b0:f2:8c89:cd30 with SMTP id k17-20020a056a20259100b000f28c89cd30mr19550715pzd.13.1683651025520; Tue, 09 May 2023 09:50:25 -0700 (PDT) MIME-Version: 1.0 References: <20230509154922.278200-1-pedro.falcato@gmail.com> <2EC24DA1-FA6A-4FAC-8750-009E7689D5EC@posteo.de> In-Reply-To: <2EC24DA1-FA6A-4FAC-8750-009E7689D5EC@posteo.de> From: "Pedro Falcato" Date: Tue, 9 May 2023 17:50:14 +0100 Message-ID: Subject: Re: [PATCH v3 1/2] Ext4Pkg: Improve extent node validation on the number of entries To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: devel@edk2.groups.io, Savva Mitrofanov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thank you! Merged the patchset as 82f4b39 and 9247980, respectively. On Tue, May 9, 2023 at 5:07=E2=80=AFPM Marvin H=C3=A4user wrote: > > Reviewed-by: Marvin H=C3=A4user > > > On 9. May 2023, at 17:49, Pedro Falcato wrote= : > > > > =EF=BB=BFImprove the extent tree node validation by validating the numb= er of > > 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). > > > > Previously, we did not validate the number of entries. This could be > > exploited for out-of-bounds reads and crashes. > > > > 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 | 32 ++++++++++++++++++++++++++---- > > 1 file changed, 28 insertions(+), 4 deletions(-) > > > > diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4= Dxe/Extents.c > > index 9e4364e50d99..2b82417c9e10 100644 > > --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c > > +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c > > @@ -1,7 +1,7 @@ > > /** @file > > Extent related routines > > > > - 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 > > **/ > > > > @@ -80,13 +80,15 @@ Ext4GetInoExtentHeader ( > > /** > > Checks if an extent header is valid. > > @param[in] Header Pointer to the EXT4_EXTENT_HEADER str= ucture. > > + @param[in] MaxEntries Maximum number of entries possible f= or this tree node. > > > > @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; > > } > > > > + // Note: We do not need to check eh_entries here, as the next branch= makes sure max >=3D entries > > + if (Header->eh_max > MaxEntries) { > > + DEBUG (( > > + DEBUG_ERROR, > > + "[ext4] Invalid extent header max entries (%u eh_max, " > > + "theoretical max is %u) (larger than permitted)\n", > > + Header->eh_max, > > + 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; > > } > > > > +// 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,6 +254,7 @@ Ext4GetExtent ( > > EXT4_EXTENT_HEADER *ExtHeader; > > EXT4_EXTENT_INDEX *Index; > > EFI_STATUS Status; > > + UINT32 MaxExtentsPerNode; > > EXT4_BLOCK_NR BlockNumber; > > > > Inode =3D File->Inode; > > @@ -275,12 +293,17 @@ Ext4GetExtent ( > > > > ExtHeader =3D Ext4GetInoExtentHeader (Inode); > > > > - if (!Ext4ExtentHeaderValid (ExtHeader)) { > > + if (!Ext4ExtentHeaderValid (ExtHeader, EXT4_NR_INLINE_EXTENTS)) { > > return EFI_VOLUME_CORRUPTED; > > } > > > > CurrentDepth =3D ExtHeader->eh_depth; > > > > + // A single node fits into a single block, so we can only have (Bloc= kSize / sizeof(EXT4_EXTENT)) - 1 > > + // extents in a single node. Note the -1, because both leaf and inte= rnal 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= leaves > > @@ -309,6 +332,7 @@ Ext4GetExtent ( > > } > > > > // Read the leaf block onto the previously-allocated buffer. > > + > > Status =3D Ext4ReadBlocks (Partition, Buffer, 1, BlockNumber); > > if (EFI_ERROR (Status)) { > > FreePool (Buffer); > > @@ -317,7 +341,7 @@ Ext4GetExtent ( > > > > ExtHeader =3D Buffer; > > > > - if (!Ext4ExtentHeaderValid (ExtHeader)) { > > + if (!Ext4ExtentHeaderValid (ExtHeader, MaxExtentsPerNode)) { > > FreePool (Buffer); > > return EFI_VOLUME_CORRUPTED; > > } > > -- > > 2.40.1 > > > --=20 Pedro