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 5F6FCD804CC for ; Mon, 30 Oct 2023 12:24:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WeXLaVFn0v8X3vYGS+SQQCaZD5FRucJmVuM114c5GEE=; 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=1698668683; v=1; b=jGnaDUBlSXHgzJJJDGgdliHg4wFb1ZFWX/MGOo9rxodYzA37lvD+LH49H1uOZWx/ETVdLgFU FrYBhm7n7oXWmomkY5kTpRxzlTfZ0+Mj7zHPchSgC+B4s4DaWMiS0/LJl5PZxSQiRcNcj4mVnOI md0C2ZwY0h2j01HhFyMcNo/w= X-Received: by 127.0.0.2 with SMTP id mfJmYY7687511xTr91h0VacA; Mon, 30 Oct 2023 05:24:43 -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.web11.147507.1698668683297445915 for ; Mon, 30 Oct 2023 05:24:43 -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-642-spuifF5uODyoVpxGxrYIlg-1; Mon, 30 Oct 2023 08:24:39 -0400 X-MC-Unique: spuifF5uODyoVpxGxrYIlg-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (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 BB6C48007B3; Mon, 30 Oct 2023 12:24:38 +0000 (UTC) X-Received: from [10.39.194.199] (unknown [10.39.194.199]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B43D6492BFC; Mon, 30 Oct 2023 12:24:37 +0000 (UTC) Message-ID: <801c76dd-a7b6-8002-45d9-e5e002c4f81c@redhat.com> Date: Mon, 30 Oct 2023 13:24:36 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/4] StandaloneMmPkg/Core: Fix potential memory leak issue To: Wei6 Xu , devel@edk2.groups.io Cc: Ard Biesheuvel , Sami Mujawar , Ray Ni References: <612df6233746ce55990359472221a193c398749b.1698651605.git.wei6.xu@intel.com> From: "Laszlo Ersek" In-Reply-To: <612df6233746ce55990359472221a193c398749b.1698651605.git.wei6.xu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 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: cg6TegqsIDBU3wqwyIYn9V45x7686176AA= 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=jGnaDUBl; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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. >=20 > 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(+) >=20 > 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 (DstBu= fferSize)); > if (DstBuffer =3D=3D NULL) { > + FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize)); > return EFI_OUT_OF_RESOURCES; > } > =20 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). (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 -=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 (#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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-