[PATCH v3 0/2] fix ineffective guard page issue

[PATCH v3 0/2] fix ineffective guard page issue

[Jian J Wang]

>v3: found and fixed another memory leak

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 | 45 +++++++++++++--------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
2.16.2.windows.1

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

[Jian J Wang]

> v3: fixed one more memory leak in the same function and updated
>     commit message accordingly.

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;

There's another memory leak after calling gBS->LocateHandleBuffer() in
the same function:

  Status = gBS->LocateHandleBuffer (
                  ByProtocol,
                  &gEfiLoadedImageProtocolGuid,
                  NULL,
                  &NoHandles,
                  &HandleBuffer
                  );

HandleBuffer is allocated in above call but never freed. This patch
will also add code to free it.

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 6298b67db1..8a93c5362a 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++) {
@@ -1025,9 +1025,10 @@ MemoryProtectionCpuArchProtocolNotify (
 
     ProtectUefiImage (LoadedImage, LoadedImageDevicePath);
   }
+  FreePool (HandleBuffer);
 
+Done:
   CoreCloseEvent (Event);
-  return;
 }
 
 /**
-- 
2.16.2.windows.1

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

[Jian J Wang]

>v3: no change

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 8a93c5362a..c43e0d08ae 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1136,26 +1136,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 v3 0/2] fix ineffective guard page issue

[Zeng, Star]

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

-----Original Message-----
From: Wang, Jian J 
Sent: Wednesday, November 7, 2018 3:13 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: [PATCH v3 0/2] fix ineffective guard page issue

>v3: found and fixed another memory leak

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 | 45 +++++++++++++--------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
2.16.2.windows.1

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

[Leif Lindholm]

On Wed, Nov 07, 2018 at 03:12:47PM +0800, Jian J Wang wrote:
> > v3: fixed one more memory leak in the same function and updated
> >     commit message accordingly.

No objection to the content, but comments not intended to be committed
should go in the cover letter or below the ---.

A changelog between revisions is best kept in the cover letter,
including all changes in reverse chronological order, like:

Changes since v2:
- ...

Changes since v1:
- ...

I expect whoever pushes the patches will edit the history out of the
commit message, but putting it in the cover-letter means less work for
them.

(I won't give a reviewed-by since I have only been commenting on
style, not functionality.)

/
    Leif

> 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;
> 
> There's another memory leak after calling gBS->LocateHandleBuffer() in
> the same function:
> 
>   Status = gBS->LocateHandleBuffer (
>                   ByProtocol,
>                   &gEfiLoadedImageProtocolGuid,
>                   NULL,
>                   &NoHandles,
>                   &HandleBuffer
>                   );
> 
> HandleBuffer is allocated in above call but never freed. This patch
> will also add code to free it.
> 
> 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 | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 6298b67db1..8a93c5362a 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++) {
> @@ -1025,9 +1025,10 @@ MemoryProtectionCpuArchProtocolNotify (
>  
>      ProtectUefiImage (LoadedImage, LoadedImageDevicePath);
>    }
> +  FreePool (HandleBuffer);
>  
> +Done:
>    CoreCloseEvent (Event);
> -  return;
>  }
>  
>  /**
> -- 
> 2.16.2.windows.1
> 

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

[Wang, Jian J]

Leif,

Thanks for the comments. I'll push the patch after removing the change log
from the commit log.

Regards,
Jian


> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, November 07, 2018 3:53 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in
> MemoryProtectionCpuArchProtocolNotify
> 
> 
> On Wed, Nov 07, 2018 at 03:12:47PM +0800, Jian J Wang wrote:
> > > v3: fixed one more memory leak in the same function and updated
> > >     commit message accordingly.
> 
> No objection to the content, but comments not intended to be committed
> should go in the cover letter or below the ---.
> 
> A changelog between revisions is best kept in the cover letter,
> including all changes in reverse chronological order, like:
> 
> Changes since v2:
> - ...
> 
> Changes since v1:
> - ...
> 
> I expect whoever pushes the patches will edit the history out of the
> commit message, but putting it in the cover-letter means less work for
> them.
> 
> (I won't give a reviewed-by since I have only been commenting on
> style, not functionality.)
> 
> /
>     Leif
> 
> > 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;
> >
> > There's another memory leak after calling gBS->LocateHandleBuffer() in
> > the same function:
> >
> >   Status = gBS->LocateHandleBuffer (
> >                   ByProtocol,
> >                   &gEfiLoadedImageProtocolGuid,
> >                   NULL,
> >                   &NoHandles,
> >                   &HandleBuffer
> >                   );
> >
> > HandleBuffer is allocated in above call but never freed. This patch
> > will also add code to free it.
> >
> > 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 | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 6298b67db1..8a93c5362a 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++) {
> > @@ -1025,9 +1025,10 @@ MemoryProtectionCpuArchProtocolNotify (
> >
> >      ProtectUefiImage (LoadedImage, LoadedImageDevicePath);
> >    }
> > +  FreePool (HandleBuffer);
> >
> > +Done:
> >    CoreCloseEvent (Event);
> > -  return;
> >  }
> >
> >  /**
> > --
> > 2.16.2.windows.1
> >