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

["Laszlo Ersek" <lersek@redhat.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-