Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM shadow stack overflow

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, April 6, 2021 10:27 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Sheng, W <w.sheng@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Feng, Roger <roger.feng@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM shadow stack overflow
> 
> Ray,
> 
> On 03/29/21 07:13, Yao, Jiewen wrote:
> > Thank you very much!
> >
> > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> can you please review and merge this patch? You were the UefiCpuPkg
> reviewer on the following two commits as well:
> 
> 3eb69b081c68 ("UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86
> SMM.", 2019-02-28)
> 
> ef91b07388e1 ("UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not
> correct", 2021-03-02)
> 
> Thanks
> Laszlo
> 
> >
> >> -----Original Message-----
> >> From: Sheng, W <w.sheng@intel.com>
> >> Sent: Friday, March 26, 2021 2:33 PM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> >> <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Feng, Roger
> >> <roger.feng@intel.com>
> >> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM
> >> shadow stack overflow
> >>
> >> Hi Jiewen,
> >> In current code, if SMM stack guard is enabled, there is a guard page at the top
> >> of SMM shadow stack.
> >> If SMM shadow stack overflow Happens, it will touch the guard page, and
> >> trigger the #PF exception.
> >> In this patch, I will check the PFAddress in SmiPFHandler(), if it belongs to the
> >> range of SMM shadow stack guard page, I will show the error message.
> >>
> >> unit test:
> >> I use recursive function to do the test. In each function call, it will push the
> >> return address to the SMM shadow stack.
> >> When the loop reaches to a certain amount, it will finally touch the guard page,
> >> and trigger #PF exception.
> >>
> >> Thank you
> >> BR
> >> Sheng Wei
> >>
> >>> -----Original Message-----
> >>> From: Yao, Jiewen <jiewen.yao@intel.com>
> >>> Sent: 2021年3月26日 14:14
> >>> To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io
> >>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> >>> Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>;
> >>> Feng, Roger <roger.feng@intel.com>
> >>> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM
> >>> shadow stack overflow
> >>>
> >>> Hi
> >>> Would you please share the info on how you do unit test for the new added
> >>> code?
> >>>
> >>> Thank you
> >>>
> >>>> -----Original Message-----
> >>>> From: Sheng, W <w.sheng@intel.com>
> >>>> Sent: Friday, March 26, 2021 2:04 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> >>>> <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Feng,
> >>>> Roger <roger.feng@intel.com>
> >>>> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Support detect SMM
> >>> shadow
> >>>> stack overflow
> >>>>
> >>>> Use SMM stack guard feature to detect SMM shadow stack overflow.
> >>>>
> >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3280
> >>>>
> >>>> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> >>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>> Cc: Ray Ni <ray.ni@intel.com>
> >>>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>> Cc: Roger Feng <roger.feng@intel.com>
> >>>> ---
> >>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 9 ++++++++-
> >>>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >>>> index 07e7ea70de..6902584b1f 100644
> >>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >>>> @@ -1016,6 +1016,7 @@ SmiPFHandler (
> >>>>  {
> >>>>    UINTN             PFAddress;
> >>>>    UINTN             GuardPageAddress;
> >>>> +  UINTN             ShadowStackGuardPageAddress;
> >>>>    UINTN             CpuIndex;
> >>>>
> >>>>    ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT); @@ -1032,7
> >>>> +1033,7 @@ SmiPFHandler (
> >>>>    }
> >>>>
> >>>>    //
> >>>> -  // If a page fault occurs in SMRAM range, it might be in a SMM
> >>>> stack guard page,
> >>>> +  // If a page fault occurs in SMRAM range, it might be in a SMM
> >>>> + stack/shadow
> >>>> stack guard page,
> >>>>    // or SMM page protection violation.
> >>>>    //
> >>>>    if ((PFAddress >= mCpuHotPlugData.SmrrBase) && @@ -1040,10 +1041,16
> >>>> @@ SmiPFHandler (
> >>>>      DumpCpuContext (InterruptType, SystemContext);
> >>>>      CpuIndex = GetCpuIndex ();
> >>>>      GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE +
> >>> CpuIndex
> >>>> * (mSmmStackSize + mSmmShadowStackSize));
> >>>> +    ShadowStackGuardPageAddress = (mSmmStackArrayBase +
> >>> mSmmStackSize
> >>>> + EFI_PAGE_SIZE + CpuIndex * (mSmmStackSize +
> >>> mSmmShadowStackSize));
> >>>>      if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> >>>>          (PFAddress >= GuardPageAddress) &&
> >>>>          (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> >>>>        DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> >>>> +    } else if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> >>>> +        (mSmmShadowStackSize > 0) &&
> >>>> +        (PFAddress >= ShadowStackGuardPageAddress) &&
> >>>> +        (PFAddress < (ShadowStackGuardPageAddress + EFI_PAGE_SIZE))) {
> >>>> +      DEBUG ((DEBUG_ERROR, "SMM shadow stack overflow!\n"));
> >>>>      } else {
> >>>>        if ((SystemContext.SystemContextX64->ExceptionData &
> >>>> IA32_PF_EC_ID) !=
> >>>> 0) {
> >>>>          DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n",
> >>>> PFAddress));
> >>>> --
> >>>> 2.16.2.windows.1
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 
>