Laszlo, Thank you very much for the comments. I went through all of them in other patch emails and I think I have no objection. So to save all of us time I'm not going to respond them separately. I'll send out v3 patch soon. Thanks again.
Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Wednesday, October 24, 2018 12:09 AM > To: Wang, Jian J <[email protected]>; [email protected] > Cc: Kinney, Michael D <[email protected]>; Ni, Ruiyu > <[email protected]>; Yao, Jiewen <[email protected]>; Zeng, Star > <[email protected]> > Subject: Re: [edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update > PCD description for new feature > > 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 <[email protected]> > > Cc: Michael D Kinney <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <[email protected]> > > --- > > 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)<BR> > > # EfiReservedMemoryType 0x0000000000000001<BR> > > # EfiLoaderCode 0x0000000000000002<BR> > > @@ -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)<BR> > > # EfiReservedMemoryType 0x0000000000000001<BR> > > # EfiLoaderCode 0x0000000000000002<BR> > > @@ -1007,14 +1011,20 @@ > > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30 > 001053 > > > > ## 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.<BR> > > # BIT1 - Enable UEFI pool guard.<BR> > > # BIT2 - Enable SMM page guard.<BR> > > # BIT3 - Enable SMM pool guard.<BR> > > + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory > detection).<BR> > > # BIT6 - Enable non-stop mode.<BR> > > # BIT7 - The direction of Guard Page for Pool Guard. > > # 0 - The returned pool is near the tail guard page.<BR> > > (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)<BR>\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)<BR>\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.<BR>\n" > > > > " BIT1 - Enable UEFI pool > guard.<BR>\n" > > > > " BIT2 - Enable SMM page > guard.<BR>\n" > > > > " BIT3 - Enable SMM pool > guard.<BR>\n" > > + > > " BIT4 - Enable UEFI > freed-memory guard (Use-After-Free memory detection).<BR>\n" > > > > " BIT7 - The direction of > Guard Page for Pool Guard.\n" > > > > " 0 - The returned pool > is near the tail guard page.<BR>\n" > > > > " 1 - The returned pool > is near the head guard page.<BR>" > > > > (4) Same as (2). > > With those changes: > > Reviewed-by: Laszlo Ersek <[email protected]> > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
