From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C41DCAC10FF for ; Tue, 31 Oct 2023 11:43:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=X2T81UL1aJ7Y0AUJyfR/zV+pKZyp8XW7B4PWqvbCJtQ=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698752593; v=1; b=o5wJXUeGOXGVwlSF1k2u3xHSPpZ2rzPf6GqurkHj9gQGBNBvKa97DsITG5fp76yq0OGIygzg uZ+EcHkkNWeoZHDRFtw+KzyZSuMHvR7BkhiRIn4QcLjDRUMYMfboyK4ewahMD7YETHELemTFkxa wTd7Ke5kQoQqkweVwf4ldoqo= X-Received: by 127.0.0.2 with SMTP id yuWeYY7687511xbNNI2Vc2nk; Tue, 31 Oct 2023 04:43:13 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.183889.1698752592952456854 for ; Tue, 31 Oct 2023 04:43:13 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-400-Ml8-b6OXPqmTONx-19W6Tw-1; Tue, 31 Oct 2023 07:43:08 -0400 X-MC-Unique: Ml8-b6OXPqmTONx-19W6Tw-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 660A4811E7D; Tue, 31 Oct 2023 11:43:08 +0000 (UTC) X-Received: from [10.39.195.34] (unknown [10.39.195.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 596012026D4C; Tue, 31 Oct 2023 11:43:06 +0000 (UTC) Message-ID: <4311610f-85f3-9ebb-51be-1e6caa23a9fa@redhat.com> Date: Tue, 31 Oct 2023 12:43:05 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue To: "Xu, Wei6" , "devel@edk2.groups.io" Cc: Ard Biesheuvel , Sami Mujawar , "Ni, Ray" References: <612df6233746ce55990359472221a193c398749b.1698651605.git.wei6.xu@intel.com> <801c76dd-a7b6-8002-45d9-e5e002c4f81c@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 5PgO4EhMIzRewbu0rllRQadSx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=o5wJXUeG; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 ; devel@edk2.groups.io > Cc: Ard Biesheuvel ; Sami Mujawar ; Ni, Ray > Subject: RE: [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory le= ak 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 >> Sent: Monday, October 30, 2023 8:25 PM >> To: Xu, Wei6 ; devel@edk2.groups.io >> Cc: Ard Biesheuvel ; Sami Mujawar >> ; Ni, Ray >> 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 >>> Cc: Ard Biesheuvel >>> Cc: Sami Mujawar >>> Cc: Ray Ni >>> Signed-off-by: Wei6 Xu >>> --- >>> 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 =3D (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES >> (DstBufferSize)); >>> if (DstBuffer =3D=3D 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 >> >> 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 discov= ered. > // Items are never removed from the driver list. List of EFI_MM_DRIVER_EN= TRY > // > LIST_ENTRY mDiscoveredList =3D INITIALIZE_LIST_HEAD_VARIABLE (mDiscovere= dList); 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 continu= e > >> >> - 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 det= ect logic to the front of the while-loop, which mean first check the MM dri= vers, 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 > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-