Re: [edk2-devel] [PATCH v4 1/4] StandaloneMmPkg/Core: Limit FwVol encapsulation section recursion

["Laszlo Ersek" <lersek@redhat.com>]

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.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
> ---
>  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(-)
> 
> diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c
> index b1ccba15b060..7b4a3c4c552b 100644
> --- a/StandaloneMmPkg/Core/Dispatcher.c
> +++ b/StandaloneMmPkg/Core/Dispatcher.c
> @@ -53,11 +53,6 @@ typedef struct {
>  // Function Prototypes
>  //
>  
> -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 first.

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 <lersek@redhat.com>

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.
>  
>    @param [in] FwVolHeader   Pointer to memory mapped FV
> +  @param [in] Depth         Nesting depth of encapsulation sections. Callers
> +                            different from MmCoreFfsFindMmDriver() are
> +                            responsible for passing in a zero Depth.
>  
>    @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 been
> +                                  greater than or equal to
> +                                  PcdFwVolMmMaxEncapsulationDepth.
>  
>  **/
>  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 (
>  
>    DEBUG ((DEBUG_INFO, "MmCoreFfsFindMmDriver - 0x%x\n", FwVolHeader));
>  
> +  if (Depth >= 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 (
>      }
>  
>      InnerFvHeader = (VOID *)(Section + 1);
> -    Status        = MmCoreFfsFindMmDriver (InnerFvHeader);
> +    Status        = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
>      if (EFI_ERROR (Status)) {
>        goto FreeDstBuffer;
>      }
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c
> index d221f1d1115d..1074f309d718 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.c
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
> @@ -9,11 +9,6 @@
>  
>  #include "StandaloneMmCore.h"
>  
> -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", gMmCorePrivate->StandaloneBfvAddress));
>    if (gMmCorePrivate->StandaloneBfvAddress != 0) {
> -    MmCoreFfsFindMmDriver ((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)gMmCorePrivate->StandaloneBfvAddress);
> +    MmCoreFfsFindMmDriver ((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)gMmCorePrivate->StandaloneBfvAddress, 0);
>      MmDispatcher ();
>    }
>  
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h
> index 822d95358c39..da23b8dc3c71 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
> @@ -846,6 +846,32 @@ DumpMmramInfo (
>    VOID
>    );
>  
> +/**
> +  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. Callers
> +                            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 been
> +                                  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
>  
> +[Pcd]
> +  gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth    ##CONSUMES
> +
>  #
>  # This configuration fails for CLANGPDB, which does not support PIE in the GCC
>  # sense. Such however is required for ARM family StandaloneMmCore
> diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec b/StandaloneMmPkg/StandaloneMmPkg.dec
> index 46784d94e421..c43632d6d8ae 100644
> --- a/StandaloneMmPkg/StandaloneMmPkg.dec
> +++ b/StandaloneMmPkg/StandaloneMmPkg.dec
> @@ -48,3 +48,8 @@ [Guids]
>    gEfiStandaloneMmNonSecureBufferGuid      = { 0xf00497e3, 0xbfa2, 0x41a1, { 0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }}
>    gEfiArmTfCpuDriverEpDescriptorGuid       = { 0x6ecbd5a1, 0xc0f8, 0x4702, { 0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }}
>  
> +[PcdsFixedAtBuild, PcdsPatchableInModule]
> +  ## Maximum permitted encapsulation levels of sections in a firmware volume,
> +  #  in the MM phase. Minimum value is 1. Sections nested more deeply are rejected.
> +  # @Prompt Maximum permitted FwVol section nesting depth (exclusive) in MM.
> +  gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth|0x10|UINT32|0x00000001



-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-