1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory

Jian J Wang posted 1 patch 6 years, 9 months ago
Failed in applying to current master (apply log)
MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
[edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory
Posted by Jian J Wang 6 years, 9 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory
Posted by Ni, Ruiyu 6 years, 9 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory
Posted by Wang, Jian J 6 years, 9 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: fix #PF caused by freeing read-only memory
Posted by Ni, Ruiyu 6 years, 9 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.