Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

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

On 3/7/2018 12:28 PM, Wu, Hao A wrote:
> Hi Ray,
> 
> Below are the answers to your feedbacks:
> 
>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Wednesday, March 07, 2018 12:14 PM
>> To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org
>> Cc: Zeng, Star; Dong, Eric
>> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
>> guard
>>
>> I think the original patch is fine.
>>
>> StackBase is already checked by using ASSERT before.
>>> +        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
>>
>> MemMap entry must be page aligned.
>>
>> No additional check is required here.
>>
>> Thank you
>> Yao Jiewen
>>
>>
>>> -----Original Message-----
>>> From: Wang, Jian J
>>> Sent: Wednesday, March 7, 2018 11:40 AM
>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@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 v2 0/2] Resolve feature conflict between NX and Stack
>> guard
>>>
>>>
>>>
>>> Regards,
>>> Jian
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ni, Ruiyu
>>>> Sent: Wednesday, March 07, 2018 11:17 AM
>>>> To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star
>> <star.zeng@intel.com>;
>>>> Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
>>>> Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
>>>> guard
>>>>
>>>> On 3/6/2018 9:33 PM, Hao Wu wrote:
>>>>> V2 changes:
>>>>>
>>>>> A. Use Hoblib APIs to get the base of stack from Hob.
>>>>> B. Remove unnecessary local variable used in function
>>>>>      InitializeDxeNxMemoryProtectionPolicy().
>>>>>
>>>>> V1 history:
>>>>>
>>>>> If enabled, NX memory protection feature will mark some types of active
>>>>> memory as NX (non-executable), which includes the first page of the stack.
>>>>> This will overwrite the attributes of the first page of the stack if the
>>>>> stack guard feature is also enabled.
>>>>>
>>>>> The series will override the attributes setting to the first page of the
>>>>> stack by adding back the 'EFI_MEMORY_RP' attribute when the stack
>> guard
>>>>> feature is enabled.
>>>>>
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> 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>
>>>>>
>>>>> Hao Wu (2):
>>>>>     MdeModulePkg/Core: Refine handling NULL detection in NX setting
>>>>>     MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
>>>>>
>>>>>    MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
>>>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
>>>> +++++++++++++++++++++++----
>>>>>    2 files changed, 67 insertions(+), 11 deletions(-)
>>>>>
>>>>
>>>>         if (MemoryMapEntry->PhysicalStart == 0 &&
>>>>             PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>>>>
>>>>           ASSERT (MemoryMapEntry->NumberOfPages > 0);
>>>>           //
>>>>           // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer
>>>> detection is
>>>>           // enabled.
>>>>           //
>>>> [Ray] 1. I prefer to move the above comments before the "if (...)".
> 
> Yes. I agree that moving the comments block out of the 'if' statement is
> more readable. I will update according to your suggestion when I pushing
> the changes.
> 
>>>>
>>>>           SetUefiImageMemoryAttributes (
>>>>             0,
>>>>             EFI_PAGES_TO_SIZE (1),
>>>>             EFI_MEMORY_RP | Attributes);
>>>>         }
>>>>
>>>>         if (StackBase != 0 &&
>>>>             (StackBase >= MemoryMapEntry->PhysicalStart &&
>>>>              StackBase <  MemoryMapEntry->PhysicalStart +
>>>>                           LShiftU64
>>> (MemoryMapEntry->NumberOfPages,
>>>> EFI_PAGE_SHIFT)) &&
>>>>             PcdGetBool (PcdCpuStackGuard)) {
>>>>
>>>>           //
>>>>           // Add EFI_MEMORY_RP attribute for the first page of the stack
>>>> if stack
>>>>           // guard is enabled.
>>>>           //
>>>>           SetUefiImageMemoryAttributes (
>>>>             StackBase,
>>>>             EFI_PAGES_TO_SIZE (1),
>>>>             EFI_MEMORY_RP | Attributes);
>>>> [Ray] 2. The StackBase is directly used here. So do we need to check
>>>> whether it's page aligned? Do we need to check whether the range
>>>> [StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
>>>>         }
>>>
>>> If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for
>>> StackBase
>>> should make sure all the conditions you mentioned, but not here.
>>>
> 
> Just as Jiewen mentioned in the previous reply. An ASSERT:
> ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
> 
> is added to ensure the stack base fetched from Hob is page-size aligned.
> 
> And the below check:
> (StackBase >= MemoryMapEntry->PhysicalStart &&
>   StackBase <  MemoryMapEntry->PhysicalStart +
>                LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT))
> 
> together with the fact that MemMap is page aligned (also mentioned by
> Jiewen) ensures that the first page of the stack is cover by the memory
> range of the MemMap.

I agree. Thanks for the explanation.
Please move the comments location when checking in the patch.

> 
> Best Regards,
> Hao Wu
> 
>>>>
>>>> --
>>>> Thanks,
>>>> Ray


-- 
Thanks,
Ray