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

["Laszlo Ersek" <lersek@redhat.com>]

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
> 
> 
> 
> 
> 
>