Re: [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature

["Wang, Jian J" <jian.j.wang@intel.com>]

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:lersek@redhat.com]
> Sent: Wednesday, October 24, 2018 12:09 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> 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 <star.zeng@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  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 <lersek@redhat.com>
> 
> Thanks
> Laszlo