* [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory @ 2018-03-02 5:58 Jian J Wang 2018-03-02 11:45 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Jian J Wang @ 2018-03-02 5:58 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Eric Dong, Laszlo Ersek 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)); -- 2.15.1.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-02 5:58 [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory Jian J Wang @ 2018-03-02 11:45 ` Laszlo Ersek 2018-03-02 11:57 ` Ni, Ruiyu 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2018-03-02 11:45 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Eric Dong 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-02 11:45 ` Laszlo Ersek @ 2018-03-02 11:57 ` Ni, Ruiyu 2018-03-03 1:31 ` Wang, Jian J 2018-03-03 15:10 ` Laszlo Ersek 0 siblings, 2 replies; 10+ messages in thread From: Ni, Ruiyu @ 2018-03-02 11:57 UTC (permalink / raw) To: Laszlo Ersek, Jian J Wang, edk2-devel; +Cc: Eric Dong 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-02 11:57 ` Ni, Ruiyu @ 2018-03-03 1:31 ` Wang, Jian J 2018-03-03 7:08 ` Ni, Ruiyu 2018-03-03 15:10 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-03-03 1:31 UTC (permalink / raw) To: Ni, Ruiyu, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric Regards, Jian > -----Original Message----- > From: Ni, Ruiyu > Sent: Friday, March 02, 2018 7:58 PM > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com> > 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 <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? > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-03 1:31 ` Wang, Jian J @ 2018-03-03 7:08 ` Ni, Ruiyu 2018-03-03 8:10 ` Wang, Jian J 0 siblings, 1 reply; 10+ messages in thread From: Ni, Ruiyu @ 2018-03-03 7:08 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric Thanks/Ray > -----Original Message----- > From: Wang, Jian J > Sent: Saturday, March 3, 2018 9:32 AM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com> > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > executable memory > > > > Regards, > Jian > > > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Friday, March 02, 2018 7:58 PM > > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J > > <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com> > > 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 <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? > > > > > > 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. Will MEMORY_XP be recorded in GCD in future? Based on today's implementation, I prefer to not check. > > > -- > > Thanks, > > Ray ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-03 7:08 ` Ni, Ruiyu @ 2018-03-03 8:10 ` Wang, Jian J 2018-03-03 9:01 ` Wang, Jian J 0 siblings, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-03-03 8:10 UTC (permalink / raw) To: Ni, Ruiyu, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric > Will MEMORY_XP be recorded in GCD in future? > Based on today's implementation, I prefer to not check. > Yes, it's in plan. Since it will impact the memory map layout, we have to be very careful to make those changes and do thorough OS boot tests. Regards, Jian > -----Original Message----- > From: Ni, Ruiyu > Sent: Saturday, March 03, 2018 3:08 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com> > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > executable memory > > > > Thanks/Ray > > > -----Original Message----- > > From: Wang, Jian J > > Sent: Saturday, March 3, 2018 9:32 AM > > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > > edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com> > > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > > executable memory > > > > > > > > Regards, > > Jian > > > > > > > -----Original Message----- > > > From: Ni, Ruiyu > > > Sent: Friday, March 02, 2018 7:58 PM > > > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > > Cc: Dong, Eric <eric.dong@intel.com> > > > 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 <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? > > > > > > > > > > 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. > > Will MEMORY_XP be recorded in GCD in future? > Based on today's implementation, I prefer to not check. > > > > > > -- > > > Thanks, > > > Ray ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-03 8:10 ` Wang, Jian J @ 2018-03-03 9:01 ` Wang, Jian J 0 siblings, 0 replies; 10+ messages in thread From: Wang, Jian J @ 2018-03-03 9:01 UTC (permalink / raw) To: Wang, Jian J, Ni, Ruiyu, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric Hi Ray and Laszlo, I'll send out a v2 patch. Please give your comments based the new one. Regards, Jian > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, > Jian J > Sent: Saturday, March 03, 2018 4:10 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2- > devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc > in executable memory > > > Will MEMORY_XP be recorded in GCD in future? > > Based on today's implementation, I prefer to not check. > > > > Yes, it's in plan. Since it will impact the memory map layout, we have to be very > careful to make those changes and do thorough OS boot tests. > > Regards, > Jian > > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Saturday, March 03, 2018 3:08 PM > > To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; > > edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com> > > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > > executable memory > > > > > > > > Thanks/Ray > > > > > -----Original Message----- > > > From: Wang, Jian J > > > Sent: Saturday, March 3, 2018 9:32 AM > > > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > > > edk2-devel@lists.01.org > > > Cc: Dong, Eric <eric.dong@intel.com> > > > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > > > executable memory > > > > > > > > > > > > Regards, > > > Jian > > > > > > > > > > -----Original Message----- > > > > From: Ni, Ruiyu > > > > Sent: Friday, March 02, 2018 7:58 PM > > > > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > > > Cc: Dong, Eric <eric.dong@intel.com> > > > > 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 <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? > > > > > > > > > > > > > > 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. > > > > Will MEMORY_XP be recorded in GCD in future? > > Based on today's implementation, I prefer to not check. > > > > > > > > > -- > > > > Thanks, > > > > Ray > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-02 11:57 ` Ni, Ruiyu 2018-03-03 1:31 ` Wang, Jian J @ 2018-03-03 15:10 ` Laszlo Ersek 2018-03-05 5:06 ` Ni, Ruiyu 1 sibling, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2018-03-03 15:10 UTC (permalink / raw) To: Ni, Ruiyu, Jian J Wang, edk2-devel; +Cc: Eric Dong 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. 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." Thanks, Laszlo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 2018-03-03 15:10 ` Laszlo Ersek @ 2018-03-05 5:06 ` Ni, Ruiyu 0 siblings, 0 replies; 10+ messages in thread From: Ni, Ruiyu @ 2018-03-05 5:06 UTC (permalink / raw) To: Laszlo Ersek, Jian J Wang, edk2-devel; +Cc: Eric Dong 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory @ 2018-03-03 9:02 Jian J Wang 0 siblings, 0 replies; 10+ messages in thread From: Jian J Wang @ 2018-03-03 9:02 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Eric Dong, Laszlo Ersek > v2 changes: > a. Reserve memory of mReservedApLoopFunc and mReservedTopOfApStack > separately to avoid making mReservedTopOfApStack executable. 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 | 38 +++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c index fd2317924f..e7ed21c6cd 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -388,9 +388,9 @@ InitMpGlobalData ( // Allocating it in advance since memory services are not available in // Exit Boot Services callback function. // - ApSafeBufferSize = CpuMpData->AddressMap.RelocateApLoopFuncSize; - ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE; - + ApSafeBufferSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES ( + CpuMpData->AddressMap.RelocateApLoopFuncSize + )); Address = BASE_4GB - 1; Status = gBS->AllocatePages ( AllocateMaxAddress, @@ -399,9 +399,39 @@ InitMpGlobalData ( &Address ); ASSERT_EFI_ERROR (Status); + mReservedApLoopFunc = (VOID *) (UINTN) Address; ASSERT (mReservedApLoopFunc != NULL); - mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); + + // + // Make sure that the buffer memory is executable if NX protection is enabled + // for EfiReservedMemoryType. + // + // TODO: Check EFI_MEMORY_XP bit set or not once it's available in DXE GCD + // service. + // + Status = gDS->GetMemorySpaceDescriptor (Address, &MemDesc); + if (!EFI_ERROR (Status)) { + gDS->SetMemorySpaceAttributes ( + Address, + ApSafeBufferSize, + MemDesc.Attributes & (~EFI_MEMORY_XP) + ); + } + + ApSafeBufferSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES ( + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE + )); + Address = BASE_4GB - 1; + Status = gBS->AllocatePages ( + AllocateMaxAddress, + EfiReservedMemoryType, + EFI_SIZE_TO_PAGES (ApSafeBufferSize), + &Address + ); + ASSERT_EFI_ERROR (Status); + + mReservedTopOfApStack = (UINTN) Address + ApSafeBufferSize; ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0); CopyMem ( mReservedApLoopFunc, -- 2.15.1.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-05 5:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-02 5:58 [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory Jian J Wang 2018-03-02 11:45 ` Laszlo Ersek 2018-03-02 11:57 ` Ni, Ruiyu 2018-03-03 1:31 ` Wang, Jian J 2018-03-03 7:08 ` Ni, Ruiyu 2018-03-03 8:10 ` Wang, Jian J 2018-03-03 9:01 ` Wang, Jian J 2018-03-03 15:10 ` Laszlo Ersek 2018-03-05 5:06 ` Ni, Ruiyu -- strict thread matches above, loose matches on Subject: below -- 2018-03-03 9:02 Jian J Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox