Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

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

On 2/1/2018 9:17 AM, Wang, Jian J wrote:
> You're right. Using a mask or separating the API into two (SetMemoryAttributes/ClearMemoryAttributes)
> is much better and can avoid many potential issues.
> 
> Regards,
> Jian
> 

For now the patch is good enough to leave NULL pointer detection
feature enabled.

Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>


> 
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Tuesday, January 30, 2018 12:38 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
>> Jiewen <jiewen.yao@intel.com>
>> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
>> NULL detection
>>
>> On 1/29/2018 7:09 PM, Jian J Wang wrote:
>>> If enabled, NX memory protection feature will mark all free memory as
>>> NX (non-executable), including page 0. This will overwrite the attributes
>>> of page 0 if NULL pointer detection feature is also enabled and then
>>> compromise the functionality of it. The solution is skipping the NX
>>> attributes setting to page 0 if NULL pointer detection feature is enabled.
>>>
>>> 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/Dxe/Misc/MemoryProtection.c | 20
>> ++++++++++++++++----
>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> index 862593f562..150167bf66 100644
>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
>>>
>>>        Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
>>> Type);
>>>        if (Attributes != 0) {
>>> -      SetUefiImageMemoryAttributes (
>>> -        MemoryMapEntry->PhysicalStart,
>>> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>> -        Attributes);
>>> +      if (MemoryMapEntry->PhysicalStart == 0 &&
>>> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>>> +        //
>>> +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
>>> +        // overwritten.
>>> +        //
>>> +        SetUefiImageMemoryAttributes (
>>> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
>>> +          Attributes);
>>> +      } else {
>>> +        SetUefiImageMemoryAttributes (
>>> +          MemoryMapEntry->PhysicalStart,
>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>> +          Attributes);
>>> +      }
>>>        }
>>>        MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>> DescriptorSize);
>>>      }
>>>
>> Does this bug expose an API-level issue?
>> SetUefiImageMemoryAttributes () should also accept a Mask value?
>>
>> --
>> Thanks,
>> Ray


-- 
Thanks,
Ray