comment below
On 10/31/23 09:37, Xu, Wei6 wrote:
> 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.
Ouch, good point! MmAddToDriverList() only links the driver image into a
list ("mDiscoveredList").
Okay, but then we can still improve the code:
* if ExtractGuidedSectionDecode() reports that it did not use DstBuffer
(i.e., it outputs a pointer pointing back into the input blob), then
there is no reason to preserve the original allocation. Especially
because the allocation is in MM RAM, which is a scarce resources.
* this is more like a question than a suggestion. Do you know if the
drivers linked into "mDiscoveredList" execute "in place" (from the
DstBuffer allocation(s)), or if they are never again needed when after
the Standalone MM dispatches has actually loaded and launched them?
Because in the latter case, it would be nice to release the original
DstBuffer allocations; otherwise they just waste MM RAM. (Either way,
I agree this is probably out of scope for now.)
* Consider the following comment, and global variable definition, in
"StandaloneMmPkg/Core/Dispatcher.c":
> //
> // The Driver List contains one copy of every driver that has been discovered.
> // Items are never removed from the driver list. List of EFI_MM_DRIVER_ENTRY
> //
> LIST_ENTRY mDiscoveredList = INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);
So, I don't understand this. The comment says *one copy* (emphasis
mine).
If the comment is right, then we can release DstBuffer immediately after
MmAddToDriverList().
If the comment is wrong, and MmAddToDriverList() indeed only *links* the
images into a list (which certainly seems to be the case), then the
comment is wrong, and should be fixed. It's fine to say that items are
never removed from the driver list, but
one copy of
should be replaced with
one non-owner reference to
Thanks!
Laszlo
>
>> (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 (#110400): https://edk2.groups.io/g/devel/message/110400
Mute This Topic: https://groups.io/mt/102270547/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
