* [PATCH v3 0/2] fix ineffective guard page issue
@ 2018-11-07 7:12 Jian J Wang
2018-11-07 7:12 ` [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jian J Wang @ 2018-11-07 7:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Leif Lindholm
>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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify 2018-11-07 7:12 [PATCH v3 0/2] fix ineffective guard page issue Jian J Wang @ 2018-11-07 7:12 ` Jian J Wang 2018-11-07 7:52 ` Leif Lindholm 2018-11-07 7:12 ` [PATCH v3 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang 2018-11-07 7:22 ` [PATCH v3 0/2] " Zeng, Star 2 siblings, 1 reply; 6+ messages in thread From: Jian J Wang @ 2018-11-07 7:12 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Leif Lindholm > 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify 2018-11-07 7:12 ` [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang @ 2018-11-07 7:52 ` Leif Lindholm 2018-11-07 8:33 ` Wang, Jian J 0 siblings, 1 reply; 6+ messages in thread From: Leif Lindholm @ 2018-11-07 7:52 UTC (permalink / raw) To: Jian J Wang; +Cc: edk2-devel, Star Zeng, Jiewen Yao, Ruiyu Ni 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify 2018-11-07 7:52 ` Leif Lindholm @ 2018-11-07 8:33 ` Wang, Jian J 0 siblings, 0 replies; 6+ messages in thread From: Wang, Jian J @ 2018-11-07 8:33 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Zeng, Star, Yao, Jiewen, Ni, Ruiyu 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] MdeModulePkg/Core: fix ineffective guard page issue 2018-11-07 7:12 [PATCH v3 0/2] fix ineffective guard page issue Jian J Wang 2018-11-07 7:12 ` [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang @ 2018-11-07 7:12 ` Jian J Wang 2018-11-07 7:22 ` [PATCH v3 0/2] " Zeng, Star 2 siblings, 0 replies; 6+ messages in thread From: Jian J Wang @ 2018-11-07 7:12 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Leif Lindholm >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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/2] fix ineffective guard page issue 2018-11-07 7:12 [PATCH v3 0/2] fix ineffective guard page issue Jian J Wang 2018-11-07 7:12 ` [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang 2018-11-07 7:12 ` [PATCH v3 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang @ 2018-11-07 7:22 ` Zeng, Star 2 siblings, 0 replies; 6+ messages in thread From: Zeng, Star @ 2018-11-07 7:22 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org Cc: Yao, Jiewen, Ni, Ruiyu, Leif Lindholm, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-07 8:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-07 7:12 [PATCH v3 0/2] fix ineffective guard page issue Jian J Wang 2018-11-07 7:12 ` [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify Jian J Wang 2018-11-07 7:52 ` Leif Lindholm 2018-11-07 8:33 ` Wang, Jian J 2018-11-07 7:12 ` [PATCH v3 2/2] MdeModulePkg/Core: fix ineffective guard page issue Jian J Wang 2018-11-07 7:22 ` [PATCH v3 0/2] " Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox