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 <ler...@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Sami Mujawar <sami.muja...@arm.com>
> Cc: Ray Ni <ray...@intel.com>
> Signed-off-by: Wei6 Xu <wei6...@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
