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

[Laszlo Ersek <lersek@redhat.com>]

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