Sorry for the late response. Thanks a lot for the review.
Comment below.
BR,
Wei
>-----Original Message-----
>From: Laszlo Ersek <ler...@redhat.com>
>Sent: Tuesday, October 31, 2023 7:43 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
>
>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.
>
Will address this in new patch version.
>
>* 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.)
>
The driver is not executed 'in place'. MmLoadImage() will allocate new Pages to
load the image, but the source is from DstBuffer.
>
>* 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
>
The comment is wrong. It's NOT *one copy*; it just links to the images in
"DstBuffer "
>
>
>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 (#110741): https://edk2.groups.io/g/devel/message/110741
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]
-=-=-=-=-=-=-=-=-=-=-=-
