Re: [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.

["Yao, Jiewen" <jiewen.yao@intel.com>]

Yes. You are right.

I agree all. Will send out V2 patch.

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, November 23, 2016 5:17 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@ml01.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.

On 11/22/16 12:47, Jiewen Yao wrote:
> This patch fixes the first part of
> https://bugzilla.tianocore.org/show_bug.cgi?id=242
>
> Previously, when SMM exception happens, "stack overflow" is misreported.
> This patch checked the PF address to see it is stack overflow, or
> it is caused by SMM page protection.
>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 28 +++++++++++++++++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h   |  9 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 27 ++++++++++++++++---
>  3 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 5033bc5..feca142 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -91,6 +91,8 @@ SmiPFHandler (
>    )
>  {
>    UINTN             PFAddress;
> +  UINTN             GuardPageAddress;
> +  UINTN             CpuIndex;
>
>    ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -98,10 +100,30 @@ SmiPFHandler (
>
>    PFAddress = AsmReadCr2 ();
>
> -  if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> -      (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> +  //
> +  // If a page fault occurs in SMRAM range, it might be in a SMM stack guard page,
> +  // or SMM page protection violation.
> +  //
> +  if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
>        (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> -    DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> +    CpuIndex = GetCpuIndex ();
> +    GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> +    if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> +        (PFAddress >= GuardPageAddress) &&
> +        (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> +      DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> +    }
> +    if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
> +      DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp);
> +      );
> +    } else {
> +      DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
> +      );
> +    }
>      CpuDeadLoop ();
>    }
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> index b6fb5cf..04a3dfb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> @@ -105,6 +105,15 @@ InitPaging (
>    VOID
>    );
>
> +/**
> +  Get CPU Index from APIC ID.
> +
> +**/
> +UINTN
> +GetCpuIndex (
> +  VOID
> +  );
> +
>  //
>  // The flag indicates if execute-disable is supported by processor.
>  //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 531e188..ec8eab7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -804,6 +804,8 @@ SmiPFHandler (
>    )
>  {
>    UINTN             PFAddress;
> +  UINTN             GuardPageAddress;
> +  UINTN             CpuIndex;
>
>    ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -817,12 +819,29 @@ SmiPFHandler (
>    }
>
>    //
> -  // If a page fault occurs in SMRAM range, it should be in a SMM stack guard page.
> +  // If a page fault occurs in SMRAM range, it might be in a SMM stack guard page,
> +  // or SMM page protection violation.
>    //
> -  if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> -      (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> +  if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
>        (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> -    DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> +    CpuIndex = GetCpuIndex ();
> +    GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> +    if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> +        (PFAddress >= GuardPageAddress) &&
> +        (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> +      DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> +    }
> +    if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
> +      DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp);
> +      );
> +    } else {
> +      DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
> +      );
> +    }
>      CpuDeadLoop ();
>    }
>
>

(1) The "PFAddress" variable is UINTN in both variants. Printing UINTN
with %lx is incorrect in the Ia32 case, because %lx takes an UINT64.

I suggest the following pattern for printing UINTN values portably:

  DEBUG ((level, "%lx", (UINT64)Value));

That is, always use %lx and add an explicit cast. The cast is a no-op on
X64, and does the right conversion on Ia32. The %lx conversion
specification matches the result in both cases.

(2) I tested the X64 stack overflow branch as follows:

I temporarily reverted (on top of your present patch) commit
0d0c245dfb147 ("OvmfPkg: set SMM stack size to 16KB"), and then ran the
certificate enrollment application that originally triggered the stack
overflow.

This is the debug output I got:

> SMM stack overflow!
> SMM exception at write (0x7FF9CFEC)
> It is invoked from the instruction before IP(0x7FFE4CA3) in module
(.../Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/PiSmmCore/PiSmmCore/DEBUG/PiSmmCore.dll)

Shouldn't you change the IA32_PF_EC_ID check into an "else if"? Because
I think once you determine the stack overflow, we shouldn't look for any
other kind of exception.

(3) I tested the Ia32 execution / write fault branch as follows:

I temporarily reverted (on top of your present patch) commit
750ec4cabd07 ("UefiCpuPkg/PiSmmCpu: Check XdSupport before set NX.").
Then, under the circumstances I reported in
<https://lists.01.org/pipermail/edk2-devel/2016-November/004929.html>, I
get:

> ConvertPageEntryAttribute 0x7FEA4067->0x800000007FEA4067
> SMM exception at write (0x7FEA4890)
> It is invoked from the instruction before IP(0x7FFB879A) in module
(.../Build/OvmfIa32/NOOPT_GCC48/IA32/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm/DEBUG/PiSmmCpuDxeSmm.dll)

Here the page fault is explained by the fact that we set the unsupported
NX bit in the PTE that maps the page, and then we try to read from the
page (not write to it). If I remember correctly at least.

Is it possible to distinguish "read" from "write" in the fault symptoms?
If it is, then I suggest to customize the error message. If it is not
possible, then I suggest to replace the word "write" with "access".

Thanks!
Laszlo