Re: [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support

[Paulo Alcantara <pcacjr@zytor.com>]

Hi Jiewen,

(sorry for the late response)

On 11/17/2017 1:43 AM, Yao, Jiewen wrote:
> Thanks for your reply. Comments below:
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Friday, November 17, 2017 6:13 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>; edk2-devel@lists.01.org; Laszlo Ersek
>> <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: RE: [edk2] [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add
>> stack trace support
>>
>> Hi Jiewen,
>>
>> On Wed, November 15, 2017 11:57 pm, Yao, Jiewen wrote:
>>> Hi Paulo
>>> Thanks to bring this cool feature.
>>>
>>> I have same feeling as Jeff Fan. It is great!
>>>
>>> I have some questions, hope you can clarify.
>>> 1) Would you please let us know which tool change is validated? Such as
>>> MSVC, or GCC?
>>
>> GCC only. This should not work on MSVC because it relies on frame pointers.
> 
> [Jiewen] Not work is OK.
> But my question is more about: Have you tested? Or have you tried?
> Do you see basic exception information can be dumped without any problem for MSVC?
> Or do you see system crash immediately for MSVC?

No, I haven't tried it. I built it with VS2015 and just run 'objdump -x 
-D' in an arbitrary .efi to check whether RBP was being used in 
function's prologues. They weren't and then I didn't even give it try. 
I'll test it though and then let you know its behavior.

> 
> 
> 
>>> 2) Would you please let us know which phase is validated? Such as PEI
>>> PreMemory, PEI PostMemory, DXE, SMM?
>>
>> DXE. I'm currently testing it by placing a random "CpuBreakpoint ()" in
>> PartitionDxe driver.
>>
> 
> [Jiewen] Ummm.
> Based upon the fact that the code you update may be used in all those phases.
> I recommend to validate them in the final patch.

Sure.

> 
> 
>>> 3) Would you please let us know if there is any special condition
>>> validated? Such as code is in an interrupt handler, SMM initialization
>>> code, thunk code during mode switch, etc.
>>> I ask this because I have seen lots of discussion on sanity check, to
>>> avoid double exception.
>>
>> At the moment I'm only ensuring proper aligned RSP and ESP values. But we
>> still need to validate the RIP values, etc. as Andrew and Brian suggested.
> 
> [Jiewen] OK. I look forward to seeing your patch to validate more.
> Same as #2. I suggest you validate all those corner cases in the final patch.

I will for sure.

> 
>>> 4) We supported some security feature to create a hole in normal memory
>>> map, such as PageGuard, StackGuard, SMM communication protection, etc.
>>> Accessing those memory directly will cause #PF immediately.
>>> Would you please let us know how we detect that case, to avoid RIP or RSP
>>> fail to the non-present page?
>>
>> Sorry. I have no idea :-( I'd hope to get some help from you guys.
> 
> 
> [Jiewen] One possible solution is to scan page table, before access this address, to make sure you are accessing present page.
> You can also filer invalid address directly by checking CPUID physical address bit before that. The invalid address will cause #GP instead of #PF.

Oh, looks like a great solution. I'll try them out and let you know if 
it worked. Thank you!!

> 
> All I all, I would like to clarify the goal for exception lib: whatever exception condition the BIOS meets, the exception handler can always dump the *basic* exception information and halt.
> The missing of advanced symbol info is acceptable. But system crash, when we want to dump the advanced info, is not the best choice.

You're right. I'll make sure to validate it as much as possible and 
avoid such issues. Please, give me some more time because currently I've 
got pretty busy with something else.

I really appreciate your help. Thanks!

Paulo

> 
> 
> 
>>> 5) May I know why we check RIP < ImageBase? Is that legal or illegal if
>>> RIP > ImageBase+ImageSize, but RIP in another PE/COFF image?
>>
>> That check was a wrong assumption that I had in the beginning. RIP may
>> point to either lower or higher addresses where the other PE/COFF images
>> are located. Fixed it in v3.
> 
> 
> [Jiewen] Great. Thanks.
> 
> 
>> Sorry for the delay in the response. I'm only able to work on this in my
>> free time.
>>
>> Thanks!
>> Paulo
>>
>>>> +    //
>>>> +    // Check if RIP is within another PE/COFF image base address
>>>> +    //
>>>> +    if (Rip < ImageBase) {
>>>> +      //
>>>> +      // Search for the respective PE/COFF image based on RIP
>>>> +      //
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>> Paulo
>>>> Alcantara
>>>> Sent: Thursday, November 16, 2017 9:18 AM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
>>>> Subject: [edk2] [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add
>>>> stack
>>>> trace support
>>>>
>>>> This patch adds stack trace support during a X64 CPU exception.
>>>>
>>>> It will dump out back trace, stack contents as well as image module
>>>> names that were part of the call stack.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>>> ---
>>>>   UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c |
>>>> 369
>>>> +++++++++++++++++++-
>>>>   1 file changed, 367 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git
>>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
>>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
>>>> index 65f0cff680..11cd7c9e1c 100644
>>>> ---
>>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
>>>> +++
>>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
>>>> @@ -14,6 +14,11 @@
>>>>
>>>>   #include "CpuExceptionCommon.h"
>>>>
>>>> +//
>>>> +// Unknown PDB file name
>>>> +//
>>>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
>>>> *mUnknownPdbFileName = "????";
>>>> +
>>>>   /**
>>>>     Return address map of exception handler template so that C code can
>>>> generate
>>>>     exception tables.
>>>> @@ -242,6 +247,350 @@ DumpCpuContext (
>>>>       );
>>>>   }
>>>>
>>>> +/**
>>>> +  Get absolute path and file name of PDB file in PE/COFF image.
>>>> +
>>>> +  @param[in]  ImageBase            Base address of PE/COFF image.
>>>> +  @param[out] PdbAbsoluteFilePath  Absolute path of PDB file.
>>>> +  @param[out] PdbFileName          File name of PDB file.
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +GetPdbFileName (
>>>> +  IN  UINTN    ImageBase,
>>>> +  OUT CHAR8    **PdbAbsoluteFilePath,
>>>> +  OUT CHAR8    **PdbFileName
>>>> +  )
>>>> +{
>>>> +  VOID   *PdbPointer;
>>>> +  CHAR8  *Str;
>>>> +
>>>> +  //
>>>> +  // Get PDB file name from PE/COFF image
>>>> +  //
>>>> +  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)ImageBase);
>>>> +  if (PdbPointer == NULL) {
>>>> +    //
>>>> +    // No PDB file name found. Set it to an unknown file name.
>>>> +    //
>>>> +    *PdbFileName = (CHAR8 *)mUnknownPdbFileName;
>>>> +    if (PdbAbsoluteFilePath != NULL) {
>>>> +      *PdbAbsoluteFilePath = NULL;
>>>> +    }
>>>> +  } else {
>>>> +    //
>>>> +    // Get file name portion out of PDB file in PE/COFF image
>>>> +    //
>>>> +    Str = (CHAR8 *)((UINTN)PdbPointer +
>>>> +                    AsciiStrLen ((CHAR8 *)PdbPointer) - sizeof *Str);
>>>> +    for (; *Str != '/' && *Str != '\\'; Str--) {
>>>> +      ;
>>>> +    }
>>>> +
>>>> +    //
>>>> +    // Set PDB file name (also skip trailing path separator: '/' or
>>>> '\\')
>>>> +    //
>>>> +    *PdbFileName = Str + 1;
>>>> +
>>>> +    if (PdbAbsoluteFilePath != NULL) {
>>>> +      //
>>>> +      // Set absolute file path of PDB file
>>>> +      //
>>>> +      *PdbAbsoluteFilePath = PdbPointer;
>>>> +    }
>>>> +  }
>>>> +}
>>>> +
>>>> +/**
>>>> +  Dump stack contents.
>>>> +
>>>> +  @param[in]  CurrentRsp         Current stack pointer address.
>>>> +  @param[in]  UnwondStacksCount  Count of unwond stack frames.
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +DumpStackContents (
>>>> +  IN UINT64  CurrentRsp,
>>>> +  IN INTN    UnwondStacksCount
>>>> +  )
>>>> +{
>>>> +  //
>>>> +  // Check for proper stack pointer alignment
>>>> +  //
>>>> +  if (((UINTN)CurrentRsp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
>>>> +    InternalPrintMessage ("!!!! Unaligned stack pointer. !!!!\n");
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Dump out stack contents
>>>> +  //
>>>> +  InternalPrintMessage ("\nStack dump:\n");
>>>> +  while (UnwondStacksCount-- > 0) {
>>>> +    InternalPrintMessage (
>>>> +      "0x%016lx: %016lx %016lx\n",
>>>> +      CurrentRsp,
>>>> +      *(UINT64 *)CurrentRsp,
>>>> +      *(UINT64 *)((UINTN)CurrentRsp + 8)
>>>> +      );
>>>> +
>>>> +    //
>>>> +    // Point to next stack
>>>> +    //
>>>> +    CurrentRsp += CPU_STACK_ALIGNMENT;
>>>> +  }
>>>> +}
>>>> +
>>>> +/**
>>>> +  Dump all image module names from call stack.
>>>> +
>>>> +  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +DumpImageModuleNames (
>>>> +  IN EFI_SYSTEM_CONTEXT   SystemContext
>>>> +  )
>>>> +{
>>>> +  EFI_STATUS  Status;
>>>> +  UINT64      Rip;
>>>> +  UINTN       ImageBase;
>>>> +  VOID        *EntryPoint;
>>>> +  CHAR8       *PdbAbsoluteFilePath;
>>>> +  CHAR8       *PdbFileName;
>>>> +  UINT64      Rbp;
>>>> +
>>>> +  //
>>>> +  // Set current RIP address
>>>> +  //
>>>> +  Rip = SystemContext.SystemContextX64->Rip;
>>>> +
>>>> +  //
>>>> +  // Set current frame pointer address
>>>> +  //
>>>> +  Rbp = SystemContext.SystemContextX64->Rbp;
>>>> +
>>>> +  //
>>>> +  // Check for proper frame pointer alignment
>>>> +  //
>>>> +  if (((UINTN)Rbp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
>>>> +    InternalPrintMessage ("!!!! Unaligned frame pointer. !!!!\n");
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Get initial PE/COFF image base address from current RIP
>>>> +  //
>>>> +  ImageBase = PeCoffSearchImageBase (Rip);
>>>> +  if (ImageBase == 0) {
>>>> +    InternalPrintMessage ("!!!! Could not find image module names.
>>>> !!!!");
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Get initial PE/COFF image's entry point
>>>> +  //
>>>> +  Status = PeCoffLoaderGetEntryPoint ((VOID *)ImageBase, &EntryPoint);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    EntryPoint = NULL;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Get file name and absolute path of initial PDB file
>>>> +  //
>>>> +  GetPdbFileName (ImageBase, &PdbAbsoluteFilePath, &PdbFileName);
>>>> +
>>>> +  //
>>>> +  // Print out initial image module name (if any)
>>>> +  //
>>>> +  if (PdbAbsoluteFilePath != NULL) {
>>>> +    InternalPrintMessage (
>>>> +      "\n%a (ImageBase=0x%016lx, EntryPoint=0x%016lx):\n",
>>>> +      PdbFileName,
>>>> +      ImageBase,
>>>> +      (UINTN)EntryPoint
>>>> +      );
>>>> +    InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Walk through call stack and find next module names
>>>> +  //
>>>> +  for (;;) {
>>>> +    //
>>>> +    // Set RIP with return address from current stack frame
>>>> +    //
>>>> +    Rip = *(UINT64 *)((UINTN)Rbp + 8);
>>>> +
>>>> +    //
>>>> +    // If RIP is zero, then stop unwinding the stack
>>>> +    //
>>>> +    if (Rip == 0) {
>>>> +      break;
>>>> +    }
>>>> +
>>>> +    //
>>>> +    // Check if RIP is within another PE/COFF image base address
>>>> +    //
>>>> +    if (Rip < ImageBase) {
>>>> +      //
>>>> +      // Search for the respective PE/COFF image based on RIP
>>>> +      //
>>>> +      ImageBase = PeCoffSearchImageBase (Rip);
>>>> +      if (ImageBase == 0) {
>>>> +        //
>>>> +        // Stop stack trace
>>>> +        //
>>>> +        break;
>>>> +      }
>>>> +
>>>> +      //
>>>> +      // Get PE/COFF image's entry point
>>>> +      //
>>>> +      Status = PeCoffLoaderGetEntryPoint ((VOID *)ImageBase,
>>>> &EntryPoint);
>>>> +      if (EFI_ERROR (Status)) {
>>>> +        EntryPoint = NULL;
>>>> +      }
>>>> +
>>>> +      //
>>>> +      // Get file name and absolute path of PDB file
>>>> +      //
>>>> +      GetPdbFileName (ImageBase, &PdbAbsoluteFilePath,
>> &PdbFileName);
>>>> +
>>>> +      //
>>>> +      // Print out image module name (if any)
>>>> +      //
>>>> +      if (PdbAbsoluteFilePath != NULL) {
>>>> +        InternalPrintMessage (
>>>> +          "%a (ImageBase=0x%016lx, EntryPoint=0x%016lx):\n",
>>>> +          PdbFileName,
>>>> +          ImageBase,
>>>> +          (UINTN)EntryPoint
>>>> +          );
>>>> +        InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
>>>> +      }
>>>> +    }
>>>> +
>>>> +    //
>>>> +    // Unwind the stack
>>>> +    //
>>>> +    Rbp = *(UINT64 *)(UINTN)Rbp;
>>>> +  }
>>>> +}
>>>> +
>>>> +/**
>>>> +  Dump stack trace.
>>>> +
>>>> +  @param[in]  SystemContext      Pointer to EFI_SYSTEM_CONTEXT.
>>>> +  @param[out] UnwondStacksCount  Count of unwond stack frames.
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +DumpStackTrace (
>>>> +  IN  EFI_SYSTEM_CONTEXT   SystemContext,
>>>> +  OUT INTN                 *UnwondStacksCount
>>>> +  )
>>>> +{
>>>> +  UINT64  Rip;
>>>> +  UINT64  Rbp;
>>>> +  UINTN   ImageBase;
>>>> +  CHAR8   *PdbFileName;
>>>> +
>>>> +  //
>>>> +  // Set current RIP address
>>>> +  //
>>>> +  Rip = SystemContext.SystemContextX64->Rip;
>>>> +
>>>> +  //
>>>> +  // Set current frame pointer address
>>>> +  //
>>>> +  Rbp = SystemContext.SystemContextX64->Rbp;
>>>> +
>>>> +  //
>>>> +  // Get initial PE/COFF image base address from current RIP
>>>> +  //
>>>> +  ImageBase = PeCoffSearchImageBase (Rip);
>>>> +  if (ImageBase == 0) {
>>>> +    InternalPrintMessage ("!!!! Could not find backtrace information.
>>>> !!!!");
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Get PDB file name from initial PE/COFF image
>>>> +  //
>>>> +  GetPdbFileName (ImageBase, NULL, &PdbFileName);
>>>> +
>>>> +  //
>>>> +  // Initialize count of unwond stacks
>>>> +  //
>>>> +  *UnwondStacksCount = 1;
>>>> +
>>>> +  //
>>>> +  // Print out back trace
>>>> +  //
>>>> +  InternalPrintMessage ("\nCall trace:\n");
>>>> +
>>>> +  for (;;) {
>>>> +    //
>>>> +    // Print stack frame in the following format:
>>>> +    //
>>>> +    // # <RIP> @ <ImageBase>+<RelOffset> (RBP) in [<ModuleName>
>> | ????]
>>>> +    //
>>>> +    InternalPrintMessage (
>>>> +      "%d 0x%016lx @ 0x%016lx+0x%x (0x%016lx) in %a\n",
>>>> +      *UnwondStacksCount - 1,
>>>> +      Rip,
>>>> +      ImageBase,
>>>> +      Rip - ImageBase - 1,
>>>> +      Rbp,
>>>> +      PdbFileName
>>>> +      );
>>>> +
>>>> +    //
>>>> +    // Set RIP with return address from current stack frame
>>>> +    //
>>>> +    Rip = *(UINT64 *)((UINTN)Rbp + 8);
>>>> +
>>>> +    //
>>>> +    // If RIP is zero, then stop unwinding the stack
>>>> +    //
>>>> +    if (Rip == 0) {
>>>> +      break;
>>>> +    }
>>>> +
>>>> +    //
>>>> +    // Check if RIP is within another PE/COFF image base address
>>>> +    //
>>>> +    if (Rip < ImageBase) {
>>>> +      //
>>>> +      // Search for the respective PE/COFF image based on RIP
>>>> +      //
>>>> +      ImageBase = PeCoffSearchImageBase (Rip);
>>>> +      if (ImageBase == 0) {
>>>> +        //
>>>> +        // Stop stack trace
>>>> +        //
>>>> +        break;
>>>> +      }
>>>> +
>>>> +      //
>>>> +      // Get PDB file name
>>>> +      //
>>>> +      GetPdbFileName (ImageBase, NULL, &PdbFileName);
>>>> +    }
>>>> +
>>>> +    //
>>>> +    // Unwind the stack
>>>> +    //
>>>> +    Rbp = *(UINT64 *)(UINTN)Rbp;
>>>> +
>>>> +    //
>>>> +    // Increment count of unwond stacks
>>>> +    //
>>>> +    (*UnwondStacksCount)++;
>>>> +  }
>>>> +}
>>>> +
>>>>   /**
>>>>     Display CPU information.
>>>>
>>>> @@ -254,9 +603,25 @@ DumpImageAndCpuContent (
>>>>     IN EFI_SYSTEM_CONTEXT   SystemContext
>>>>     )
>>>>   {
>>>> +  INTN UnwondStacksCount;
>>>> +
>>>> +  //
>>>> +  // Dump CPU context
>>>> +  //
>>>>     DumpCpuContext (ExceptionType, SystemContext);
>>>> +
>>>> +  //
>>>> +  // Dump stack trace
>>>> +  //
>>>> +  DumpStackTrace (SystemContext, &UnwondStacksCount);
>>>> +
>>>> +  //
>>>> +  // Dump image module names
>>>> +  //
>>>> +  DumpImageModuleNames (SystemContext);
>>>> +
>>>>     //
>>>> -  // Dump module image base and module entry point by RIP
>>>> +  // Dump stack contents
>>>>     //
>>>> -  DumpModuleImageInfo (SystemContext.SystemContextX64->Rip);
>>>> +  DumpStackContents (SystemContext.SystemContextX64->Rsp,
>>>> UnwondStacksCount);
>>>>   }
>>>> --
>>>> 2.14.3
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>>
>>
>>
>> --
>> Paulo Alcantara, HP Inc.
>> Speaking for myself only.
>