From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 8A60FD80FDE for ; Mon, 30 Oct 2023 11:44:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=nYddTN72ZwXppeVoyVnvjWYpMETPSVWH3FTuOPvATeA=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698666275; v=1; b=F+plCOyqAo+9mxdbD0dB1hW63w/EsQuLOkc8oPRD3oVSpNbzS7iiHiLpZf+XUQWoQL7kZaKt dYDsdyl8nCffmREmpJOMVVX6CRnCzwhQpNYw+pKFmlGjp/x0ch3kKgc2A6CR/ZnDkctvUVTir6Y E5YhjDvjixyrE/zXqyT/PeXQ= X-Received: by 127.0.0.2 with SMTP id 7e4GYY7687511xNIN3XeYVag; Mon, 30 Oct 2023 04:44:35 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.146639.1698666274435509575 for ; Mon, 30 Oct 2023 04:44:34 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-128-ZF7znCo8ME6Kn_DzZJGo6g-1; Mon, 30 Oct 2023 07:44:27 -0400 X-MC-Unique: ZF7znCo8ME6Kn_DzZJGo6g-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9A2418007B3; Mon, 30 Oct 2023 11:44:26 +0000 (UTC) X-Received: from [10.39.194.199] (unknown [10.39.194.199]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A0D53492BE0; Mon, 30 Oct 2023 11:44:25 +0000 (UTC) Message-ID: <73338cc6-89cd-1eda-47ec-461d54b19363@redhat.com> Date: Mon, 30 Oct 2023 12:44:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 1/4] StandaloneMmPkg/Core: Limit FwVol encapsulation section recursion To: Wei6 Xu , devel@edk2.groups.io Cc: Ard Biesheuvel , Sami Mujawar , Ray Ni References: <44a87be85d8b0f475fb30ff9a9a9bf4e2d8f9e26.1698651605.git.wei6.xu@intel.com> From: "Laszlo Ersek" In-Reply-To: <44a87be85d8b0f475fb30ff9a9a9bf4e2d8f9e26.1698651605.git.wei6.xu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: YdeKo2PCkafeADAvvzrEPrnNx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=F+plCOyq; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 10/30/23 08:49, Wei6 Xu wrote: > MmCoreFfsFindMmDriver() is called recursively for encapsulation sections. > Currently this recursion is not limited. Introduce a new PCD > (fixed-at-build, or patchable-in-module), and make MmCoreFfsFindMmDriver(= ) > track the section nesting depth against that PCD. >=20 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Ray Ni > Signed-off-by: Wei6 Xu > --- > StandaloneMmPkg/Core/FwVol.c | 16 ++++++++++++++-- > StandaloneMmPkg/Core/StandaloneMmCore.c | 5 +++-- > StandaloneMmPkg/Core/StandaloneMmCore.inf | 3 +++ > StandaloneMmPkg/StandaloneMmPkg.dec | 5 +++++ > 4 files changed, 25 insertions(+), 4 deletions(-) >=20 > diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c > index 1f6d7714ba97..e1e20ffd14ac 100644 > --- a/StandaloneMmPkg/Core/FwVol.c > +++ b/StandaloneMmPkg/Core/FwVol.c > @@ -48,6 +48,9 @@ FvIsBeingProcessed ( > MM driver and return its PE32 image. > =20 > @param [in] FwVolHeader Pointer to memory mapped FV > + @param [in] Depth Nesting depth of encapsulation sections. Cal= lers > + different from MmCoreFfsFindMmDriver() are > + responsible for passing in a zero Depth. > =20 > @retval EFI_SUCCESS Success. > @retval EFI_INVALID_PARAMETER Invalid parameter. > @@ -55,11 +58,15 @@ FvIsBeingProcessed ( > @retval EFI_OUT_OF_RESOURCES Out of resources. > @retval EFI_VOLUME_CORRUPTED Firmware volume is corrupted. > @retval EFI_UNSUPPORTED Operation not supported. > + @retval EFI_ABORTED Recursion aborted because Depth has be= en > + greater than or equal to > + PcdFwVolMmMaxEncapsulationDepth. > =20 > **/ > EFI_STATUS > MmCoreFfsFindMmDriver ( > - IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader, > + IN UINT32 Depth > ) > { > EFI_STATUS Status; > @@ -84,6 +91,11 @@ MmCoreFfsFindMmDriver ( > =20 > DEBUG ((DEBUG_INFO, "MmCoreFfsFindMmDriver - 0x%x\n", FwVolHeader)); > =20 > + if (Depth >=3D PcdGet32 (PcdFwVolMmMaxEncapsulationDepth)) { > + DEBUG ((DEBUG_ERROR, "%a: recursion aborted due to nesting depth\n",= __func__)); > + return EFI_ABORTED; > + } > + > if (FvHasBeenProcessed (FwVolHeader)) { > return EFI_SUCCESS; > } > @@ -172,7 +184,7 @@ MmCoreFfsFindMmDriver ( > } > =20 > InnerFvHeader =3D (VOID *)(Section + 1); > - Status =3D MmCoreFfsFindMmDriver (InnerFvHeader); > + Status =3D MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1); > if (EFI_ERROR (Status)) { > goto FreeDstBuffer; > } > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Co= re/StandaloneMmCore.c > index d221f1d1115d..523ea0a632a1 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.c > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c > @@ -11,7 +11,8 @@ > =20 > EFI_STATUS > MmCoreFfsFindMmDriver ( > - IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader, > + IN UINT32 Depth > ); > =20 > EFI_STATUS > @@ -643,7 +644,7 @@ StandaloneMmMain ( > // > DEBUG ((DEBUG_INFO, "Mm Dispatch StandaloneBfvAddress - 0x%08x\n", gMm= CorePrivate->StandaloneBfvAddress)); > if (gMmCorePrivate->StandaloneBfvAddress !=3D 0) { > - MmCoreFfsFindMmDriver ((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)gMmCoreP= rivate->StandaloneBfvAddress); > + MmCoreFfsFindMmDriver ((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)gMmCoreP= rivate->StandaloneBfvAddress, 0); > MmDispatcher (); > } > =20 > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/= Core/StandaloneMmCore.inf > index c44b9ff33303..02ecd68f37e2 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf > @@ -76,6 +76,9 @@ [Guids] > gEfiEventExitBootServicesGuid > gEfiEventReadyToBootGuid > =20 > +[Pcd] > + gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth ##CO= NSUMES > + > # > # This configuration fails for CLANGPDB, which does not support PIE in t= he GCC > # sense. Such however is required for ARM family StandaloneMmCore > diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec b/StandaloneMmPkg/Standa= loneMmPkg.dec > index 46784d94e421..c43632d6d8ae 100644 > --- a/StandaloneMmPkg/StandaloneMmPkg.dec > +++ b/StandaloneMmPkg/StandaloneMmPkg.dec > @@ -48,3 +48,8 @@ [Guids] > gEfiStandaloneMmNonSecureBufferGuid =3D { 0xf00497e3, 0xbfa2, 0x4= 1a1, { 0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }} > gEfiArmTfCpuDriverEpDescriptorGuid =3D { 0x6ecbd5a1, 0xc0f8, 0x4= 702, { 0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }} > =20 > +[PcdsFixedAtBuild, PcdsPatchableInModule] > + ## Maximum permitted encapsulation levels of sections in a firmware vo= lume, > + # in the MM phase. Minimum value is 1. Sections nested more deeply ar= e rejected. > + # @Prompt Maximum permitted FwVol section nesting depth (exclusive) in= MM. > + gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth|0x10|UI= NT32|0x00000001 Would it be possible to move the declaration of MmCoreFfsFindMmDriver() to the header file "StandaloneMmCore.h"? I find it unfortunate that we currently declare the function in "StandaloneMmPkg/Core/StandaloneMmCore.c". Should that function declaration ever diverge from the definition in "StandaloneMmPkg/Core/FwVol.c", we'd only notice that by a crash (garbage parameter passing). It's best to have it in the header file, so that the compiler can check that the call in StandaloneMmMain() and the actual function definition in "FwVol.c" are in sync. Apologies for not pointing this out earlier, but I would have not expected this. After all, we do already have a module-internal header file. With that update: Reviewed-by: Laszlo Ersek Thank you! Laszlo -=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 (#110313): https://edk2.groups.io/g/devel/message/110313 Mute This Topic: https://groups.io/mt/102270546/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-