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

[Paulo Alcantara]

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:pca...@zytor.com]
Sent: Friday, November 17, 2017 6:13 AM
To: Yao, Jiewen <jiewen@intel.com>
Cc: Paulo Alcantara <pca...@zytor.com>; edk2-devel@lists.01.org; Laszlo Ersek
<ler...@redhat.com>; Dong, Eric <eric.d...@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-boun...@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 <ler...@redhat.com>; Dong, Eric <eric.d...@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.d...@intel.com>
Cc: Laszlo 

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

[Yao, Jiewen]

Thanks for your reply. Comments below:

> -Original Message-
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Friday, November 17, 2017 6:13 AM
> To: Yao, Jiewen <jiewen@intel.com>
> Cc: Paulo Alcantara <pca...@zytor.com>; edk2-devel@lists.01.org; Laszlo Ersek
> <ler...@redhat.com>; Dong, Eric <eric.d...@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?



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


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

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

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.



> > 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-boun...@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 <ler...@redhat.com>; Dong, Eric <eric.d...@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 <

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

[Paulo Alcantara]

Hi Brijan,

On Thu, November 16, 2017 1:43 pm, Brian J. Johnson
>
> (These comments apply to patch 3/3 as well.)
>
> Typo:  UnwondStacksCount should be UnwoundStacksCount

Fixed in v3. Thanks!

>
> It's good to check the alignment of the stack, as you're doing.  But
> I'll reiterate that you absolutely need some better sanity checking of
> the stack and IP addresses before you dereference them.  Remember that
> they could be absolutely *anything* at the entry to this code.
> Something caused an error, and it may have been one of those registers.

Yes - you're absolutely right. I currently have no idea how to validate
those values as well as didn't have time to do so. Sorry. I hope to get to
it ASAP or someone could help me with that.

>
> Also, if the code was built with a compiler which isn't using RBP as a
> base pointer, RBP is unlikely to contain a stack address.  That will
> cause issues if you use it for unwinding without a sanity check.

Yes. I haven't figured out how to do that yet. It's also in my TODO list.

Thank you very much for the suggestions!

Paulo

>
> Thanks,
> Brian
>
>> ---
>>   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]  ImageBaseBase 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  UINTNImageBase,
>> +  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 INTNUnwondStacksCount
>> +  )
>> +{
>> +  //
>> +  // 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 

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

[Paulo Alcantara]

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.

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

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

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

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

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-boun...@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 <ler...@redhat.com>; Dong, Eric <eric.d...@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.d...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Signed-off-by: Paulo Alcantara <pca...@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]  ImageBaseBase 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  UINTNImageBase,
>> +  OUT CHAR8**PdbAbsoluteFilePath,
>> +  OUT CHAR8**PdbFileName
>> +  )
>> +{
>> +  VOID   *PdbPointer;
>> +  CHAR8  *Str;
>> +
>> +  //
>> +  // Get PDB file name from PE/COFF image
>> +  //
>> +  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)ImageBase);
>&

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

[Brian J. Johnson]

On 11/15/2017 07:18 PM, Paulo Alcantara wrote:

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 
Cc: Laszlo Ersek 
Signed-off-by: Paulo Alcantara 


(These comments apply to patch 3/3 as well.)

Typo:  UnwondStacksCount should be UnwoundStacksCount

It's good to check the alignment of the stack, as you're doing.  But 
I'll reiterate that you absolutely need some better sanity checking of 
the stack and IP addresses before you dereference them.  Remember that 
they could be absolutely *anything* at the entry to this code. 
Something caused an error, and it may have been one of those registers.


Also, if the code was built with a compiler which isn't using RBP as a 
base pointer, RBP is unlikely to contain a stack address.  That will 
cause issues if you use it for unwinding without a sanity check.


Thanks,
Brian


---
  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]  ImageBaseBase 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  UINTNImageBase,
+  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 INTNUnwondStacksCount
+  )
+{
+  //
+  // 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
+  //
+  

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

[Yao, Jiewen]

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?

2) Would you please let us know which phase is validated? Such as PEI 
PreMemory, PEI PostMemory, DXE, SMM?

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.

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?

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?
> +//
> +// 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-boun...@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 <ler...@redhat.com>; Dong, Eric <eric.d...@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.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Paulo Alcantara <pca...@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]  ImageBaseBase 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  UINTNImageBase,
> +  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
> +DumpSta

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

[Paulo Alcantara]

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 
Cc: Laszlo Ersek 
Signed-off-by: Paulo Alcantara 
---
 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]  ImageBaseBase 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  UINTNImageBase,
+  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 INTNUnwondStacksCount
+  )
+{
+  //
+  // 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, );
+  if (EFI_ERROR (Status)) {
+EntryPoint = NULL;
+  }
+
+  //
+  // Get file name and absolute path of initial PDB file
+  //
+  GetPdbFileName (ImageBase, , );
+
+  //
+  // 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