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

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

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