[PATCH v2 0/2] *fix ineffective guard page issue

[PATCH v2 0/2] *fix ineffective guard page issue

[Jian J Wang]

>v2: re-generate the patch per Leif's comments

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1295

Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>

Jian J Wang (2):
  MdeModulePkg/Core: fill logic hole in
    MemoryProtectionCpuArchProtocolNotify
  MdeModulePkg/Core: fix ineffective guard page issue

 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 44 +++++++++++++--------------
 1 file changed, 21 insertions(+), 23 deletions(-)

-- 
2.16.2.windows.1

[PATCH v2 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify

[Jian J Wang]

> v2: re-generate this patch per Leif's comments. No logic changes.

At the end of of MemoryProtectionCpuArchProtocolNotify there's cleanup
code to free resource. But at line 978, 994, 1005 the function returns
directly. This patch use "goto" to replace "return" to make sure the
resource is freed before exit.

1029:  CoreCloseEvent (Event);
1030:  return;

Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 6298b67db1..98fdc2c618 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -975,7 +975,7 @@ MemoryProtectionCpuArchProtocolNotify (
   DEBUG ((DEBUG_INFO, "MemoryProtectionCpuArchProtocolNotify:\n"));
   Status = CoreLocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu);
   if (EFI_ERROR (Status)) {
-    return;
+    goto Done;
   }
 
   //
@@ -991,7 +991,7 @@ MemoryProtectionCpuArchProtocolNotify (
   HeapGuardCpuArchProtocolNotify ();
 
   if (mImageProtectionPolicy == 0) {
-    return;
+    goto Done;
   }
 
   Status = gBS->LocateHandleBuffer (
@@ -1002,7 +1002,7 @@ MemoryProtectionCpuArchProtocolNotify (
                   &HandleBuffer
                   );
   if (EFI_ERROR (Status) && (NoHandles == 0)) {
-    return ;
+    goto Done;
   }
 
   for (Index = 0; Index < NoHandles; Index++) {
@@ -1026,8 +1026,8 @@ MemoryProtectionCpuArchProtocolNotify (
     ProtectUefiImage (LoadedImage, LoadedImageDevicePath);
   }
 
+Done:
   CoreCloseEvent (Event);
-  return;
 }
 
 /**
-- 
2.16.2.windows.1

[PATCH v2 2/2] MdeModulePkg/Core: fix ineffective guard page issue

[Jian J Wang]

> v2: re-generate this patch per Leif's comments. No logic changes.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1295

This issue originates from following patch which allows to enable
paging if PcdImageProtectionPolicy and PcdDxeNxMemoryProtectionPolicy
(in addition to PcdSetNxForStack) are set to enable related features.

  5267926134d17e86672b84fd57b438f05ffa68e1

Due to above change, PcdImageProtectionPolicy will be set to 0 by
default in many platforms, which, in turn, cause following code in
MdeModulePkg\Core\Dxe\Misc\MemoryProtection.c fail the creation of
notify event of CpuArchProtocol.

1138:  if (mImageProtectionPolicy != 0 ||
           PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
1139:  Status = CoreCreateEvent (
...
1142:             MemoryProtectionCpuArchProtocolNotify,
...
1145:             );

Then following call flow won't be done and Guard pages will not be
set as not-present in SetAllGuardPages() eventually.

   MemoryProtectionCpuArchProtocolNotify()
=> HeapGuardCpuArchProtocolNotify()
=> SetAllGuardPages()

The solution is removing the if(...) statement so that the notify
event will always be created and registered. This won't cause
unnecessary code execution because, in the notify event handler,
the related PCDs like

    PcdImageProtectionPolicy and
    PcdDxeNxMemoryProtectionPolicy

will be checked again before doing related jobs.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 36 +++++++++++++--------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 98fdc2c618..30798b05b9 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1135,26 +1135,24 @@ CoreInitializeMemoryProtection (
   ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==
           GetPermissionAttributeForMemoryType (EfiConventionalMemory));
 
-  if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
-    Status = CoreCreateEvent (
-               EVT_NOTIFY_SIGNAL,
-               TPL_CALLBACK,
-               MemoryProtectionCpuArchProtocolNotify,
-               NULL,
-               &Event
-               );
-    ASSERT_EFI_ERROR(Status);
+  Status = CoreCreateEvent (
+             EVT_NOTIFY_SIGNAL,
+             TPL_CALLBACK,
+             MemoryProtectionCpuArchProtocolNotify,
+             NULL,
+             &Event
+             );
+  ASSERT_EFI_ERROR(Status);
 
-    //
-    // Register for protocol notifactions on this event
-    //
-    Status = CoreRegisterProtocolNotify (
-               &gEfiCpuArchProtocolGuid,
-               Event,
-               &Registration
-               );
-    ASSERT_EFI_ERROR(Status);
-  }
+  //
+  // Register for protocol notifactions on this event
+  //
+  Status = CoreRegisterProtocolNotify (
+             &gEfiCpuArchProtocolGuid,
+             Event,
+             &Registration
+             );
+  ASSERT_EFI_ERROR(Status);
 
   //
   // Register a callback to disable NULL pointer detection at EndOfDxe
-- 
2.16.2.windows.1

Re: [PATCH v2 0/2] *fix ineffective guard page issue

[Zeng, Star]

On 2018/11/7 8:53, Jian J Wang wrote:
>> v2: re-generate the patch per Leif's comments
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1295
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>

Reviewed-by: Star Zeng <star.zeng@intel.com>

> 
> Jian J Wang (2):
>    MdeModulePkg/Core: fill logic hole in
>      MemoryProtectionCpuArchProtocolNotify
>    MdeModulePkg/Core: fix ineffective guard page issue
> 
>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 44 +++++++++++++--------------
>   1 file changed, 21 insertions(+), 23 deletions(-)
>