Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory

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

On 3/3/2018 11:10 PM, Laszlo Ersek wrote:
> Hi Ray,
> 
> On 03/02/18 12:57, Ni, Ruiyu wrote:
>> On 3/2/2018 7:45 PM, Laszlo Ersek wrote:
>>> On 03/02/18 06:58, Jian J Wang wrote:
>>>> if PcdDxeNxMemoryProtectionPolicy is enabled for EfiReservedMemoryType
>>>> of memory, #PF will be triggered for each APs after ExitBootServices
>>>> in SCRT test. The root cause is that AP wakeup code executed at that
>>>> time is stored in memory of type EfiReservedMemoryType (referenced by
>>>> global mReservedApLoopFunc), which is marked as non-executable.
>>>>
>>>> This patch fixes this issue by setting memory of mReservedApLoopFunc to
>>>> be executable immediately after allocation.
>>>>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>>> ---
>>>>    UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> index fd2317924f..5fcb08677c 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> @@ -399,6 +399,21 @@ InitMpGlobalData (
>>>>                       &Address
>>>>                       );
>>>>      ASSERT_EFI_ERROR (Status);
>>>> +
>>>> +  //
>>>> +  // Make sure that the buffer memory is executable.
>>>> +  //
>>>> +  Status = gDS->GetMemorySpaceDescriptor (Address, &MemDesc);
>>>> +  if (!EFI_ERROR (Status)) {
>>>> +    gDS->SetMemorySpaceAttributes (
>>>> +           Address,
>>>> +           EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
>>>> +             CpuMpData->AddressMap.RelocateApLoopFuncSize
>>>> +             )),
>>>> +           MemDesc.Attributes & (~EFI_MEMORY_XP)
>>>> +           );
>>>> +  }
>>>> +
>>>>      mReservedApLoopFunc = (VOID *) (UINTN) Address;
>>>>      ASSERT (mReservedApLoopFunc != NULL);
>>>>      mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE
>>>> (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
>>>>
>>>
>>> Honestly, I see little point in the "Dxe Nx Memory Protection Policy"
>>> when we then override it *every time* it gets in our way.
>>> "RelocateApLoopFuncSize" is likely significantly smaller than a full
>>> page, so we're making a good chunk of the "safe stack(s)" executable too.
>>>
>>> Anyway, can you perhaps check BIT0 (standing for EfiReservedMemoryType)
>>> in PcdDxeNxMemoryProtectionPolicy, to see if the above hack is necessary?
>>>
>>> Thanks
>>> Laszlo
>>>
>>
>> Checking PCD is not very good I think.
> 
> I'll look at v2 next week, just a short comment now: I don't understand
> why you are opposed to the PCD check. Reserved memory is generally
> expected to be executable, and the issue surfaces *precisely* when
> reserved memory is marked as noexec in the PCD in question. That's
> exactly the reason why the above logic is needed.

Sorry I didn't see this comments.
Standing from CPU owner's perspective, I don't like to check PCD here.
Or, to be straight, I want to have least knowledge of any extra
non-spec defined interfaces.

> 
> Approach it from this side: if I was reading the code (without the PCD
> check), I would ask myself, "why are we checking for noexec here? we
> just allocated this chunk of reserved memory from normal system memory.
> It should be executable already".
> 
> So, I think the PCD check is somewhat important functionally, and quite
> important for documentation purposes. And it's a lot better than adding
> a comment.
> 
>> If checking is really needed, how about check MemDesc.Attributes
>> EFI_MEMORY_XP bit?
> 
> I think that would check for the symptom, not for the root cause. To a
> person reading the code, it doesn't provide any more information than
> the current code. "Okay, it's not executable, so we mark it executable
> manually. But why isn't it executable in the first place? We just
> allocated it from system memory."This code is for AP to sleep, even at runtime, before OS takes control
of AP. So making it as BS code may cause issues.

> 
> Thanks,
> Laszlo
> 


-- 
Thanks,
Ray