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

["Xu, Wei6" <wei6.xu@intel.com>]

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