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

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

Leif,

Thanks for catching that. I'll re-generate the patch files.

Regards,
Jian

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, November 06, 2018 8:10 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page
> issue
> 
> On Sat, Nov 03, 2018 at 02:42:21PM +0800, Jian J Wang wrote:
> > 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 handler be 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 to do its job.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 30e5c5153c..30798b05b9 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > @@ -1135,7 +1135,6 @@ CoreInitializeMemoryProtection (
> >    ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==
> >            GetPermissionAttributeForMemoryType (EfiConventionalMemory));
> >
> > -  if (mImageProtectionPolicy != 0 || PcdGet64
> (PcdDxeNxMemoryProtectionPolicy) != 0) {
> >    Status = CoreCreateEvent (
> >               EVT_NOTIFY_SIGNAL,
> >               TPL_CALLBACK,
> > @@ -1154,7 +1153,6 @@ CoreInitializeMemoryProtection (
> >               &Registration
> >               );
> >    ASSERT_EFI_ERROR(Status);
> > -  }
> 
> And here we see why.
> The indentation changes need to be part of this patch, not 1/2.
> 
> /
>     Leif
> 
> >
> >    //
> >    // Register a callback to disable NULL pointer detection at EndOfDxe
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel