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

[Laszlo Ersek <lersek@redhat.com>]

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|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.<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