* [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory @ 2018-03-15 6:22 Jian J Wang 2018-03-15 6:55 ` Ni, Ruiyu 0 siblings, 1 reply; 4+ messages in thread From: Jian J Wang @ 2018-03-15 6:22 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao, Ruiyu Ni SMM core will add a HEADER before each allocated pool memory and clean up this header once it's freed. If a block of allocated pool is marked as read-only after allocation (EfiRuntimeServicesCode type of pool in SMM will always be marked as read-only), #PF exception will be triggered during memory pool freeing. Normally EfiRuntimeServicesCode type of pool should not be freed in the real world. But some test suites will actually do memory free for all types of memory for the purpose of functionality and conformance test. So this issue should be fixed anyway. Cc: Star Zeng <star.zeng@intel.com> Cc: Eric Dong <eric.dong@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/PiSmmCore/HeapGuard.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c index f9657f9baa..d5556eb79c 100644 --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c @@ -941,6 +941,7 @@ AdjustMemoryF ( EFI_PHYSICAL_ADDRESS MemoryToTest; UINTN PagesToFree; UINT64 GuardBitmap; + UINT64 Attributes; if (Memory == NULL || NumberOfPages == NULL || *NumberOfPages == 0) { return; @@ -949,6 +950,27 @@ AdjustMemoryF ( Start = *Memory; PagesToFree = *NumberOfPages; + // + // In case the memory to free is marked as read-only (e.g. EfiRuntimeServicesCode). + // + if (mSmmMemoryAttribute != NULL) { + Attributes = 0; + mSmmMemoryAttribute->GetMemoryAttributes ( + mSmmMemoryAttribute, + Start, + EFI_PAGES_TO_SIZE (PagesToFree), + &Attributes + ); + if ((Attributes & EFI_MEMORY_RO) != 0) { + mSmmMemoryAttribute->ClearMemoryAttributes ( + mSmmMemoryAttribute, + Start, + EFI_PAGES_TO_SIZE (PagesToFree), + EFI_MEMORY_RO + ); + } + } + // // Head Guard must be one page before, if any. // -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory 2018-03-15 6:22 [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory Jian J Wang @ 2018-03-15 6:55 ` Ni, Ruiyu 2018-03-15 7:07 ` Wang, Jian J 0 siblings, 1 reply; 4+ messages in thread From: Ni, Ruiyu @ 2018-03-15 6:55 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Eric Dong, Star Zeng On 3/15/2018 2:22 PM, Jian J Wang wrote: > SMM core will add a HEADER before each allocated pool memory and clean > up this header once it's freed. If a block of allocated pool is marked > as read-only after allocation (EfiRuntimeServicesCode type of pool in > SMM will always be marked as read-only), #PF exception will be triggered > during memory pool freeing. > > Normally EfiRuntimeServicesCode type of pool should not be freed in the > real world. But some test suites will actually do memory free for all > types of memory for the purpose of functionality and conformance test. > So this issue should be fixed anyway. Does DxeCore have such problem? > > Cc: Star Zeng <star.zeng@intel.com> > Cc: Eric Dong <eric.dong@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/PiSmmCore/HeapGuard.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > index f9657f9baa..d5556eb79c 100644 > --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > @@ -941,6 +941,7 @@ AdjustMemoryF ( > EFI_PHYSICAL_ADDRESS MemoryToTest; > UINTN PagesToFree; > UINT64 GuardBitmap; > + UINT64 Attributes; > > if (Memory == NULL || NumberOfPages == NULL || *NumberOfPages == 0) { > return; > @@ -949,6 +950,27 @@ AdjustMemoryF ( > Start = *Memory; > PagesToFree = *NumberOfPages; > > + // > + // In case the memory to free is marked as read-only (e.g. EfiRuntimeServicesCode). > + // > + if (mSmmMemoryAttribute != NULL) { > + Attributes = 0; > + mSmmMemoryAttribute->GetMemoryAttributes ( > + mSmmMemoryAttribute, > + Start, > + EFI_PAGES_TO_SIZE (PagesToFree), > + &Attributes > + ); > + if ((Attributes & EFI_MEMORY_RO) != 0) { > + mSmmMemoryAttribute->ClearMemoryAttributes ( > + mSmmMemoryAttribute, > + Start, > + EFI_PAGES_TO_SIZE (PagesToFree), > + EFI_MEMORY_RO > + ); > + } > + } > + > // > // Head Guard must be one page before, if any. > // > -- Thanks, Ray ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory 2018-03-15 6:55 ` Ni, Ruiyu @ 2018-03-15 7:07 ` Wang, Jian J 2018-03-16 6:44 ` Ni, Ruiyu 0 siblings, 1 reply; 4+ messages in thread From: Wang, Jian J @ 2018-03-15 7:07 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star DxeCore will only apply EFI_MEMORY_RO to image CODE memory (controlled by PcdImageProtectionPolicy). Normally allocated rt_code/bs_code memory won't be marked as read-only. So my answer is NO. Regards, Jian > -----Original Message----- > From: Ni, Ruiyu > Sent: Thursday, March 15, 2018 2:56 PM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; > Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by > freeing read-only memory > > On 3/15/2018 2:22 PM, Jian J Wang wrote: > > SMM core will add a HEADER before each allocated pool memory and clean > > up this header once it's freed. If a block of allocated pool is marked > > as read-only after allocation (EfiRuntimeServicesCode type of pool in > > SMM will always be marked as read-only), #PF exception will be triggered > > during memory pool freeing. > > > > Normally EfiRuntimeServicesCode type of pool should not be freed in the > > real world. But some test suites will actually do memory free for all > > types of memory for the purpose of functionality and conformance test. > > So this issue should be fixed anyway. > > Does DxeCore have such problem? > > > > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Eric Dong <eric.dong@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/PiSmmCore/HeapGuard.c | 22 > ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > > index f9657f9baa..d5556eb79c 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > > +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > > @@ -941,6 +941,7 @@ AdjustMemoryF ( > > EFI_PHYSICAL_ADDRESS MemoryToTest; > > UINTN PagesToFree; > > UINT64 GuardBitmap; > > + UINT64 Attributes; > > > > if (Memory == NULL || NumberOfPages == NULL || *NumberOfPages == 0) > { > > return; > > @@ -949,6 +950,27 @@ AdjustMemoryF ( > > Start = *Memory; > > PagesToFree = *NumberOfPages; > > > > + // > > + // In case the memory to free is marked as read-only (e.g. > EfiRuntimeServicesCode). > > + // > > + if (mSmmMemoryAttribute != NULL) { > > + Attributes = 0; > > + mSmmMemoryAttribute->GetMemoryAttributes ( > > + mSmmMemoryAttribute, > > + Start, > > + EFI_PAGES_TO_SIZE (PagesToFree), > > + &Attributes > > + ); > > + if ((Attributes & EFI_MEMORY_RO) != 0) { > > + mSmmMemoryAttribute->ClearMemoryAttributes ( > > + mSmmMemoryAttribute, > > + Start, > > + EFI_PAGES_TO_SIZE (PagesToFree), > > + EFI_MEMORY_RO > > + ); > > + } > > + } > > + > > // > > // Head Guard must be one page before, if any. > > // > > > > > -- > Thanks, > Ray ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory 2018-03-15 7:07 ` Wang, Jian J @ 2018-03-16 6:44 ` Ni, Ruiyu 0 siblings, 0 replies; 4+ messages in thread From: Ni, Ruiyu @ 2018-03-16 6:44 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star On 3/15/2018 3:07 PM, Wang, Jian J wrote: > DxeCore will only apply EFI_MEMORY_RO to image CODE memory (controlled by > PcdImageProtectionPolicy). Normally allocated rt_code/bs_code memory won't be > marked as read-only. So my answer is NO. > > Regards, > Jian > > >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Thursday, March 15, 2018 2:56 PM >> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; >> Zeng, Star <star.zeng@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by >> freeing read-only memory >> >> On 3/15/2018 2:22 PM, Jian J Wang wrote: >>> SMM core will add a HEADER before each allocated pool memory and clean >>> up this header once it's freed. If a block of allocated pool is marked >>> as read-only after allocation (EfiRuntimeServicesCode type of pool in >>> SMM will always be marked as read-only), #PF exception will be triggered >>> during memory pool freeing. >>> >>> Normally EfiRuntimeServicesCode type of pool should not be freed in the >>> real world. But some test suites will actually do memory free for all >>> types of memory for the purpose of functionality and conformance test. >>> So this issue should be fixed anyway. >> >> Does DxeCore have such problem? >> >>> >>> Cc: Star Zeng <star.zeng@intel.com> >>> Cc: Eric Dong <eric.dong@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/PiSmmCore/HeapGuard.c | 22 >> ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c >> b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c >>> index f9657f9baa..d5556eb79c 100644 >>> --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c >>> +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c >>> @@ -941,6 +941,7 @@ AdjustMemoryF ( >>> EFI_PHYSICAL_ADDRESS MemoryToTest; >>> UINTN PagesToFree; >>> UINT64 GuardBitmap; >>> + UINT64 Attributes; >>> >>> if (Memory == NULL || NumberOfPages == NULL || *NumberOfPages == 0) >> { >>> return; >>> @@ -949,6 +950,27 @@ AdjustMemoryF ( >>> Start = *Memory; >>> PagesToFree = *NumberOfPages; >>> >>> + // >>> + // In case the memory to free is marked as read-only (e.g. >> EfiRuntimeServicesCode). >>> + // >>> + if (mSmmMemoryAttribute != NULL) { >>> + Attributes = 0; >>> + mSmmMemoryAttribute->GetMemoryAttributes ( >>> + mSmmMemoryAttribute, >>> + Start, >>> + EFI_PAGES_TO_SIZE (PagesToFree), >>> + &Attributes >>> + ); >>> + if ((Attributes & EFI_MEMORY_RO) != 0) { >>> + mSmmMemoryAttribute->ClearMemoryAttributes ( >>> + mSmmMemoryAttribute, >>> + Start, >>> + EFI_PAGES_TO_SIZE (PagesToFree), >>> + EFI_MEMORY_RO >>> + ); >>> + } >>> + } >>> + >>> // >>> // Head Guard must be one page before, if any. >>> // >>> >> >> >> -- >> Thanks, >> Ray Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> -- Thanks, Ray ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-16 6:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-15 6:22 [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory Jian J Wang 2018-03-15 6:55 ` Ni, Ruiyu 2018-03-15 7:07 ` Wang, Jian J 2018-03-16 6:44 ` Ni, Ruiyu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox