From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3F6442117923B for ; Tue, 23 Oct 2018 09:09:23 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9A304307D90F; Tue, 23 Oct 2018 16:09:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-214.rdu2.redhat.com [10.10.120.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7152417F7C; Tue, 23 Oct 2018 16:09:20 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Michael D Kinney , Ruiyu Ni , Jiewen Yao , Star Zeng References: <20181023145331.5768-1-jian.j.wang@intel.com> <20181023145331.5768-2-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <1335092f-2353-e8ae-480e-fda0e610148a@redhat.com> Date: Tue, 23 Oct 2018 18:09:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181023145331.5768-2-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Tue, 23 Oct 2018 16:09:22 +0000 (UTC) Subject: Re: [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Oct 2018 16:09:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/23/18 16:53, Jian J Wang wrote: >> v2 changes: >> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of >> PcdHeapGuardPropertyMask instead. Update related descriptions in both >> dec and uni files. > > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue > which is illegal access to memory which has been freed. The principle > behind is similar to heap guard feature, that is we'll turn all pool > memory allocation to page allocation and mark them to be not-present > once they are freed. > > This also implies that, once a page is allocated and freed, it cannot > be re-allocated. This will bring another issue, which is that there's > risk that memory space will be used out. To address it, this patch > series add logic put part (at most 64 pages a time) of freed pages > back into page pool, so that the memory service can still have memory > to allocate, when all memory space have been allocated once. This is > called memory promotion. The promoted pages are always from the eldest > pages freed. > > BIT4 of following PCD is used to enable the freed-memory guard feature. > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > > Please note this feature cannot be enabled with heap guard at the same > time. > > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++ > MdeModulePkg/MdeModulePkg.uni | 6 +++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 6037504fa7..255b92ea67 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -955,6 +955,8 @@ > # free pages for all of them. The page allocation for the type related to > # cleared bits keeps the same as ususal. > # > + # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask. > + # > # Below is bit mask for this PCD: (Order is same as UEFI spec)
> # EfiReservedMemoryType 0x0000000000000001
> # EfiLoaderCode 0x0000000000000002
> @@ -984,6 +986,8 @@ > # if there's enough free memory for all of them. The pool allocation for the > # type related to cleared bits keeps the same as ususal. > # > + # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask. > + # > # Below is bit mask for this PCD: (Order is same as UEFI spec)
> # EfiReservedMemoryType 0x0000000000000001
> # EfiLoaderCode 0x0000000000000002
> @@ -1007,14 +1011,20 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053 > > ## This mask is to control Heap Guard behavior. > + # > # Note that due to the limit of pool memory implementation and the alignment > # requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee > # that the returned pool is exactly adjacent to head guard page or tail guard > # page. (1) The above changes are not related to the use-after-free feature; they should go into a separate cleanup patch. (Which is very welcome otherwise.) The cleanup patch should be patch #1 in the series. The subject should be, for example: MdeModulePkg: clean up Heap Guard PageType / PoolType PCD documentation (71 characters) > + # > + # Note that UEFI freed-memory guard and pool/page guard cannot be enabled > + # at the same time. > + # > # BIT0 - Enable UEFI page guard.
> # BIT1 - Enable UEFI pool guard.
> # BIT2 - Enable SMM page guard.
> # BIT3 - Enable SMM pool guard.
> + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).
> # BIT6 - Enable non-stop mode.
> # BIT7 - The direction of Guard Page for Pool Guard. > # 0 - The returned pool is near the tail guard page.
(2) This should go into patch #2 in the series. However, the current subject line is useless: MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature Instead, I suggest: MdeModulePkg: introduce UEFI Use After Free feature bit in Heap Guard PCD (73 characters). > diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni > index a6bcb627cf..e72b893509 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -1171,6 +1171,7 @@ > " before and after corresponding type of pages allocated if there's enough\n" > " free pages for all of them. The page allocation for the type related to\n" > " cleared bits keeps the same as ususal.\n\n" > + " This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n" > " Below is bit mask for this PCD: (Order is same as UEFI spec)
\n" > " EfiReservedMemoryType 0x0000000000000001\n" > " EfiLoaderCode 0x0000000000000002\n" > @@ -1198,6 +1199,7 @@ > " before and after corresponding type of pages which the allocated pool occupies,\n" > " if there's enough free memory for all of them. The pool allocation for the\n" > " type related to cleared bits keeps the same as ususal.\n\n" > + " This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.\n\n" > " Below is bit mask for this PCD: (Order is same as UEFI spec)
\n" > " EfiReservedMemoryType 0x0000000000000001\n" > " EfiLoaderCode 0x0000000000000002\n" (3) Same as (1). > @@ -1225,11 +1227,13 @@ > "Note that due to the limit of pool memory implementation and the alignment\n" > "requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee\n" > "that the returned pool is exactly adjacent to head guard page or tail guard\n" > - "page.\n" > + "page.\n\n" > + "Note that UEFI freed-memory guard and pool/page guard cannot be enabled at the same time.\n\n" > " BIT0 - Enable UEFI page guard.
\n" > " BIT1 - Enable UEFI pool guard.
\n" > " BIT2 - Enable SMM page guard.
\n" > " BIT3 - Enable SMM pool guard.
\n" > + " BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).
\n" > " BIT7 - The direction of Guard Page for Pool Guard.\n" > " 0 - The returned pool is near the tail guard page.
\n" > " 1 - The returned pool is near the head guard page.
" > (4) Same as (2). With those changes: Reviewed-by: Laszlo Ersek Thanks Laszlo