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

[edk2-devel] [PATCH v3 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


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 section address might be wrong
  StandaloneMmPkg/Core: Fix the failure to find uncompressed inner FV

 StandaloneMmPkg/Core/FwVol.c              | 50 ++++++++++++++++++-----
 StandaloneMmPkg/Core/StandaloneMmCore.c   |  5 ++-
 StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 ++
 StandaloneMmPkg/StandaloneMmPkg.dec       |  5 +++
 4 files changed, 51 insertions(+), 12 deletions(-)

-- 
2.29.2.windows.2



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

[edk2-devel] [PATCH v3 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/FwVol.c              | 16 ++++++++++++++--
 StandaloneMmPkg/Core/StandaloneMmCore.c   |  5 +++--
 StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 +++
 StandaloneMmPkg/StandaloneMmPkg.dec       |  5 +++++
 4 files changed, 25 insertions(+), 4 deletions(-)

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..523ea0a632a1 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -11,7 +11,8 @@
 
 EFI_STATUS
 MmCoreFfsFindMmDriver (
-  IN  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader
+  IN  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader,
+  IN  UINT32                      Depth
   );
 
 EFI_STATUS
@@ -643,7 +644,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.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 (#110297): https://edk2.groups.io/g/devel/message/110297
Mute This Topic: https://groups.io/mt/102270546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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.
> 
> 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              | 16 ++++++++++++++--
>  StandaloneMmPkg/Core/StandaloneMmCore.c   |  5 +++--
>  StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 +++
>  StandaloneMmPkg/StandaloneMmPkg.dec       |  5 +++++
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> 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..523ea0a632a1 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.c
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
> @@ -11,7 +11,8 @@
>  
>  EFI_STATUS
>  MmCoreFfsFindMmDriver (
> -  IN  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader
> +  IN  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader,
> +  IN  UINT32                      Depth
>    );
>  
>  EFI_STATUS
> @@ -643,7 +644,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.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

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

Thank you!
Laszlo



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

[edk2-devel] [PATCH v3 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.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index e1e20ffd14ac..9d0ce66ef839 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
     //
     DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
     if (DstBuffer == NULL) {
+      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
       return EFI_OUT_OF_RESOURCES;
     }
 
-- 
2.29.2.windows.2



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

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

[Laszlo Ersek]

On 10/30/23 08:49, Wei6 Xu wrote:
> In MmCoreFfsFindMmDriver(), ScratchBuffer is not freed in the error
> return path that DstBuffer page allocation fails. Free ScratchBuffer
> before return with error.
> 
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index e1e20ffd14ac..9d0ce66ef839 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
>      //
>      DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
>      if (DstBuffer == NULL) {
> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
>        return EFI_OUT_OF_RESOURCES;
>      }
>  

This patch is good, with regard to ScratchBuffer:

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

However, upon further staring at the code, I think that we have a
DstBuffer life-cycle problem as well, independently of ScratchBuffer:

(1) ExtractGuidedSectionDecode() does not necessarily use the
caller-allocated buffer. The library class header file
"MdePkg/Include/Library/ExtractGuidedSectionLib.h" says that, "If the
decoded buffer is identical to the data in InputSection, then
OutputBuffer is set to point at the data in InputSection.  Otherwise,
the decoded data will be placed in caller allocated buffer specified by
OutputBuffer."

This means that the ExtractGuidedSectionDecode() call may change the
value of DstBuffer (rather than changing the contents of the buffer that
DstBuffer points at) -- in which case freeing DstBuffer is wrong.

This means we need a second variable. One variable needs to preserve the
allocation address, and the address of the other variable must be passed
to ExtractGuidedSectionDecode(). After the call, we need to free the
*original* variable (the one that ExtractGuidedSectionDecode() could not
have overwritten).

(2) As far as I can tell, we leak our original DstBuffer allocation in
two cases:

- Upon every iteration of the loop after the first iteration, we
overwrite the DstBuffer variable with the new allocation address. The
old one is lost (leaked).

My understanding is that, after the recursive MmCoreFfsFindMmDriver()
call returns, we no longer need the decompressed DstBuffer, therefore,
we should free the *original* DstBuffer allocation (per (1)) right there.

- The last (potentially: only one) iteration of the loop allocates
DstBuffer, and that allocation is never freed. We don't overwrite the
address with a new allocation's address, but still we never free the
original allocation. The FreeDstBuffer label is apparently never reached.

(3) And finally, a logic bug (or at least questionable behavior):

The loop at the *top* of the function scans the firmware volume for
embedded firmware volumes (recursing into them if any are found), while
the loop at the *bottom* of the function scans the *same* firmware
volume for MM driver binaries (adding them to the "MM driver list"),
starting anew from the beginning of the firmware volume.

Now, there are many exit points in the function-top loop. Those can be
classified in two groups: "break", and "return/goto". The former class
makes sense. The latter class does not seem to make sense to me.

Consider: just because we fail to scan the firmware volume for embedded
firmware volumes, for any reason really, should we really abandon
scanning the same firmware volume for MM driver binaries? What I don't
understand here in particular is the *inconsistency* between the exit
points, in the function-top loop:

- if we realize there are no (more) embedded FVs, we break out; good

- if we realize the next embedded FV is not "GUID defined", we break
out; good (well, questionable -- perhaps we should continue scanning?
the next embedded FV could be GUID defined after all!)

- if ExtractGuidedSectionGetInfo() fails, we break out again; good (or,
well, we could continue the scanning, but anyway)

- if the *decoding* fails, including the allocations, or we fail to find
a proper FV image section, or the recursive MmCoreFfsFindMmDriver() call
fails, then we *abandon* the MM driver images in the *current* firmware
image. That is what does not make any sense to me, compared to the
above-noted exit points. Just because we couldn't extract a compressed,
embedded FV image, why ignore the MM drivers in *this* image?

Sorry for creating more and more work for you, but I'm starting to think
that the whole loop should be rewritten. :/

Well, even if we don't change this scanning logic, at least properly
releasing DstBuffer would be nice (i.e., addressing points (1) and (2)).

Thanks for bearing with me
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110314): https://edk2.groups.io/g/devel/message/110314
Mute This Topic: https://groups.io/mt/102270547/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 v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue

[Xu, Wei6]

Thanks a lot for reviewing the patch.
I have different opinions with (2), could you please check that? Thanks a lot.
If you agree (2) is not an issue, I will prepare a new patch version to only address (1) and (3)

BR,
Wei
>-----Original Message-----
>From: Laszlo Ersek <lersek@redhat.com>
>Sent: Monday, October 30, 2023 8:25 PM
>To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
>Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
><sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>Subject: Re: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory
>leak issue
>
>On 10/30/23 08:49, Wei6 Xu wrote:
>> In MmCoreFfsFindMmDriver(), ScratchBuffer is not freed in the error
>> return path that DstBuffer page allocation fails. Free ScratchBuffer
>> before return with error.
>>
>> 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/StandaloneMmPkg/Core/FwVol.c
>> b/StandaloneMmPkg/Core/FwVol.c index e1e20ffd14ac..9d0ce66ef839
>100644
>> --- a/StandaloneMmPkg/Core/FwVol.c
>> +++ b/StandaloneMmPkg/Core/FwVol.c
>> @@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
>>      //
>>      DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES
>(DstBufferSize));
>>      if (DstBuffer == NULL) {
>> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES
>> + (ScratchBufferSize));
>>        return EFI_OUT_OF_RESOURCES;
>>      }
>>
>
>This patch is good, with regard to ScratchBuffer:
>
>Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>However, upon further staring at the code, I think that we have a DstBuffer
>life-cycle problem as well, independently of ScratchBuffer:
>
>(1) ExtractGuidedSectionDecode() does not necessarily use the caller-
>allocated buffer. The library class header file
>"MdePkg/Include/Library/ExtractGuidedSectionLib.h" says that, "If the
>decoded buffer is identical to the data in InputSection, then OutputBuffer is
>set to point at the data in InputSection.  Otherwise, the decoded data will be
>placed in caller allocated buffer specified by OutputBuffer."
>
>This means that the ExtractGuidedSectionDecode() call may change the value
>of DstBuffer (rather than changing the contents of the buffer that DstBuffer
>points at) -- in which case freeing DstBuffer is wrong.
>
>This means we need a second variable. One variable needs to preserve the
>allocation address, and the address of the other variable must be passed to
>ExtractGuidedSectionDecode(). After the call, we need to free the
>*original* variable (the one that ExtractGuidedSectionDecode() could not
>have overwritten).
>

Will prepare a new patch version to address this.

>(2) As far as I can tell, we leak our original DstBuffer allocation in two cases:
>
>- Upon every iteration of the loop after the first iteration, we overwrite the
>DstBuffer variable with the new allocation address. The old one is lost (leaked).
>
>My understanding is that, after the recursive MmCoreFfsFindMmDriver() call
>returns, we no longer need the decompressed DstBuffer, therefore, we
>should free the *original* DstBuffer allocation (per (1)) right there.
>
>- The last (potentially: only one) iteration of the loop allocates DstBuffer, and
>that allocation is never freed. We don't overwrite the address with a new
>allocation's address, but still we never free the original allocation. The
>FreeDstBuffer label is apparently never reached.
>

In the success case, DstBuffer should NOT be freed, because the buffer holds the MM drivers, which will be used in the driver dispatch process later.

>(3) And finally, a logic bug (or at least questionable behavior):
>
>The loop at the *top* of the function scans the firmware volume for
>embedded firmware volumes (recursing into them if any are found), while
>the loop at the *bottom* of the function scans the *same* firmware volume
>for MM driver binaries (adding them to the "MM driver list"), starting anew
>from the beginning of the firmware volume.
>
>Now, there are many exit points in the function-top loop. Those can be
>classified in two groups: "break", and "return/goto". The former class makes
>sense. The latter class does not seem to make sense to me.
>
>Consider: just because we fail to scan the firmware volume for embedded
>firmware volumes, for any reason really, should we really abandon scanning
>the same firmware volume for MM driver binaries? What I don't understand
>here in particular is the *inconsistency* between the exit points, in the
>function-top loop:
>
>- if we realize there are no (more) embedded FVs, we break out; good
>
>- if we realize the next embedded FV is not "GUID defined", we break out;
>good (well, questionable -- perhaps we should continue scanning?
>the next embedded FV could be GUID defined after all!)
>

If the FfsFindSectionData with EFI_SECTION_GUID_DEFINED fails, it means there is no GUID defined embedded FV at all in current FwVol. No need to continue scanning.

>- if ExtractGuidedSectionGetInfo() fails, we break out again; good (or, well, we
>could continue the scanning, but anyway)

Will prepare a new patch version to address this: change break to continue

>
>- if the *decoding* fails, including the allocations, or we fail to find a proper FV
>image section, or the recursive MmCoreFfsFindMmDriver() call fails, then we
>*abandon* the MM driver images in the *current* firmware image. That is
>what does not make any sense to me, compared to the above-noted exit
>points. Just because we couldn't extract a compressed, embedded FV image,
>why ignore the MM drivers in *this* image?
>

Will prepare a new patch version to address this: move the MM drivers detect logic to the front of the while-loop, which mean first check the MM drivers, then check the embedded FVs

>Sorry for creating more and more work for you, but I'm starting to think that
>the whole loop should be rewritten. :/
>
>Well, even if we don't change this scanning logic, at least properly releasing
>DstBuffer would be nice (i.e., addressing points (1) and (2)).
>
>Thanks for bearing with me
>Laszlo



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

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

[Xu, Wei6]

Delete one my wrong comments.

-----Original Message-----
From: Xu, Wei6 
Sent: Tuesday, October 31, 2023 2:40 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue

Thanks a lot for reviewing the patch.
I have different opinions with (2), could you please check that? Thanks a lot.
If you agree (2) is not an issue, I will prepare a new patch version to only address (1) and (3)

BR,
Wei
>-----Original Message-----
>From: Laszlo Ersek <lersek@redhat.com>
>Sent: Monday, October 30, 2023 8:25 PM
>To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
>Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar 
><sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>Subject: Re: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory 
>leak issue
>
>On 10/30/23 08:49, Wei6 Xu wrote:
>> In MmCoreFfsFindMmDriver(), ScratchBuffer is not freed in the error 
>> return path that DstBuffer page allocation fails. Free ScratchBuffer 
>> before return with error.
>>
>> 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/StandaloneMmPkg/Core/FwVol.c 
>> b/StandaloneMmPkg/Core/FwVol.c index e1e20ffd14ac..9d0ce66ef839
>100644
>> --- a/StandaloneMmPkg/Core/FwVol.c
>> +++ b/StandaloneMmPkg/Core/FwVol.c
>> @@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
>>      //
>>      DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES
>(DstBufferSize));
>>      if (DstBuffer == NULL) {
>> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES 
>> + (ScratchBufferSize));
>>        return EFI_OUT_OF_RESOURCES;
>>      }
>>
>
>This patch is good, with regard to ScratchBuffer:
>
>Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>However, upon further staring at the code, I think that we have a 
>DstBuffer life-cycle problem as well, independently of ScratchBuffer:
>
>(1) ExtractGuidedSectionDecode() does not necessarily use the caller- 
>allocated buffer. The library class header file 
>"MdePkg/Include/Library/ExtractGuidedSectionLib.h" says that, "If the 
>decoded buffer is identical to the data in InputSection, then 
>OutputBuffer is set to point at the data in InputSection.  Otherwise, 
>the decoded data will be placed in caller allocated buffer specified by OutputBuffer."
>
>This means that the ExtractGuidedSectionDecode() call may change the 
>value of DstBuffer (rather than changing the contents of the buffer 
>that DstBuffer points at) -- in which case freeing DstBuffer is wrong.
>
>This means we need a second variable. One variable needs to preserve 
>the allocation address, and the address of the other variable must be 
>passed to ExtractGuidedSectionDecode(). After the call, we need to free 
>the
>*original* variable (the one that ExtractGuidedSectionDecode() could 
>not have overwritten).
>

Will prepare a new patch version to address this.

>(2) As far as I can tell, we leak our original DstBuffer allocation in two cases:
>
>- Upon every iteration of the loop after the first iteration, we 
>overwrite the DstBuffer variable with the new allocation address. The old one is lost (leaked).
>
>My understanding is that, after the recursive MmCoreFfsFindMmDriver() 
>call returns, we no longer need the decompressed DstBuffer, therefore, 
>we should free the *original* DstBuffer allocation (per (1)) right there.
>
>- The last (potentially: only one) iteration of the loop allocates 
>DstBuffer, and that allocation is never freed. We don't overwrite the 
>address with a new allocation's address, but still we never free the 
>original allocation. The FreeDstBuffer label is apparently never reached.
>

In the success case, DstBuffer should NOT be freed, because the buffer holds the MM drivers, which will be used in the driver dispatch process later.

>(3) And finally, a logic bug (or at least questionable behavior):
>
>The loop at the *top* of the function scans the firmware volume for 
>embedded firmware volumes (recursing into them if any are found), while 
>the loop at the *bottom* of the function scans the *same* firmware 
>volume for MM driver binaries (adding them to the "MM driver list"), 
>starting anew from the beginning of the firmware volume.
>
>Now, there are many exit points in the function-top loop. Those can be 
>classified in two groups: "break", and "return/goto". The former class 
>makes sense. The latter class does not seem to make sense to me.
>
>Consider: just because we fail to scan the firmware volume for embedded 
>firmware volumes, for any reason really, should we really abandon 
>scanning the same firmware volume for MM driver binaries? What I don't 
>understand here in particular is the *inconsistency* between the exit 
>points, in the function-top loop:
>
>- if we realize there are no (more) embedded FVs, we break out; good
>
>- if we realize the next embedded FV is not "GUID defined", we break 
>out; good (well, questionable -- perhaps we should continue scanning?
>the next embedded FV could be GUID defined after all!)
>
>- if ExtractGuidedSectionGetInfo() fails, we break out again; good (or, 
>well, we could continue the scanning, but anyway)

Will prepare a new patch version to address this: change break to continue

>
>- if the *decoding* fails, including the allocations, or we fail to 
>find a proper FV image section, or the recursive 
>MmCoreFfsFindMmDriver() call fails, then we
>*abandon* the MM driver images in the *current* firmware image. That is 
>what does not make any sense to me, compared to the above-noted exit 
>points. Just because we couldn't extract a compressed, embedded FV 
>image, why ignore the MM drivers in *this* image?
>

Will prepare a new patch version to address this: move the MM drivers detect logic to the front of the while-loop, which mean first check the MM drivers, then check the embedded FVs

>Sorry for creating more and more work for you, but I'm starting to 
>think that the whole loop should be rewritten. :/
>
>Well, even if we don't change this scanning logic, at least properly 
>releasing DstBuffer would be nice (i.e., addressing points (1) and (2)).
>
>Thanks for bearing with me
>Laszlo



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

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

[Laszlo Ersek]

comment below

On 10/31/23 09:37, Xu, Wei6 wrote:
> Delete one my wrong comments.
>
> -----Original Message-----
> From: Xu, Wei6
> Sent: Tuesday, October 31, 2023 2:40 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue
>
> Thanks a lot for reviewing the patch.
> I have different opinions with (2), could you please check that?
> Thanks a lot.
> If you agree (2) is not an issue, I will prepare a new patch version
> to only address (1) and (3)
>
> BR,
> Wei
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Monday, October 30, 2023 8:25 PM
>> To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
>> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential
>> memory leak issue
>>
>> On 10/30/23 08:49, Wei6 Xu wrote:
>>> In MmCoreFfsFindMmDriver(), ScratchBuffer is not freed in the error
>>> return path that DstBuffer page allocation fails. Free ScratchBuffer
>>> before return with error.
>>>
>>> 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 | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/StandaloneMmPkg/Core/FwVol.c
>>> b/StandaloneMmPkg/Core/FwVol.c index e1e20ffd14ac..9d0ce66ef839
>> 100644
>>> --- a/StandaloneMmPkg/Core/FwVol.c
>>> +++ b/StandaloneMmPkg/Core/FwVol.c
>>> @@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
>>>      //
>>>      DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES
>> (DstBufferSize));
>>>      if (DstBuffer == NULL) {
>>> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES
>>> + (ScratchBufferSize));
>>>        return EFI_OUT_OF_RESOURCES;
>>>      }
>>>
>>
>> This patch is good, with regard to ScratchBuffer:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> However, upon further staring at the code, I think that we have a
>> DstBuffer life-cycle problem as well, independently of ScratchBuffer:
>>
>> (1) ExtractGuidedSectionDecode() does not necessarily use the caller-
>> allocated buffer. The library class header file
>> "MdePkg/Include/Library/ExtractGuidedSectionLib.h" says that, "If the
>> decoded buffer is identical to the data in InputSection, then
>> OutputBuffer is set to point at the data in InputSection.  Otherwise,
>> the decoded data will be placed in caller allocated buffer specified
>> by OutputBuffer."
>>
>> This means that the ExtractGuidedSectionDecode() call may change the
>> value of DstBuffer (rather than changing the contents of the buffer
>> that DstBuffer points at) -- in which case freeing DstBuffer is
>> wrong.
>>
>> This means we need a second variable. One variable needs to preserve
>> the allocation address, and the address of the other variable must be
>> passed to ExtractGuidedSectionDecode(). After the call, we need to
>> free the *original* variable (the one that
>> ExtractGuidedSectionDecode() could not have overwritten).
>>
>
> Will prepare a new patch version to address this.
>
>> (2) As far as I can tell, we leak our original DstBuffer allocation
>> in two cases:
>>
>> - Upon every iteration of the loop after the first iteration, we
>> overwrite the DstBuffer variable with the new allocation address. The
>> old one is lost (leaked).
>>
>> My understanding is that, after the recursive MmCoreFfsFindMmDriver()
>> call returns, we no longer need the decompressed DstBuffer,
>> therefore, we should free the *original* DstBuffer allocation (per
>> (1)) right there.
>>
>> - The last (potentially: only one) iteration of the loop allocates
>> DstBuffer, and that allocation is never freed. We don't overwrite the
>> address with a new allocation's address, but still we never free the
>> original allocation. The FreeDstBuffer label is apparently never
>> reached.
>>
>
> In the success case, DstBuffer should NOT be freed, because the buffer
> holds the MM drivers, which will be used in the driver dispatch
> process later.

Ouch, good point! MmAddToDriverList() only links the driver image into a
list ("mDiscoveredList").

Okay, but then we can still improve the code:

* if ExtractGuidedSectionDecode() reports that it did not use DstBuffer
  (i.e., it outputs a pointer pointing back into the input blob), then
  there is no reason to preserve the original allocation. Especially
  because the allocation is in MM RAM, which is a scarce resources.

* this is more like a question than a suggestion. Do you know if the
  drivers linked into "mDiscoveredList" execute "in place" (from the
  DstBuffer allocation(s)), or if they are never again needed when after
  the Standalone MM dispatches has actually loaded and launched them?
  Because in the latter case, it would be nice to release the original
  DstBuffer allocations; otherwise they just waste MM RAM. (Either way,
  I agree this is probably out of scope for now.)

* Consider the following comment, and global variable definition, in
  "StandaloneMmPkg/Core/Dispatcher.c":

> //
> // The Driver List contains one copy of every driver that has been discovered.
> // Items are never removed from the driver list. List of EFI_MM_DRIVER_ENTRY
> //
> LIST_ENTRY  mDiscoveredList = INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);

So, I don't understand this. The comment says *one copy* (emphasis
mine).

If the comment is right, then we can release DstBuffer immediately after
MmAddToDriverList().

If the comment is wrong, and MmAddToDriverList() indeed only *links* the
images into a list (which certainly seems to be the case), then the
comment is wrong, and should be fixed. It's fine to say that items are
never removed from the driver list, but

  one copy of

should be replaced with

  one non-owner reference to

Thanks!
Laszlo

>
>> (3) And finally, a logic bug (or at least questionable behavior):
>>
>> The loop at the *top* of the function scans the firmware volume for
>> embedded firmware volumes (recursing into them if any are found), while
>> the loop at the *bottom* of the function scans the *same* firmware
>> volume for MM driver binaries (adding them to the "MM driver list"),
>> starting anew from the beginning of the firmware volume.
>>
>> Now, there are many exit points in the function-top loop. Those can be
>> classified in two groups: "break", and "return/goto". The former class
>> makes sense. The latter class does not seem to make sense to me.
>>
>> Consider: just because we fail to scan the firmware volume for embedded
>> firmware volumes, for any reason really, should we really abandon
>> scanning the same firmware volume for MM driver binaries? What I don't
>> understand here in particular is the *inconsistency* between the exit
>> points, in the function-top loop:
>>
>> - if we realize there are no (more) embedded FVs, we break out; good
>>
>> - if we realize the next embedded FV is not "GUID defined", we break
>> out; good (well, questionable -- perhaps we should continue scanning?
>> the next embedded FV could be GUID defined after all!)
>>
>> - if ExtractGuidedSectionGetInfo() fails, we break out again; good (or,
>> well, we could continue the scanning, but anyway)
>
> Will prepare a new patch version to address this: change break to continue
>
>>
>> - if the *decoding* fails, including the allocations, or we fail to
>> find a proper FV image section, or the recursive
>> MmCoreFfsFindMmDriver() call fails, then we
>> *abandon* the MM driver images in the *current* firmware image. That is
>> what does not make any sense to me, compared to the above-noted exit
>> points. Just because we couldn't extract a compressed, embedded FV
>> image, why ignore the MM drivers in *this* image?
>>
>
> Will prepare a new patch version to address this: move the MM drivers detect logic to the front of the while-loop, which mean first check the MM drivers, then check the embedded FVs
>
>> Sorry for creating more and more work for you, but I'm starting to
>> think that the whole loop should be rewritten. :/
>>
>> Well, even if we don't change this scanning logic, at least properly
>> releasing DstBuffer would be nice (i.e., addressing points (1) and (2)).
>>
>> Thanks for bearing with me
>> Laszlo
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110400): https://edk2.groups.io/g/devel/message/110400
Mute This Topic: https://groups.io/mt/102270547/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 v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue

[Xu, Wei6]

Sorry for the late response. Thanks a lot for the review.
Comment below.

BR,
Wei

>-----Original Message-----
>From: Laszlo Ersek <lersek@redhat.com>
>Sent: Tuesday, October 31, 2023 7:43 PM
>To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
>Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
><sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>Subject: Re: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory
>leak issue
>
>comment below
>
>On 10/31/23 09:37, Xu, Wei6 wrote:
>> Delete one my wrong comments.
>>
>> -----Original Message-----
>> From: Xu, Wei6
>> Sent: Tuesday, October 31, 2023 2:40 PM
>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
>> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: RE: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory
>> leak issue
>>
>> Thanks a lot for reviewing the patch.
>> I have different opinions with (2), could you please check that?
>> Thanks a lot.
>> If you agree (2) is not an issue, I will prepare a new patch version
>> to only address (1) and (3)
>>
>> BR,
>> Wei
>>> -----Original Message-----
>>> From: Laszlo Ersek <lersek@redhat.com>
>>> Sent: Monday, October 30, 2023 8:25 PM
>>> To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
>>> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
>>> Subject: Re: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential
>>> memory leak issue
>>>
>>> On 10/30/23 08:49, Wei6 Xu wrote:
>>>> In MmCoreFfsFindMmDriver(), ScratchBuffer is not freed in the error
>>>> return path that DstBuffer page allocation fails. Free ScratchBuffer
>>>> before return with error.
>>>>
>>>> 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 | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/StandaloneMmPkg/Core/FwVol.c
>>>> b/StandaloneMmPkg/Core/FwVol.c index e1e20ffd14ac..9d0ce66ef839
>>> 100644
>>>> --- a/StandaloneMmPkg/Core/FwVol.c
>>>> +++ b/StandaloneMmPkg/Core/FwVol.c
>>>> @@ -150,6 +150,7 @@ MmCoreFfsFindMmDriver (
>>>>      //
>>>>      DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES
>>> (DstBufferSize));
>>>>      if (DstBuffer == NULL) {
>>>> +      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES
>>>> + (ScratchBufferSize));
>>>>        return EFI_OUT_OF_RESOURCES;
>>>>      }
>>>>
>>>
>>> This patch is good, with regard to ScratchBuffer:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> However, upon further staring at the code, I think that we have a
>>> DstBuffer life-cycle problem as well, independently of ScratchBuffer:
>>>
>>> (1) ExtractGuidedSectionDecode() does not necessarily use the caller-
>>> allocated buffer. The library class header file
>>> "MdePkg/Include/Library/ExtractGuidedSectionLib.h" says that, "If the
>>> decoded buffer is identical to the data in InputSection, then
>>> OutputBuffer is set to point at the data in InputSection.  Otherwise,
>>> the decoded data will be placed in caller allocated buffer specified
>>> by OutputBuffer."
>>>
>>> This means that the ExtractGuidedSectionDecode() call may change the
>>> value of DstBuffer (rather than changing the contents of the buffer
>>> that DstBuffer points at) -- in which case freeing DstBuffer is
>>> wrong.
>>>
>>> This means we need a second variable. One variable needs to preserve
>>> the allocation address, and the address of the other variable must be
>>> passed to ExtractGuidedSectionDecode(). After the call, we need to
>>> free the *original* variable (the one that
>>> ExtractGuidedSectionDecode() could not have overwritten).
>>>
>>
>> Will prepare a new patch version to address this.
>>
>>> (2) As far as I can tell, we leak our original DstBuffer allocation
>>> in two cases:
>>>
>>> - Upon every iteration of the loop after the first iteration, we
>>> overwrite the DstBuffer variable with the new allocation address. The
>>> old one is lost (leaked).
>>>
>>> My understanding is that, after the recursive MmCoreFfsFindMmDriver()
>>> call returns, we no longer need the decompressed DstBuffer,
>>> therefore, we should free the *original* DstBuffer allocation (per
>>> (1)) right there.
>>>
>>> - The last (potentially: only one) iteration of the loop allocates
>>> DstBuffer, and that allocation is never freed. We don't overwrite the
>>> address with a new allocation's address, but still we never free the
>>> original allocation. The FreeDstBuffer label is apparently never
>>> reached.
>>>
>>
>> In the success case, DstBuffer should NOT be freed, because the buffer
>> holds the MM drivers, which will be used in the driver dispatch
>> process later.
>
>Ouch, good point! MmAddToDriverList() only links the driver image into a list
>("mDiscoveredList").
>
>Okay, but then we can still improve the code:
>
>* if ExtractGuidedSectionDecode() reports that it did not use DstBuffer
>  (i.e., it outputs a pointer pointing back into the input blob), then
>  there is no reason to preserve the original allocation. Especially
>  because the allocation is in MM RAM, which is a scarce resources.
>

Will address this in new patch version.

>
>* this is more like a question than a suggestion. Do you know if the
>  drivers linked into "mDiscoveredList" execute "in place" (from the
>  DstBuffer allocation(s)), or if they are never again needed when after
>  the Standalone MM dispatches has actually loaded and launched them?
>  Because in the latter case, it would be nice to release the original
>  DstBuffer allocations; otherwise they just waste MM RAM. (Either way,
>  I agree this is probably out of scope for now.)
>

The driver is not executed 'in place'. MmLoadImage() will allocate new Pages to load the image, but the source is from DstBuffer.

>
>* Consider the following comment, and global variable definition, in
>  "StandaloneMmPkg/Core/Dispatcher.c":
>
>> //
>> // The Driver List contains one copy of every driver that has been discovered.
>> // Items are never removed from the driver list. List of
>> EFI_MM_DRIVER_ENTRY // LIST_ENTRY  mDiscoveredList =
>> INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);
>
>So, I don't understand this. The comment says *one copy* (emphasis mine).
>
>If the comment is right, then we can release DstBuffer immediately after
>MmAddToDriverList().
>
>If the comment is wrong, and MmAddToDriverList() indeed only *links* the
>images into a list (which certainly seems to be the case), then the comment is
>wrong, and should be fixed. It's fine to say that items are never removed from
>the driver list, but
>
>  one copy of
>
>should be replaced with
>
>  one non-owner reference to
>

The comment is wrong. It's NOT *one copy*; it just links to the images in "DstBuffer "

>
>
>Thanks!
>Laszlo
>
>>
>>> (3) And finally, a logic bug (or at least questionable behavior):
>>>
>>> The loop at the *top* of the function scans the firmware volume for
>>> embedded firmware volumes (recursing into them if any are found),
>>> while the loop at the *bottom* of the function scans the *same*
>>> firmware volume for MM driver binaries (adding them to the "MM driver
>>> list"), starting anew from the beginning of the firmware volume.
>>>
>>> Now, there are many exit points in the function-top loop. Those can
>>> be classified in two groups: "break", and "return/goto". The former
>>> class makes sense. The latter class does not seem to make sense to me.
>>>
>>> Consider: just because we fail to scan the firmware volume for
>>> embedded firmware volumes, for any reason really, should we really
>>> abandon scanning the same firmware volume for MM driver binaries?
>>> What I don't understand here in particular is the *inconsistency*
>>> between the exit points, in the function-top loop:
>>>
>>> - if we realize there are no (more) embedded FVs, we break out; good
>>>
>>> - if we realize the next embedded FV is not "GUID defined", we break
>>> out; good (well, questionable -- perhaps we should continue scanning?
>>> the next embedded FV could be GUID defined after all!)
>>>
>>> - if ExtractGuidedSectionGetInfo() fails, we break out again; good
>>> (or, well, we could continue the scanning, but anyway)
>>
>> Will prepare a new patch version to address this: change break to
>> continue
>>
>>>
>>> - if the *decoding* fails, including the allocations, or we fail to
>>> find a proper FV image section, or the recursive
>>> MmCoreFfsFindMmDriver() call fails, then we
>>> *abandon* the MM driver images in the *current* firmware image. That
>>> is what does not make any sense to me, compared to the above-noted
>>> exit points. Just because we couldn't extract a compressed, embedded
>>> FV image, why ignore the MM drivers in *this* image?
>>>
>>
>> Will prepare a new patch version to address this: move the MM drivers
>> detect logic to the front of the while-loop, which mean first check
>> the MM drivers, then check the embedded FVs
>>
>>> Sorry for creating more and more work for you, but I'm starting to
>>> think that the whole loop should be rewritten. :/
>>>
>>> Well, even if we don't change this scanning logic, at least properly
>>> releasing DstBuffer would be nice (i.e., addressing points (1) and (2)).
>>>
>>> Thanks for bearing with me
>>> Laszlo
>>



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

[edk2-devel] [PATCH v3 3/4] StandaloneMmPkg/Core: Fix issue that section address 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.

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 | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index 9d0ce66ef839..fa335d62c252 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -116,23 +116,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;
     }
-- 
2.29.2.windows.2



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

Re: [edk2-devel] [PATCH v3 3/4] StandaloneMmPkg/Core: Fix issue that section address might be wrong

[Laszlo Ersek]

On 10/30/23 08:49, Wei6 Xu 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.
> 
> 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 | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index 9d0ce66ef839..fa335d62c252 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -116,23 +116,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;
>      }

(1) Can you remove the SectionData and SectionDataSize variables as
well? I think they are unused at this point.

With that:

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

Ah wait, you're going to use those variables in the next patch, again.
OK then. Just take my R-b for this patch.


(2) Now that I'm looking at the code in more depth, I don't even
understand what the original intent of the FfsFindSectionData() call was!

The output values SectionData and SectionDataSize were not used for
anything!

So it seems like FfsFindSectionData() was called just to make sure an
EFI_SECTION_GUID_DEFINED section *existed*. And then we'd treat the very
*first* section after the file header -- not too robustly identified, at
that -- as a GUIDed section, for extracting its info.

So this patch actually fixes two warts: one, the file header size is now
considered more generally, two, we don't just assume that the very first
section is the GUID-defined section, but look it up. Phew.

Laszlo



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

[edk2-devel] [PATCH v3 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index fa335d62c252..783dbaf9b048 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -116,6 +116,25 @@ 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);
+      }
+    }
+
+    //
+    // 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 (#110300): https://edk2.groups.io/g/devel/message/110300
Mute This Topic: https://groups.io/mt/102270549/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 10/30/23 08:49, Wei6 Xu 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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index fa335d62c252..783dbaf9b048 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -116,6 +116,25 @@ 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);
> +      }
> +    }
> +
> +    //
> +    // Check compressed firmware volumes
> +    //
>      Status = FfsFindSection (
>                 EFI_SECTION_GUID_DEFINED,
>                 FileHeader,

(1) In case we find an EFI_SECTION_FIRMWARE_VOLUME_IMAGE, do we still want to look for an EFI_SECTION_GUID_DEFINED?

I think that's quite unlikely. If you agree, can you add a "continue" right after the new MmCoreFfsFindMmDriver() call?

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

(2) We have a separate "InnerFvHeader" assignment, near the bottom of this (first) loop in the function:

     Status = FindFfsSectionInSections (
                DstBuffer,
                DstBufferSize,
                EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
                &Section
                );
     if (EFI_ERROR (Status)) {
       goto FreeDstBuffer;
     }
 
     InnerFvHeader = (VOID *)(Section + 1);
     Status        = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);

That doesn't look right to me; what if Section in fact points to an EFI_COMMON_SECTION_HEADER2?

FfsFindSectionData() handles this internally; it checks IS_SECTION2(), and then adjusts SectionData accordingly:

      if (IS_SECTION2 (Section)) {
        *SectionData     = (VOID *)((EFI_COMMON_SECTION_HEADER2 *)Section + 1);
        ...
      } else {
        *SectionData     = (VOID *)(Section + 1);
        ...
      }

I think we need to set InnerFvHeader similarly, here. Can you please append a separate patch for fixing that?

Thanks!
Laszlo



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