Re: [RFC v5 0/8] Stack trace support in X64 exception handling

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

Thanks Paulo.

The series looks really good. I give it thumb up. :-)

The 8 patches keep updating 4 files, so I squash them when I review. The comment below is for the final code, not for a specific patch.

1) CpuExceptionCommon.c: IsLinearAddressRangeValid().
  //
  // Check for valid input parameters
  //
  if (LinearAddressStart == 0 || LinearAddressEnd == 0 ||
      LinearAddressStart > LinearAddressEnd) {
    return FALSE;
  }

I think LinearAddressStart is a valid case. In BIOS, we do update IVT in 0 address when CSM is enabled.
I do not think we need exclude it here. If we enabled ZeroPointer protection, it can be excluded in page table parsing.

2) I found below logic appears in 2 functions - DumpImageModuleNames() and DumpStacktrace(). Is that possible to merge them?
We can calculate in DumpImageAndCpuContent() and use Eip as parameter. Just in case there is 3rd function need use EIP, it won't miss the calculation.
(It is a bug fix we did recently.)

  //
  // Set current EIP address
  //
  if ((ExceptionType == EXCEPT_IA32_PAGE_FAULT) &&
      ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0)) {
    //
    // The EIP in SystemContext could not be used
    // if it is page fault with I/D set.
    //
    Eip = *(UINT32 *)(UINTN)SystemContext.SystemContextIa32->Esp;
  } else {
    Eip = SystemContext.SystemContextIa32->Eip;
  }

3) I am a little surprised on PeCoffSearchImageBase() issue.

We have 4 PeCoffSearchImageBase() call in each arch. DumpImageModuleNames() calls twice and DumpStacktrace() calls twice.
Do you know which specific one triggers the zero address #PF issue?

  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\ArchExceptionHandler.c(547):  ImageBase = PeCoffSearchImageBase (Eip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\ArchExceptionHandler.c(613):    ImageBase = PeCoffSearchImageBase (Eip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\ArchExceptionHandler.c(682):  ImageBase = PeCoffSearchImageBase (Eip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\ArchExceptionHandler.c(741):    ImageBase = PeCoffSearchImageBase (Eip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchExceptionHandler.c(540):  ImageBase = PeCoffSearchImageBase (Rip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchExceptionHandler.c(613):    ImageBase = PeCoffSearchImageBase (Rip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchExceptionHandler.c(710):  ImageBase = PeCoffSearchImageBase (Rip);
  C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchExceptionHandler.c(779):    ImageBase = PeCoffSearchImageBase (Rip);

The EIP from SystemContext seems good. I assume there is not a problem here.

  if ((ExceptionType == EXCEPT_IA32_PAGE_FAULT) &&
      ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0)) {
    //
    // The EIP in SystemContext could not be used
    // if it is page fault with I/D set.
    //
    Eip = *(UINT32 *)(UINTN)SystemContext.SystemContextIa32->Esp;
  } else {
    Eip = SystemContext.SystemContextIa32->Eip;
  }

  //
  // Get initial PE/COFF image base address from current EIP
  //
  ImageBase = PeCoffSearchImageBase (Eip);

But the EIP from stack frame is unknown and risky. Especially for the code in mode-switch, such as PEI->DXE, UEFI->CSM, AP wakeup->AP C code, SMM entrypoint->SMM C code.
Should we add a check for EIP here?

    //
    // Set EIP with return address from current stack frame
    //
    Eip = *(UINT32 *)((UINTN)Ebp + 4);

    //
    // If EIP is zero, then stop unwinding the stack
    //
    if (Eip == 0) {
      break;
    }

    //
    // Search for the respective PE/COFF image based on EIP
    //
    ImageBase = PeCoffSearchImageBase (Eip);

If you can help us do some more investigation on the root-cause and narrow down the issue, I will appreciate that.

We can decide how to fix once it is root-caused.

Maybe we just do a simple IsLogicalAddressValid () check before we call PeCoffSearchImageBase(). :-)


Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Monday, January 15, 2018 8:23 AM
> To: edk2-devel@lists.01.org
> Cc: Rick Bramley <richard.bramley@hp.com>; Dong, Eric
> <eric.dong@intel.com>; Kimon Berlin <kimon.berlin@hp.com>; Andrew Fish
> <afish@apple.com>; Yao, Jiewen <jiewen.yao@intel.com>; Diego Medaglia
> <diego.meaglia@hp.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling
> 
> Hi,
> 
> This series adds stack trace support during IA32 and X64 CPU exceptions.
> 
> Informations like back trace, stack contents and image module names
> (that were part of the call stack) will be dumped out.
> 
> The current limitation is that it relies on available frame pointers
> (GCC only) in order to successfully unwind the stack.
> 
> Jiewen,
> 
> Thank you very much for your time on this. I've applied the changes you
> suggested, as well as tested it on IA32 PAE paging mode - it worked as
> expected.
> 
> Other than that, I also tested the stack trace in SMM code by manually
> calling CpuBreakPoint() and then it broke with another exception
> (page fault). I didn't have much time to look into that, but what I've
> observed is that the page fault ocurred during the search of PE/COFF
> image base address (in PeCoffSearchImageBase). The function attempts to
> search for the image base from "Address" through 0, so any of those
> dereferenced addresses triggers the page fault.
> 
> Do you know how we could fix that issue? Perhaps introducing a
> AddressValidationLib (as Brian suggested previously) and use it within
> PeCoffSearchImageBase()?
> 
> I'd also like to thank Brian & Jeff for all the support!
> 
> Thanks
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: stacktrace_v5
> 
> Cc: Rick Bramley <richard.bramley@hp.com>
> Cc: Kimon Berlin <kimon.berlin@hp.com>
> Cc: Diego Medaglia <diego.meaglia@hp.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Brian Johnson <brian.johnson@hpe.com>
> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paulo Alcantara <paulo@hp.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
> ---
> 
> v1 -> v2:
>   * Add IA32 arch support (GCC toolchain only)
>   * Replace hard-coded stack alignment value (16) with
>     CPU_STACK_ALIGNMENT.
>   * Check for proper stack and frame pointer alignments.
>   * Fix initialization of UnwoundStacksCount to 1.
>   * Move GetPdbFileName() to common code since it will be used by both
>     IA32 and X64 implementations.
> 
> v2 -> v3:
>   * Fixed wrong assumption about "RIP < ImageBase" to start searching
>     for another PE/COFF image. That is, RIP may point to lower and
>     higher addresses for any other PE/COFF images. Both IA32 & X64.
>     (Thanks Andrew & Jiewen)
>   * Fixed typo: unwond -> unwound. Both IA32 & X64. (Thanks Brian)
> 
> v3 -> v4:
>   * Validate all frame/stack pointer addresses before dereferencing them
>     as requested by Brian & Jiewen.
>   * Correctly print out IP addresses during the stack traces (by Jeff)
> 
> v4 -> v5:
>   * Fixed address calculations and improved code as suggested by Jiewen.
>   * Fixed parameter validation as suggested by Brian.
>   * Tested stack stack with IA32 PAE paging mode.
> 
> Paulo Alcantara (8):
>   UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support
>   UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName()
>   UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support
>   UefiCpuPkg/CpuExceptionHandlerLib: Add helper to validate memory
>     addresses
>   UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers
>   UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses
>   UefiCpuPkg/CpuExceptionHandlerLib: Validate memory address ranges
>   UefiCpuPkg/CpuExceptionHandlerLib: Add early check in
>     DumpStackContents
> 
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> | 537 ++++++++++++++++++--
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> |  59 ++-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c |
> 483 +++++++++++++++++-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |
> 426 +++++++++++++++-
>  4 files changed, 1435 insertions(+), 70 deletions(-)
> 
> --
> 2.14.3
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel