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

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

On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:
> 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.
>>>> +        //

By the way, could you please add an assertion here?
ASSERT (MemoryMapEntry->NumberOfPages != 0);
>>>> +        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