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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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