[edk2-devel] [PATCH v4 0/4] StandaloneMmCore finds drivers in uncompressed inner fv.

[edk2-devel] [PATCH v4 0/4] StandaloneMmCore finds drivers in uncompressed inner fv.

[Xu, Wei6]

V1:
This patch is to fix the issue that StandaloneMmCore fails to detect uncompressed inner FV.
PR: https://github.com/tianocore/edk2/pull/4943

V2:
Based on V1, fix some other issues
1. Add Missing object size checks before casting pointers to header types
  a. InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData; 
     This is introduced in V1, add the size check on SectionDataSize against EFI_FIRMWARE_VOLUME_HEADER
  b. Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
     Use FfsFindSection instead of FfsFindSectionData to avoid pointer casting.
2. Fix potential memory leak issue that ScratchBuffer is not freed when page allocation for DstBuffer fails.
PR: https://github.com/tianocore/edk2/pull/4965

V3:
1. Separate patch per individual issue fix on patch V2.
2. Fix one more issue: Limit FwVol encapsulation section recursion in MmCoreFfsFindMmDriver().
PR: https://github.com/tianocore/edk2/pull/4975

V4:
Patch (1/4): Move the declaration of MmCoreFfsFindMmDriver() from source file to the header file "StandaloneMmCore.h"
Patch (2/4): Handle the case that ExtractGuidedSectionDecode()'s decoded buffer is identical to the data in InputSection
Patch (3/4): Fix the issue 'Section + 1' migth be a wrong address for InnerFvHeader if Section is EFI_COMMON_SECTION_HEADER2.
Patch (4/4): 'Continue' if an EFI_SECTION_FIRMWARE_VOLUME_IMAGE is found, do not look for an EFI_SECTION_GUID_DEFINED again.
PR: https://github.com/tianocore/edk2/pull/5004

For the recursion logic improvement, Let's do it in other patch set in future after this patch is committed.

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>

Wei6 Xu (4):
  StandaloneMmPkg/Core: Limit FwVol encapsulation section recursion
  StandaloneMmPkg/Core: Fix potential memory leak issue
  StandaloneMmPkg/Core: Fix issue that offset calculation might be wrong
  StandaloneMmPkg/Core: Fix the failure to find uncompressed inner FV

 StandaloneMmPkg/Core/Dispatcher.c         |  5 --
 StandaloneMmPkg/Core/FwVol.c              | 88 +++++++++++++++++------
 StandaloneMmPkg/Core/StandaloneMmCore.c   |  7 +-
 StandaloneMmPkg/Core/StandaloneMmCore.h   | 26 +++++++
 StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 +
 StandaloneMmPkg/StandaloneMmPkg.dec       |  5 ++
 6 files changed, 103 insertions(+), 31 deletions(-)

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110736): https://edk2.groups.io/g/devel/message/110736
Mute This Topic: https://groups.io/mt/102415997/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Xu, Wei6]

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.
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
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110737): https://edk2.groups.io/g/devel/message/110737
Mute This Topic: https://groups.io/mt/102415999/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue

[Xu, Wei6]

In MmCoreFfsFindMmDriver(),
- ScratchBuffer is not freed in the error return path that DstBuffer page
allocation fails. Free ScratchBuffer before return with error.
- If the decoded buffer is identical to the data in InputSection,
ExtractGuidedSectionDecode() will change the value of DstBuffer rather
than changing the contents of the buffer that DstBuffer points at, in
which case freeing DstBuffer is wrong. Introduce a local variable
AllocatedDstBuffer for buffer free, free AllocatedDstBuffer immediately
if it is not used.

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/FwVol.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index e1e20ffd14ac..c3054ef751ed 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -84,6 +84,7 @@ MmCoreFfsFindMmDriver (
   UINT32                      DstBufferSize;
   VOID                        *ScratchBuffer;
   UINT32                      ScratchBufferSize;
+  VOID                        *AllocatedDstBuffer;
   VOID                        *DstBuffer;
   UINT16                      SectionAttribute;
   UINT32                      AuthenticationStatus;
@@ -148,25 +149,35 @@ MmCoreFfsFindMmDriver (
     //
     // Allocate destination buffer, extra one page for adjustment
     //
-    DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
-    if (DstBuffer == NULL) {
+    AllocatedDstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
+    if (AllocatedDstBuffer == NULL) {
+      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
       return EFI_OUT_OF_RESOURCES;
     }
 
     //
     // Call decompress function
     //
-    Status = ExtractGuidedSectionDecode (
-               Section,
-               &DstBuffer,
-               ScratchBuffer,
-               &AuthenticationStatus
-               );
+    DstBuffer = AllocatedDstBuffer;
+    Status    = ExtractGuidedSectionDecode (
+                  Section,
+                  &DstBuffer,
+                  ScratchBuffer,
+                  &AuthenticationStatus
+                  );
     FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
     if (EFI_ERROR (Status)) {
       goto FreeDstBuffer;
     }
 
+    //
+    // Free allocated DstBuffer if it is not used
+    //
+    if (DstBuffer != AllocatedDstBuffer) {
+      FreePages (AllocatedDstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
+      AllocatedDstBuffer = NULL;
+    }
+
     DEBUG ((
       DEBUG_INFO,
       "Processing compressed firmware volume (AuthenticationStatus == %x)\n",
@@ -210,7 +221,9 @@ MmCoreFfsFindMmDriver (
   return EFI_SUCCESS;
 
 FreeDstBuffer:
-  FreePages (DstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
+  if (AllocatedDstBuffer != NULL) {
+    FreePages (AllocatedDstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
+  }
 
   return Status;
 }
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110738): https://edk2.groups.io/g/devel/message/110738
Mute This Topic: https://groups.io/mt/102416000/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 3/4] StandaloneMmPkg/Core: Fix issue that offset calculation might be wrong

[Xu, Wei6]

MmCoreFfsFindMmDriver() assumes FileHeader is EFI_FFS_FILE_HEADER.
If FileHeader is an EFI_FFS_FILE_HEADER2, 'FileHeader + 1' will get a
wrong section address. Use FfsFindSection to get the section directly,
instead of 'FileHeader + 1' to avoid this issue.
MmCoreFfsFindMmDriver() also assumes section is EFI_COMMON_SECTION_HEADER.
If Section is EFI_COMMON_SECTION_HEADER2, 'Section + 1' will get a wrong
wrong InnerFvHeader adress. Add section head detection and calculate the
address accordingly.

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/FwVol.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index c3054ef751ed..4d2b63a448e7 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -79,8 +79,6 @@ MmCoreFfsFindMmDriver (
   UINTN                       DepexSize;
   UINTN                       Index;
   EFI_COMMON_SECTION_HEADER   *Section;
-  VOID                        *SectionData;
-  UINTN                       SectionDataSize;
   UINT32                      DstBufferSize;
   VOID                        *ScratchBuffer;
   UINT32                      ScratchBufferSize;
@@ -117,23 +115,21 @@ MmCoreFfsFindMmDriver (
       break;
     }
 
-    Status = FfsFindSectionData (
+    Status = FfsFindSection (
                EFI_SECTION_GUID_DEFINED,
                FileHeader,
-               &SectionData,
-               &SectionDataSize
+               &Section
                );
     if (EFI_ERROR (Status)) {
       break;
     }
 
-    Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
-    Status  = ExtractGuidedSectionGetInfo (
-                Section,
-                &DstBufferSize,
-                &ScratchBufferSize,
-                &SectionAttribute
-                );
+    Status = ExtractGuidedSectionGetInfo (
+               Section,
+               &DstBufferSize,
+               &ScratchBufferSize,
+               &SectionAttribute
+               );
     if (EFI_ERROR (Status)) {
       break;
     }
@@ -194,8 +190,13 @@ MmCoreFfsFindMmDriver (
       goto FreeDstBuffer;
     }
 
-    InnerFvHeader = (VOID *)(Section + 1);
-    Status        = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
+    if (IS_SECTION2 (Section)) {
+      InnerFvHeader = (VOID *)((EFI_COMMON_SECTION_HEADER2 *)Section + 1);
+    } else {
+      InnerFvHeader = (VOID *)(Section + 1);
+    }
+
+    Status = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
     if (EFI_ERROR (Status)) {
       goto FreeDstBuffer;
     }
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110739): https://edk2.groups.io/g/devel/message/110739
Mute This Topic: https://groups.io/mt/102416001/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 4/4] StandaloneMmPkg/Core: Fix the failure to find uncompressed inner FV

[Xu, Wei6]

The MmCoreFfsFindMmDriver only checks for encapsulated compressed FVs.
When an inner FV is uncompressed, StandaloneMmCore will miss the FV and
all the MM drivers in the FV will not be dispatched.
Add checks for uncompressed inner FV to fix this issue.

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/FwVol.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index 4d2b63a448e7..07500cee41f3 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -79,6 +79,8 @@ MmCoreFfsFindMmDriver (
   UINTN                       DepexSize;
   UINTN                       Index;
   EFI_COMMON_SECTION_HEADER   *Section;
+  VOID                        *SectionData;
+  UINTN                       SectionDataSize;
   UINT32                      DstBufferSize;
   VOID                        *ScratchBuffer;
   UINT32                      ScratchBufferSize;
@@ -115,6 +117,26 @@ MmCoreFfsFindMmDriver (
       break;
     }
 
+    //
+    // Check uncompressed firmware volumes
+    //
+    Status = FfsFindSectionData (
+               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
+               FileHeader,
+               &SectionData,
+               &SectionDataSize
+               );
+    if (!EFI_ERROR (Status)) {
+      if (SectionDataSize > sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
+        InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
+        MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
+        continue;
+      }
+    }
+
+    //
+    // Check compressed firmware volumes
+    //
     Status = FfsFindSection (
                EFI_SECTION_GUID_DEFINED,
                FileHeader,
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110740): https://edk2.groups.io/g/devel/message/110740
Mute This Topic: https://groups.io/mt/102416011/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue

[Laszlo Ersek]

On 11/6/23 08:52, Xu, Wei6 wrote:
> In MmCoreFfsFindMmDriver(),
> - ScratchBuffer is not freed in the error return path that DstBuffer page
> allocation fails. Free ScratchBuffer before return with error.
> - If the decoded buffer is identical to the data in InputSection,
> ExtractGuidedSectionDecode() will change the value of DstBuffer rather
> than changing the contents of the buffer that DstBuffer points at, in
> which case freeing DstBuffer is wrong. Introduce a local variable
> AllocatedDstBuffer for buffer free, free AllocatedDstBuffer immediately
> if it is not used.
> 
> 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/FwVol.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index e1e20ffd14ac..c3054ef751ed 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -84,6 +84,7 @@ MmCoreFfsFindMmDriver (
>    UINT32                      DstBufferSize;
>    VOID                        *ScratchBuffer;
>    UINT32                      ScratchBufferSize;
> +  VOID                        *AllocatedDstBuffer;
>    VOID                        *DstBuffer;
>    UINT16                      SectionAttribute;
>    UINT32                      AuthenticationStatus;
> @@ -148,25 +149,35 @@ MmCoreFfsFindMmDriver (
>      //
>      // Allocate destination buffer, extra one page for adjustment
>      //
> -    DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
> -    if (DstBuffer == NULL) {
> +    AllocatedDstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
> +    if (AllocatedDstBuffer == NULL) {
> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
>        return EFI_OUT_OF_RESOURCES;
>      }
>  
>      //
>      // Call decompress function
>      //
> -    Status = ExtractGuidedSectionDecode (
> -               Section,
> -               &DstBuffer,
> -               ScratchBuffer,
> -               &AuthenticationStatus
> -               );
> +    DstBuffer = AllocatedDstBuffer;
> +    Status    = ExtractGuidedSectionDecode (
> +                  Section,
> +                  &DstBuffer,
> +                  ScratchBuffer,
> +                  &AuthenticationStatus
> +                  );
>      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
>      if (EFI_ERROR (Status)) {
>        goto FreeDstBuffer;
>      }
>  
> +    //
> +    // Free allocated DstBuffer if it is not used
> +    //
> +    if (DstBuffer != AllocatedDstBuffer) {
> +      FreePages (AllocatedDstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
> +      AllocatedDstBuffer = NULL;
> +    }
> +
>      DEBUG ((
>        DEBUG_INFO,
>        "Processing compressed firmware volume (AuthenticationStatus == %x)\n",
> @@ -210,7 +221,9 @@ MmCoreFfsFindMmDriver (
>    return EFI_SUCCESS;
>  
>  FreeDstBuffer:
> -  FreePages (DstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
> +  if (AllocatedDstBuffer != NULL) {
> +    FreePages (AllocatedDstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
> +  }
>  
>    return Status;
>  }

Right, if AllocatedDstBuffer is needed, then we free it only upon error;
otherwise, we free it early on, so that it's released upon both error
and success.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110922): https://edk2.groups.io/g/devel/message/110922
Mute This Topic: https://groups.io/mt/102416000/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 3/4] StandaloneMmPkg/Core: Fix issue that offset calculation might be wrong

[Laszlo Ersek]

On 11/6/23 08:52, Xu, Wei6 wrote:
> MmCoreFfsFindMmDriver() assumes FileHeader is EFI_FFS_FILE_HEADER.
> If FileHeader is an EFI_FFS_FILE_HEADER2, 'FileHeader + 1' will get a
> wrong section address. Use FfsFindSection to get the section directly,
> instead of 'FileHeader + 1' to avoid this issue.
> MmCoreFfsFindMmDriver() also assumes section is EFI_COMMON_SECTION_HEADER.
> If Section is EFI_COMMON_SECTION_HEADER2, 'Section + 1' will get a wrong
> wrong InnerFvHeader adress. Add section head detection and calculate the
> address accordingly.
> 
> 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/FwVol.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index c3054ef751ed..4d2b63a448e7 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -79,8 +79,6 @@ MmCoreFfsFindMmDriver (
>    UINTN                       DepexSize;
>    UINTN                       Index;
>    EFI_COMMON_SECTION_HEADER   *Section;
> -  VOID                        *SectionData;
> -  UINTN                       SectionDataSize;
>    UINT32                      DstBufferSize;
>    VOID                        *ScratchBuffer;
>    UINT32                      ScratchBufferSize;
> @@ -117,23 +115,21 @@ MmCoreFfsFindMmDriver (
>        break;
>      }
>  
> -    Status = FfsFindSectionData (
> +    Status = FfsFindSection (
>                 EFI_SECTION_GUID_DEFINED,
>                 FileHeader,
> -               &SectionData,
> -               &SectionDataSize
> +               &Section
>                 );
>      if (EFI_ERROR (Status)) {
>        break;
>      }
>  
> -    Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
> -    Status  = ExtractGuidedSectionGetInfo (
> -                Section,
> -                &DstBufferSize,
> -                &ScratchBufferSize,
> -                &SectionAttribute
> -                );
> +    Status = ExtractGuidedSectionGetInfo (
> +               Section,
> +               &DstBufferSize,
> +               &ScratchBufferSize,
> +               &SectionAttribute
> +               );
>      if (EFI_ERROR (Status)) {
>        break;
>      }
> @@ -194,8 +190,13 @@ MmCoreFfsFindMmDriver (
>        goto FreeDstBuffer;
>      }
>  
> -    InnerFvHeader = (VOID *)(Section + 1);
> -    Status        = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
> +    if (IS_SECTION2 (Section)) {
> +      InnerFvHeader = (VOID *)((EFI_COMMON_SECTION_HEADER2 *)Section + 1);
> +    } else {
> +      InnerFvHeader = (VOID *)(Section + 1);
> +    }
> +
> +    Status = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
>      if (EFI_ERROR (Status)) {
>        goto FreeDstBuffer;
>      }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110923): https://edk2.groups.io/g/devel/message/110923
Mute This Topic: https://groups.io/mt/102416001/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 4/4] StandaloneMmPkg/Core: Fix the failure to find uncompressed inner FV

[Laszlo Ersek]

On 11/6/23 08:52, Xu, Wei6 wrote:
> The MmCoreFfsFindMmDriver only checks for encapsulated compressed FVs.
> When an inner FV is uncompressed, StandaloneMmCore will miss the FV and
> all the MM drivers in the FV will not be dispatched.
> Add checks for uncompressed inner FV to fix this issue.
> 
> 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/FwVol.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index 4d2b63a448e7..07500cee41f3 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -79,6 +79,8 @@ MmCoreFfsFindMmDriver (
>    UINTN                       DepexSize;
>    UINTN                       Index;
>    EFI_COMMON_SECTION_HEADER   *Section;
> +  VOID                        *SectionData;
> +  UINTN                       SectionDataSize;
>    UINT32                      DstBufferSize;
>    VOID                        *ScratchBuffer;
>    UINT32                      ScratchBufferSize;
> @@ -115,6 +117,26 @@ MmCoreFfsFindMmDriver (
>        break;
>      }
>  
> +    //
> +    // Check uncompressed firmware volumes
> +    //
> +    Status = FfsFindSectionData (
> +               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
> +               FileHeader,
> +               &SectionData,
> +               &SectionDataSize
> +               );
> +    if (!EFI_ERROR (Status)) {
> +      if (SectionDataSize > sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
> +        InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
> +        MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
> +        continue;
> +      }
> +    }
> +
> +    //
> +    // Check compressed firmware volumes
> +    //
>      Status = FfsFindSection (
>                 EFI_SECTION_GUID_DEFINED,
>                 FileHeader,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110924): https://edk2.groups.io/g/devel/message/110924
Mute This Topic: https://groups.io/mt/102416011/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-