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 4E6027803DB for ; Wed, 8 Nov 2023 19:47:39 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zVf8XQN0WknxbQY7KhfI33reXBAiprbmOK8fi/+4QcA=; 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=1699472858; v=1; b=lJjJ26ojyQyaGt/vuk9iZ9DAz6A8H2nfK5uXnM1dUFqblvrhFqfWnvqFxxBtAvN7+/tP7Mal 2T5VmUQORO6ydnUME1ubrWfD8o4FfibEXVHEWnXdcJlL8p9Pei6AfjjPcqBOkyrVH7LdH9lBKUH jaX7r/F9GpT3fAa6ttm7vh/I= X-Received: by 127.0.0.2 with SMTP id TaOxYY7687511xq4NcTGtRAK; Wed, 08 Nov 2023 11:47:38 -0800 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.web11.24530.1699472857208090121 for ; Wed, 08 Nov 2023 11:47:37 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-604-JesFYQDJPlKePlUTl8HipQ-1; Wed, 08 Nov 2023 14:47:34 -0500 X-MC-Unique: JesFYQDJPlKePlUTl8HipQ-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (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 AAC763C10146; Wed, 8 Nov 2023 19:47:33 +0000 (UTC) X-Received: from [10.39.192.41] (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 402362166B27; Wed, 8 Nov 2023 19:47:32 +0000 (UTC) Message-ID: <2c01fd05-932f-d3c3-1188-cfea9292efab@redhat.com> Date: Wed, 8 Nov 2023 20:47:30 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v4 1/4] StandaloneMmPkg/Core: Limit FwVol encapsulation section recursion To: devel@edk2.groups.io, wei6.xu@intel.com Cc: Ard Biesheuvel , Sami Mujawar , Ray Ni References: <0e4c7373de1592b4349903bbc28aca7ffa46351a.1699253390.git.wei6.xu@intel.com> From: "Laszlo Ersek" In-Reply-To: <0e4c7373de1592b4349903bbc28aca7ffa46351a.1699253390.git.wei6.xu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 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: XCJ25Fcmw0UyfsvKHTjyLGZ8x7686176AA= 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=lJjJ26oj; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/6/23 08:52, Xu, Wei6 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/Dispatcher.c | 5 ----- > StandaloneMmPkg/Core/FwVol.c | 16 ++++++++++++-- > StandaloneMmPkg/Core/StandaloneMmCore.c | 7 +----- > StandaloneMmPkg/Core/StandaloneMmCore.h | 26 +++++++++++++++++++++++ > StandaloneMmPkg/Core/StandaloneMmCore.inf | 3 +++ > StandaloneMmPkg/StandaloneMmPkg.dec | 5 +++++ > 6 files changed, 49 insertions(+), 13 deletions(-) >=20 > diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dis= patcher.c > index b1ccba15b060..7b4a3c4c552b 100644 > --- a/StandaloneMmPkg/Core/Dispatcher.c > +++ b/StandaloneMmPkg/Core/Dispatcher.c > @@ -53,11 +53,6 @@ typedef struct { > // Function Prototypes > // > =20 > -EFI_STATUS > -MmCoreFfsFindMmDriver ( > - IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > - ); > - > /** > Insert InsertedDriverEntry onto the mScheduledQueue. To do this you > must add any driver with a before dependency on InsertedDriverEntry fi= rst. Whoa, so we actually had an (unused) declaration in "Dispatcher.c" as well, which we missed in v3 altogether. I assume now that the declaration is moved to the header file, the compiler reports the conflict! In v3 this declaration actually got out of sync, IIUC. So the declaration centralizaton has already paid off. Reviewed-by: Laszlo Ersek Laszlo > 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..1074f309d718 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.c > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c > @@ -9,11 +9,6 @@ > =20 > #include "StandaloneMmCore.h" > =20 > -EFI_STATUS > -MmCoreFfsFindMmDriver ( > - IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > - ); > - > EFI_STATUS > MmDispatcher ( > VOID > @@ -643,7 +638,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.h b/StandaloneMmPkg/Co= re/StandaloneMmCore.h > index 822d95358c39..da23b8dc3c71 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.h > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h > @@ -846,6 +846,32 @@ DumpMmramInfo ( > VOID > ); > =20 > +/** > + Given the pointer to the Firmware Volume Header find the > + MM driver and return its PE32 image. > + > + @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. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_NOT_FOUND Could not find section data. > + @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. > + > +**/ > +EFI_STATUS > +MmCoreFfsFindMmDriver ( > + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader, > + IN UINT32 Depth > + ); > + > extern UINTN mMmramRangeCount; > extern EFI_MMRAM_DESCRIPTOR *mMmramRanges; > extern EFI_SYSTEM_TABLE *mEfiSystemTable; > 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 -=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 (#110921): https://edk2.groups.io/g/devel/message/110921 Mute This Topic: https://groups.io/mt/102415999/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-