Regards, Jian
> -----Original Message----- > From: Ni, Ruiyu > Sent: Friday, March 02, 2018 7:58 PM > To: Laszlo Ersek <[email protected]>; Wang, Jian J <[email protected]>; > [email protected] > Cc: Dong, Eric <[email protected]> > Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > executable memory > > 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 <[email protected]> > >> Cc: Eric Dong <[email protected]> > >> Cc: Laszlo Ersek <[email protected]> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Jian J Wang <[email protected]> > >> --- > >> 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? > > a. Page attributes update has to be in page unit. If we want to avoid making stack memory executable, reserving it in a separate memory page is the only way I can think of. b. Checking MemDesc.Attributes against EFI_MEMORY_XP doesn't work here. The reason is that EFI_MEMORY_XP is set to configured type of memory via CPU Arch protocol in DxeCore code, which won't be recorded in GCD service data. Maybe checking PCD BIT0 is the only way according current situation. > -- > Thanks, > Ray _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
