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