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

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

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.
If checking is really needed, how about check MemDesc.Attributes 
EFI_MEMORY_XP bit?


-- 
Thanks,
Ray