* [PATCH 0/2] fix ineffective guard page issue
@ 2018-11-03 6:42 Jian J Wang
2018-11-03 6:42 ` [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang
2018-11-03 6:42 ` [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang
0 siblings, 2 replies; 6+ messages in thread
From: Jian J Wang @ 2018-11-03 6:42 UTC (permalink / raw)
To: edk2-devel
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1295
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify 2018-11-03 6:42 [PATCH 0/2] fix ineffective guard page issue Jian J Wang @ 2018-11-03 6:42 ` Jian J Wang 2018-11-06 12:07 ` Leif Lindholm 2018-11-03 6:42 ` [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang 1 sibling, 1 reply; 6+ messages in thread From: Jian J Wang @ 2018-11-03 6:42 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni 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> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang <jian.j.wang@intel.com> --- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 42 +++++++++++++-------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index 6298b67db1..30e5c5153c 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; } /** @@ -1136,24 +1136,24 @@ CoreInitializeMemoryProtection ( 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); } // -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify 2018-11-03 6:42 ` [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang @ 2018-11-06 12:07 ` Leif Lindholm 0 siblings, 0 replies; 6+ messages in thread From: Leif Lindholm @ 2018-11-06 12:07 UTC (permalink / raw) To: Jian J Wang; +Cc: edk2-devel, Ruiyu Ni, Jiewen Yao, Star Zeng On Sat, Nov 03, 2018 at 02:42:20PM +0800, Jian J Wang wrote: > 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> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > --- > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 42 +++++++++++++-------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > index 6298b67db1..30e5c5153c 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; > } > > /** I have no comments on the bits above, but everything below here looks like it's only ruining the indentation without any functional changes at all. / Leif > @@ -1136,24 +1136,24 @@ CoreInitializeMemoryProtection ( > 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); > } > > // > -- > 2.16.2.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue 2018-11-03 6:42 [PATCH 0/2] fix ineffective guard page issue Jian J Wang 2018-11-03 6:42 ` [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang @ 2018-11-03 6:42 ` Jian J Wang 2018-11-06 12:09 ` Leif Lindholm 1 sibling, 1 reply; 6+ messages in thread From: Jian J Wang @ 2018-11-03 6:42 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni 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); - } // // Register a callback to disable NULL pointer detection at EndOfDxe -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue 2018-11-03 6:42 ` [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang @ 2018-11-06 12:09 ` Leif Lindholm 2018-11-07 0:38 ` Wang, Jian J 0 siblings, 1 reply; 6+ messages in thread From: Leif Lindholm @ 2018-11-06 12:09 UTC (permalink / raw) To: Jian J Wang; +Cc: edk2-devel, Ruiyu Ni, Jiewen Yao, Star Zeng 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue 2018-11-06 12:09 ` Leif Lindholm @ 2018-11-07 0:38 ` Wang, Jian J 0 siblings, 0 replies; 6+ messages in thread From: Wang, Jian J @ 2018-11-07 0:38 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ni, Ruiyu, Yao, Jiewen, Zeng, Star 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-07 0:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-03 6:42 [PATCH 0/2] fix ineffective guard page issue Jian J Wang 2018-11-03 6:42 ` [PATCH 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang 2018-11-06 12:07 ` Leif Lindholm 2018-11-03 6:42 ` [PATCH 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang 2018-11-06 12:09 ` Leif Lindholm 2018-11-07 0:38 ` Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox