Delete one my wrong comments.
-----Original Message-----
From: Xu, Wei6
Sent: Tuesday, October 31, 2023 2:40 PM
To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Sami Mujawar
<sami.muja...@arm.com>; Ni, Ray <ray...@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 <ler...@redhat.com>
>Sent: Monday, October 30, 2023 8:25 PM
>To: Xu, Wei6 <wei6...@intel.com>; devel@edk2.groups.io
>Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Sami Mujawar
><sami.muja...@arm.com>; Ni, Ray <ray...@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 <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 | 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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
