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;
     }
 
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110298): https://edk2.groups.io/g/devel/message/110298
Mute This Topic: https://groups.io/mt/102270547/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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).

(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.

(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)

- 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?

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 (#110314): https://edk2.groups.io/g/devel/message/110314
Mute This Topic: https://groups.io/mt/102270547/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Delete one my wrong comments.

-----Original Message-----
From: Xu, Wei6 
Sent: Tuesday, October 31, 2023 2:40 PM
To: Laszlo Ersek <lersek@redhat.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

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 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 <lersek@redhat.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
>
> 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.

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Sorry for the late response. Thanks a lot for the review.
Comment below.

BR,
Wei

>-----Original Message-----
>From: Laszlo Ersek <lersek@redhat.com>
>Sent: Tuesday, October 31, 2023 7:43 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
>
>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 <lersek@redhat.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
>>
>> 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.
>
>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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-