* [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
* [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 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
* 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