1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v4 0/4] StandaloneMmCore finds drivers in uncompressed inner fv.
  4. View patch

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

Xu, Wei6 posted 4 patches 2 years, 3 months ago
[edk2-devel] [PATCH v4 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue
Posted by Xu, Wei6 2 years, 3 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue
Posted by Laszlo Ersek 2 years, 3 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.