UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 484 ++++++++++++++++++-- UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 41 +- UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 445 +++++++++++++++++- UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 384 +++++++++++++++- 4 files changed, 1296 insertions(+), 58 deletions(-)
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.
(Sorry for the very long delay - I was very busy working on something
else and then went to vacations)
Jiewen,
I have tested it with VS2015x86 and the stack trace just hanged when
printing out the first EIP (that is, no frame pointer at all).
Thanks!
Paulo
Repo: https://github.com/pcacjr/edk2.git
Branch: stacktrace_v4
Cc: Rick Bramley <richard.bramley@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>
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)
Paulo Alcantara (6):
UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support
UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName()
UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support
UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory
addresses
UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers
UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 484 ++++++++++++++++++--
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 41 +-
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 445 +++++++++++++++++-
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 384 +++++++++++++++-
4 files changed, 1296 insertions(+), 58 deletions(-)
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Thanks Paulo. I tried to apply the patch series to latest code, but fail with below message. Have you rebased to latest code? =================== git.exe am --3way --ignore-space-change --keep-cr "C:\home\EdkIIGit\edk2\[edk2]-[RFC-v4-1-6]-UefiCpuPkg-CpuExceptionHandlerLib-X64-Add-stack-trace-support.patch" Applying: UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Using index info to reconstruct a base tree... M UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c Falling back to patching base and 3-way merge... Auto-merging UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c CONFLICT (content): Merge conflict in UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c Patch failed at 0001 UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". .git/rebase-apply/patch:13: trailing whitespace. // .git/rebase-apply/patch:14: trailing whitespace. // Unknown PDB file name .git/rebase-apply/patch:15: trailing whitespace. // .git/rebase-apply/patch:16: trailing whitespace. GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "????"; .git/rebase-apply/patch:17: trailing whitespace. warning: squelched 369 whitespace errors warning: 374 lines add whitespace errors. error: Failed to merge in the changes. Fail ================================================= > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Paulo > Alcantara > Sent: Friday, December 29, 2017 11:49 AM > To: edk2-devel@lists.01.org > Cc: Rick Bramley <richard.bramley@hp.com>; Dong, Eric > <eric.dong@intel.com>; Andrew Fish <afish@apple.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [edk2] [RFC v4 0/6] 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. > > (Sorry for the very long delay - I was very busy working on something > else and then went to vacations) > > Jiewen, > > I have tested it with VS2015x86 and the stack trace just hanged when > printing out the first EIP (that is, no frame pointer at all). > > Thanks! > Paulo > > Repo: https://github.com/pcacjr/edk2.git > Branch: stacktrace_v4 > > Cc: Rick Bramley <richard.bramley@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> > 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) > > Paulo Alcantara (6): > UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support > UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() > UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support > UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory > addresses > UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers > UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses > > UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c > | 484 ++++++++++++++++++-- > UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h > | 41 +- > UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | > 445 +++++++++++++++++- > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | > 384 +++++++++++++++- > 4 files changed, 1296 insertions(+), 58 deletions(-) > > -- > 2.14.3 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
"Yao, Jiewen" <jiewen.yao@intel.com> writes: Hi Jiewen, > I tried to apply the patch series to latest code, but fail with below message. > > Have you rebased to latest code? Hrm - I thought I did before sending out v4. Sorry for that. I'll make sure to rebase it against latest master before sending out v5. Thanks Paulo > > =================== > git.exe am --3way --ignore-space-change --keep-cr "C:\home\EdkIIGit\edk2\[edk2]-[RFC-v4-1-6]-UefiCpuPkg-CpuExceptionHandlerLib-X64-Add-stack-trace-support.patch" > Applying: UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support > Using index info to reconstruct a base tree... > M UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > Falling back to patching base and 3-way merge... > Auto-merging UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > CONFLICT (content): Merge conflict in UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > Patch failed at 0001 UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support > The copy of the patch that failed is found in: .git/rebase-apply/patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > .git/rebase-apply/patch:13: trailing whitespace. > // > .git/rebase-apply/patch:14: trailing whitespace. > // Unknown PDB file name > .git/rebase-apply/patch:15: trailing whitespace. > // > .git/rebase-apply/patch:16: trailing whitespace. > GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "????"; > .git/rebase-apply/patch:17: trailing whitespace. > > warning: squelched 369 whitespace errors > warning: 374 lines add whitespace errors. > error: Failed to merge in the changes. > > Fail > ================================================= > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Paulo >> Alcantara >> Sent: Friday, December 29, 2017 11:49 AM >> To: edk2-devel@lists.01.org >> Cc: Rick Bramley <richard.bramley@hp.com>; Dong, Eric >> <eric.dong@intel.com>; Andrew Fish <afish@apple.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com> >> Subject: [edk2] [RFC v4 0/6] 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. >> >> (Sorry for the very long delay - I was very busy working on something >> else and then went to vacations) >> >> Jiewen, >> >> I have tested it with VS2015x86 and the stack trace just hanged when >> printing out the first EIP (that is, no frame pointer at all). >> >> Thanks! >> Paulo >> >> Repo: https://github.com/pcacjr/edk2.git >> Branch: stacktrace_v4 >> >> Cc: Rick Bramley <richard.bramley@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> >> 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) >> >> Paulo Alcantara (6): >> UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support >> UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() >> UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support >> UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory >> addresses >> UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers >> UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses >> >> UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c >> | 484 ++++++++++++++++++-- >> UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h >> | 41 +- >> UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | >> 445 +++++++++++++++++- >> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | >> 384 +++++++++++++++- >> 4 files changed, 1296 insertions(+), 58 deletions(-) >> >> -- >> 2.14.3 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Paulo
I have some more thought on #3 for your consideration.
Given the situation that we are doing heap guard feature, a *valid* EIP address might not be enough to guarantee the address is inside of an *executable* page.
I am thinking if we need check the non-executable bit (BIT63) as well for DumpImageModuleNames(). (No need for DumpStacktrace()).
We can add IsLogicalAddressExecutable() on top of IsLogicalAddressValid().
Also I do not object the idea to add check inside of PeCoffSearchImageBase().
I am thinking in another way - we can let *caller* to input a validation function for the PeCoffLib, instead of let PeCoffLib depend on another library.
For example:
PeCoffSearchImageBaseEx (Address, ValidateAddressFunc)
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,
> Jiewen
> Sent: Wednesday, January 17, 2018 8:57 PM
> To: Paulo Alcantara <paulo@paulo.ac>; 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>; Diego Medaglia <diego.meaglia@hp.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: Re: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling
>
> 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\Arch
> ExceptionHandler.c(547): ImageBase = PeCoffSearchImageBase (Eip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Eip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(682): ImageBase = PeCoffSearchImageBase (Eip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(741): ImageBase = PeCoffSearchImageBase (Eip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(540): ImageBase = PeCoffSearchImageBase (Rip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Rip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(710): ImageBase = PeCoffSearchImageBase (Rip);
>
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Jiewen,
"Yao, Jiewen" <jiewen.yao@intel.com> writes:
> I have some more thought on #3 for your consideration.
>
> Given the situation that we are doing heap guard feature, a *valid*
> EIP address might not be enough to guarantee the address is inside of
> an *executable* page.
OK.
>
> I am thinking if we need check the non-executable bit (BIT63) as well for DumpImageModuleNames(). (No need for DumpStacktrace()).
> We can add IsLogicalAddressExecutable() on top of
> IsLogicalAddressValid().
OK. I can do that.
> Also I do not object the idea to add check inside of PeCoffSearchImageBase().
> I am thinking in another way - we can let *caller* to input a validation function for the PeCoffLib, instead of let PeCoffLib depend on another library.
>
> For example:
> PeCoffSearchImageBaseEx (Address, ValidateAddressFunc)
Looks good to me! Whether or not we're making it into a external library
in the future, we should at least get it working and well tested for the
stack trace support.
Thanks!
Paulo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,
>> Jiewen
>> Sent: Wednesday, January 17, 2018 8:57 PM
>> To: Paulo Alcantara <paulo@paulo.ac>; 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>; Diego Medaglia <diego.meaglia@hp.com>; Laszlo Ersek
>> <lersek@redhat.com>
>> Subject: Re: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling
>>
>> 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\Arch
>> ExceptionHandler.c(547): ImageBase = PeCoffSearchImageBase (Eip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
>> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Eip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
>> ExceptionHandler.c(682): ImageBase = PeCoffSearchImageBase (Eip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
>> ExceptionHandler.c(741): ImageBase = PeCoffSearchImageBase (Eip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
>> ExceptionHandler.c(540): ImageBase = PeCoffSearchImageBase (Rip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
>> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Rip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
>> ExceptionHandler.c(710): ImageBase = PeCoffSearchImageBase (Rip);
>>
>> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
>> ExceptionHandler.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
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
"Yao, Jiewen" <jiewen.yao@intel.com> writes:
Hi Jiewen,
Thank you very much for teh review! My comments below:
> 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.
OK - didn't know about it. I'll remove that check.
> 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;
> }
Yes - I think it's possible and a good improvement. I'll do it.
> 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);
>
When I saw the #PF when testing stack trace in SMM, I was running out of
time and I just saved the log file with the trace. I'm attaching the
log for you, but I'm still going to look into that issue when time
permits.
I haven't looked on how the SMI entry is set up, but don't you think
that we should do something similiar like in DXE and PEI phases with
"push $0" and help the debugger or tracer know when stop unwinding the
stack -- in DEBUG mode, at least? Of course, if that's really possible.
> 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;
> }
Possibly. I'll add some debug and see what the Eip looks like at this
point.
> //
> // 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);
I think so. There is no guarantee that the return address (Eip) will be
valid even if we're handling a valid frame pointer. It might get
corrupted at some point. Thanks for pointing it out! I'll validate
them.
> 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.
Sure. I will.
Thanks for the comments! I learnt a lot of with them :-)
Paulo
>
>> -----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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Paulo Alcantara <paulo@paulo.ac> writes: >> 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); >> > > When I saw the #PF when testing stack trace in SMM, I was running out of > time and I just saved the log file with the trace. I'm attaching the > log for you, but I'm still going to look into that issue when time > permits. Forgot to attach the log file. Done. :-) !!!! X64 Exception Type - 03(#BP - Breakpoint) CPU Apic ID - 00000000 !!!! RIP - 000000007FF48151, CS - 0000000000000038, RFLAGS - 0000000000000046 RAX - 0000000000000000, RCX - 000000007FEBB020, RDX - 0000000000040000 RBX - 0000000000000000, RSP - 000000007FF7B760, RBP - 000000007FF7B760 RSI - 000000007FF44018, RDI - 000000007FEBB018 R8 - 00000000000000FF, R9 - 0000000000048400, R10 - 00000000000000C9 R11 - 0000000000000069, R12 - 0000000000000000, R13 - 0000000000000000 R14 - 0000000000000000, R15 - 0000000000000000 DS - 0000000000000020, ES - 0000000000000020, FS - 0000000000000020 GS - 0000000000000020, SS - 0000000000000020 CR0 - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FF6C000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000007FF6B000 000000000000004F, LDTR - 0000000000000000 IDTR - 000000007FF75000 00000000000001FF, TR - 0000000000000040 FXSAVE_STATE - 000000007FF7B3C0 Call trace: 0 0x000000007FF48151 @ 0x000000007FF46000+0x2151 (0x000000007FF7B760) in VariableSmm.dll 1 0x000000007FF54E35 @ 0x000000007FF46000+0xEE35 (0x000000007FF7B7D0) in VariableSmm.dll 2 0x000000007FF5659E @ 0x000000007FF46000+0x1059E (0x000000007FF7B800) in VariableSmm.dll 3 0x000000007FF4BCFE @ 0x000000007FF46000+0x5CFE (0x000000007FF7B840) in VariableSmm.dll 4 0x000000007FFE9BFA @ 0x000000007FFDB000+0xEBFA (0x000000007FF7B8A0) in PiSmmCore.dll 5 0x000000007FFEAAC4 @ 0x000000007FFDB000+0xFAC4 (0x000000007FF7B9C0) in PiSmmCore.dll 6 0x000000007FFEADD1 @ 0x000000007FFDB000+0xFDD1 (0x000000007FF7BA20) in PiSmmCore.dll 7 0x000000007FFE43F8 @ 0x000000007FFDB000+0x93F8 (0x000000007FF7BA90) in PiSmmCore.dll 8 0x000000007FFA8B9A @ 0x000000007FF9C000+0xCB9A (0x000000007FF7BD50) in PiSmmCpuDxeSmm.dll 9 0x000000007FFA9E92 @ 0x000000007FF9C000+0xDE92 (0x000000007FF7BDC0) in PiSmmCpuDxeSmm.dll !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 S:0 RIP - 000000007FFA1BBE, CS - 0000000000000038, RFLAGS - 0000000000010006 RAX - 000000007FF89FFC, RCX - 000000007FF8E149, RDX - FFFFFFFFFFFFFFFF RBX - 0000000000000000, RSP - 000000007FF7B1A0, RBP - 000000007FF7B1E0 RSI - 000000007FFA9E92, RDI - 000000007FF9C000 R8 - 0000000000000001, R9 - 0000000000000000, R10 - 0000000000000000 R11 - 0000000000000069, R12 - 0000000000000000, R13 - 0000000000000000 R14 - 0000000000000000, R15 - 0000000000000000 DS - 0000000000000020, ES - 0000000000000020, FS - 0000000000000020 GS - 0000000000000020, SS - 0000000000000020 CR0 - 0000000080010033, CR2 - 000000007FF89FFC, CR3 - 000000007FF6C000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000007FF6B000 000000000000004F, LDTR - 0000000000000000 IDTR - 000000007FF75000 00000000000001FF, TR - 0000000000000040 FXSAVE_STATE - 000000007FF76C60 Paulo >>> -----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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Jiewen,
On 1/17/2018 10:57 AM, Yao, Jiewen wrote:
> 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(). :-)
I was able to work a little bit on stack trace support yesterday, and
here's what I came up with by following your suggestions:
1. Centralized the calculation of initial EIP in DumpImageAndCpuContent()
2. LinearAddressStart 0 is now a valid case
3. Validate all return addresses (EIP/RIP) from every frame pointer.
Additionally, I run a few quick tests by generating exceptions from DXE
and SMM and here's my result:
OVMF X64: DXE & SMM worked.
OVMF X64 IA32: DXE & SMM worked.
OVMF IA32: DXE worked, but SMM not.
SMM was failing in both X64 and IA32 - however when I started validating
the return addresses from the frame pointers and, when we find an
invalid return address (e.g. last valid frame), we stop the trace. With
that, SMM is now working in X64.
Since the page fault is only occurring in IA32 with no paging enabled
(default case), I suspect that when we don't have paging, we are unable
to successfully validate the memory address when it's i.e. outside SMRAM
- that is, we don't know when to stop unwinding the stack.
Any ideas on what might be causing that?
Other than that, I'll try to do some more tests this weekend and come
back with the results. (That's why I didn't send out the v6 yet)
Repo: https://git.paulo.ac/pub/edk2.git
Branch: stacktrace_v6
Thanks
Paulo
>
>
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Thanks Paulo.
=======================
OVMF IA32: DXE worked, but SMM not.
Since the page fault is only occurring in IA32 with no paging enabled
(default case), I suspect that when we don't have paging, we are unable
to successfully validate the memory address when it's i.e. outside SMRAM
- that is, we don't know when to stop unwinding the stack.
=======================
For IA32 SMM, I am a little confused.
We unconditionally setup page table for SMM, no matter it is IA32 or X64.
If you find a SMM driver running in a page-disable environment, it means, the SMM CORE load the driver in SMRAM, but the SMM CPU driver has not rebased the CPU yet.
SMM driver is still using the PageTable/GDT/IDT setup by DXE CPU driver, not SMM CPU driver.
Would you please double confirm what you have observed?
You can just check the boot log file to see if PiSmmCpu driver has run or not.
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Monday, January 29, 2018 9:38 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; 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>; Diego Medaglia <diego.meaglia@hp.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: Re: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling
>
> Hi Jiewen,
>
> On 1/17/2018 10:57 AM, Yao, Jiewen wrote:
> > 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\Arch
> ExceptionHandler.c(547): ImageBase = PeCoffSearchImageBase (Eip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Eip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(682): ImageBase = PeCoffSearchImageBase (Eip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch
> ExceptionHandler.c(741): ImageBase = PeCoffSearchImageBase (Eip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(540): ImageBase = PeCoffSearchImageBase (Rip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(613): ImageBase = PeCoffSearchImageBase (Rip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.c(710): ImageBase = PeCoffSearchImageBase (Rip);
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch
> ExceptionHandler.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(). :-)
>
> I was able to work a little bit on stack trace support yesterday, and
> here's what I came up with by following your suggestions:
>
> 1. Centralized the calculation of initial EIP in DumpImageAndCpuContent()
>
> 2. LinearAddressStart 0 is now a valid case
>
> 3. Validate all return addresses (EIP/RIP) from every frame pointer.
>
> Additionally, I run a few quick tests by generating exceptions from DXE
> and SMM and here's my result:
>
> OVMF X64: DXE & SMM worked.
> OVMF X64 IA32: DXE & SMM worked.
> OVMF IA32: DXE worked, but SMM not.
>
> SMM was failing in both X64 and IA32 - however when I started validating
> the return addresses from the frame pointers and, when we find an
> invalid return address (e.g. last valid frame), we stop the trace. With
> that, SMM is now working in X64.
>
> Since the page fault is only occurring in IA32 with no paging enabled
> (default case), I suspect that when we don't have paging, we are unable
> to successfully validate the memory address when it's i.e. outside SMRAM
> - that is, we don't know when to stop unwinding the stack.
>
> Any ideas on what might be causing that?
>
> Other than that, I'll try to do some more tests this weekend and come
> back with the results. (That's why I didn't send out the v6 yet)
>
> Repo: https://git.paulo.ac/pub/edk2.git
> Branch: stacktrace_v6
>
> Thanks
> Paulo
>
> >
> >
> > 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
"Yao, Jiewen" <jiewen.yao@intel.com> writes: Hi Jiewen, > ======================= > OVMF IA32: DXE worked, but SMM not. > > Since the page fault is only occurring in IA32 with no paging enabled > (default case), I suspect that when we don't have paging, we are unable > to successfully validate the memory address when it's i.e. outside SMRAM > - that is, we don't know when to stop unwinding the stack. > ======================= > > For IA32 SMM, I am a little confused. > We unconditionally setup page table for SMM, no matter it is IA32 or X64. > > If you find a SMM driver running in a page-disable environment, it means, the SMM CORE load the driver in SMRAM, but the SMM CPU driver has not rebased the CPU yet. > SMM driver is still using the PageTable/GDT/IDT setup by DXE CPU > driver, not SMM CPU driver. OK - thanks for clarifying that. > Would you please double confirm what you have observed? > > You can just check the boot log file to see if PiSmmCpu driver has run > or not. Sure. I will do it and then get back to you once I got the results. Thanks Paulo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 401 +++++++++++++++++++-
1 file changed, 393 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 56180f4c17..4db9f6465e 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.
@@ -399,20 +404,281 @@ DumpCpuContext (
}
/**
- Display CPU information.
+ Get absolute path and file name of PDB file in PE/COFF image.
- @param ExceptionType Exception type.
- @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+ @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
-DumpImageAndCpuContent (
+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] UnwoundStacksCount Count of unwound stack frames.
+**/
+STATIC
+VOID
+DumpStackContents (
+ IN UINT64 CurrentRsp,
+ IN INTN UnwoundStacksCount
+ )
+{
+ //
+ // 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 (UnwoundStacksCount-- > 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] ExceptionType Exception type.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+**/
+STATIC
+VOID
+DumpImageModuleNames (
IN EFI_EXCEPTION_TYPE ExceptionType,
IN EFI_SYSTEM_CONTEXT SystemContext
)
{
- DumpCpuContext (ExceptionType, SystemContext);
+ EFI_STATUS Status;
+ UINT64 Rip;
+ UINTN ImageBase;
+ VOID *EntryPoint;
+ CHAR8 *PdbAbsoluteFilePath;
+ CHAR8 *PdbFileName;
+ UINT64 Rbp;
+ UINTN LastImageBase;
+
+ //
+ // Set current RIP address
+ //
+ if ((ExceptionType == EXCEPT_IA32_PAGE_FAULT) &&
+ ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0)) {
+ //
+ // The RIP in SystemContext could not be used
+ // if it is page fault with I/D set.
+ //
+ Rip = *(UINT64 *)(UINTN)SystemContext.SystemContextX64->Rsp;
+ } else {
+ 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;
+ }
+
+ //
+ // Set last PE/COFF image base address
+ //
+ LastImageBase = ImageBase;
+
+ //
+ // 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;
+ }
+
+ //
+ // Search for the respective PE/COFF image based on RIP
+ //
+ ImageBase = PeCoffSearchImageBase (Rip);
+ if (ImageBase == 0) {
+ //
+ // Stop stack trace
+ //
+ break;
+ }
+
+ //
+ // If RIP points to another PE/COFF image, then find its respective PDB file
+ // name.
+ //
+ if (LastImageBase != ImageBase) {
+ //
+ // 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);
+ }
+
+ //
+ // Save last PE/COFF image base address
+ //
+ LastImageBase = ImageBase;
+ }
+
+ //
+ // Unwind the stack
+ //
+ Rbp = *(UINT64 *)(UINTN)Rbp;
+ }
+}
+
+/**
+ Dump stack trace.
+
+ @param[in] ExceptionType Exception type.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+ @param[out] UnwoundStacksCount Count of unwound stack frames.
+**/
+STATIC
+VOID
+DumpStacktrace (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext,
+ OUT INTN *UnwoundStacksCount
+ )
+{
+ UINT64 Rip;
+ UINT64 Rbp;
+ UINTN ImageBase;
+ CHAR8 *PdbFileName;
+
//
- // Dump module image base and module entry point by RIP
+ // Set current RIP address
//
if ((ExceptionType == EXCEPT_IA32_PAGE_FAULT) &&
((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0)) {
@@ -420,8 +686,127 @@ DumpImageAndCpuContent (
// The RIP in SystemContext could not be used
// if it is page fault with I/D set.
//
- DumpModuleImageInfo ((*(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp));
+ Rip = *(UINT64 *)(UINTN)SystemContext.SystemContextX64->Rsp;
} else {
- DumpModuleImageInfo (SystemContext.SystemContextX64->Rip);
+ 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 unwound stacks
+ //
+ *UnwoundStacksCount = 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",
+ *UnwoundStacksCount - 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;
+ }
+
+ //
+ // 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 unwound stacks
+ //
+ (*UnwoundStacksCount)++;
}
}
+
+/**
+ Display CPU information.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+**/
+VOID
+DumpImageAndCpuContent (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ )
+{
+ INTN UnwoundStacksCount;
+
+ //
+ // Dump CPU context
+ //
+ DumpCpuContext (ExceptionType, SystemContext);
+
+ //
+ // Dump stack trace
+ //
+ DumpStacktrace (ExceptionType, SystemContext, &UnwoundStacksCount);
+
+ //
+ // Dump image module names
+ //
+ DumpImageModuleNames (ExceptionType, SystemContext);
+
+ //
+ // Dump stack contents
+ //
+ DumpStackContents (SystemContext.SystemContextX64->Rsp, UnwoundStacksCount);
+}
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
This function will be used by both IA32 and X64 exception handling in
order to print out image module names during stack unwinding.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 60 +++++++++++++++++++-
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 14 +++++
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 59 -------------------
3 files changed, 73 insertions(+), 60 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
index 01b0610364..d9abbd772d 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -54,6 +54,11 @@ CONST CHAR8 *mExceptionNameStr[] = {
#define EXCEPTION_KNOWN_NAME_NUM (sizeof (mExceptionNameStr) / sizeof (CHAR8 *))
+//
+// Unknown PDB file name
+//
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "????";
+
/**
Get ASCII format string exception name by exception type.
@@ -177,4 +182,57 @@ ReadAndVerifyVectorInfo (
VectorInfo ++;
}
return EFI_SUCCESS;
-}
\ No newline at end of file
+}
+
+/**
+ 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.
+**/
+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;
+ }
+ }
+}
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
index e10d9379d5..64c7094513 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -327,5 +327,19 @@ AsmGetTssTemplateMap (
OUT EXCEPTION_HANDLER_TEMPLATE_MAP *AddressMap
);
+/**
+ 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.
+**/
+VOID
+GetPdbFileName (
+ IN UINTN ImageBase,
+ OUT CHAR8 **PdbAbsoluteFilePath,
+ OUT CHAR8 **PdbFileName
+ );
+
#endif
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 4db9f6465e..523dce95c9 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -14,11 +14,6 @@
#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.
@@ -403,60 +398,6 @@ 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.
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
This patch adds stack trace support during a IA32 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 <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 42 ---
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 11 -
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 335 +++++++++++++++++++-
3 files changed, 327 insertions(+), 61 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
index d9abbd772d..66892320c8 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -109,48 +109,6 @@ InternalPrintMessage (
SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
}
-/**
- Find and display image base address and return image base and its entry point.
-
- @param CurrentEip Current instruction pointer.
-
-**/
-VOID
-DumpModuleImageInfo (
- IN UINTN CurrentEip
- )
-{
- EFI_STATUS Status;
- UINTN Pe32Data;
- VOID *PdbPointer;
- VOID *EntryPoint;
-
- Pe32Data = PeCoffSearchImageBase (CurrentEip);
- if (Pe32Data == 0) {
- InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
- } else {
- //
- // Find Image Base entry point
- //
- Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
- if (EFI_ERROR (Status)) {
- EntryPoint = NULL;
- }
- InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
- PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
- if (PdbPointer != NULL) {
- InternalPrintMessage ("%a", PdbPointer);
- } else {
- InternalPrintMessage ("(No PDB) " );
- }
- InternalPrintMessage (
- " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
- (VOID *) Pe32Data,
- EntryPoint
- );
- }
-}
-
/**
Read and save reserved vector information
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
index 64c7094513..ec46c2d9d3 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -130,17 +130,6 @@ InternalPrintMessage (
...
);
-/**
- Find and display image base address and return image base and its entry point.
-
- @param CurrentEip Current instruction pointer.
-
-**/
-VOID
-DumpModuleImageInfo (
- IN UINTN CurrentEip
- );
-
/**
Display CPU information.
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 04f2ab593c..c5d6ea0939 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -399,20 +399,156 @@ DumpCpuContext (
}
/**
- Display CPU information.
+ Dump stack trace.
- @param ExceptionType Exception type.
- @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+ @param[in] ExceptionType Exception type.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+ @param[out] UnwoundStacksCount Count of unwound stack frames.
**/
+STATIC
VOID
-DumpImageAndCpuContent (
+DumpStacktrace (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext,
+ OUT INTN *UnwoundStacksCount
+ )
+{
+ UINT32 Eip;
+ UINT32 Ebp;
+ UINTN ImageBase;
+ CHAR8 *PdbFileName;
+
+ //
+ // 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;
+ }
+
+ //
+ // Set current frame pointer address
+ //
+ Ebp = SystemContext.SystemContextIa32->Ebp;
+
+ //
+ // Check for proper frame pointer alignment
+ //
+ if (((UINTN)Ebp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
+ InternalPrintMessage ("!!!! Unaligned frame pointer. !!!!\n");
+ return;
+ }
+
+ //
+ // Get initial PE/COFF image base address from current EIP
+ //
+ ImageBase = PeCoffSearchImageBase (Eip);
+ 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 unwound stacks
+ //
+ *UnwoundStacksCount = 1;
+
+ //
+ // Print out back trace
+ //
+ InternalPrintMessage ("\nCall trace:\n");
+
+ for (;;) {
+ //
+ // Print stack frame in the following format:
+ //
+ // # <EIP> @ <ImageBase>+<RelOffset> (EBP) in [<ModuleName> | ????]
+ //
+ InternalPrintMessage (
+ "%d 0x%08x @ 0x%08x+0x%x (0x%08x) in %a\n",
+ *UnwoundStacksCount - 1,
+ Eip,
+ ImageBase,
+ Eip - ImageBase - 1,
+ Ebp,
+ PdbFileName
+ );
+
+ //
+ // 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 (ImageBase == 0) {
+ //
+ // Stop stack trace
+ //
+ break;
+ }
+
+ //
+ // Get PDB file name
+ //
+ GetPdbFileName (ImageBase, NULL, &PdbFileName);
+
+ //
+ // Unwind the stack
+ //
+ Ebp = *(UINT32 *)(UINTN)Ebp;
+
+ //
+ // Increment count of unwound stacks
+ //
+ (*UnwoundStacksCount)++;
+ }
+}
+
+/**
+ Dump all image module names from call stack.
+
+ @param[in] ExceptionType Exception type.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+**/
+STATIC
+VOID
+DumpImageModuleNames (
IN EFI_EXCEPTION_TYPE ExceptionType,
IN EFI_SYSTEM_CONTEXT SystemContext
)
{
- DumpCpuContext (ExceptionType, SystemContext);
+ EFI_STATUS Status;
+ UINT32 Eip;
+ UINT32 Ebp;
+ UINTN ImageBase;
+ VOID *EntryPoint;
+ CHAR8 *PdbAbsoluteFilePath;
+ CHAR8 *PdbFileName;
+ UINTN LastImageBase;
+
//
- // Dump module image base and module entry point by EIP
+ // Set current EIP address
//
if ((ExceptionType == EXCEPT_IA32_PAGE_FAULT) &&
((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0)) {
@@ -420,8 +556,191 @@ DumpImageAndCpuContent (
// The EIP in SystemContext could not be used
// if it is page fault with I/D set.
//
- DumpModuleImageInfo ((*(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp));
+ Eip = *(UINT32 *)(UINTN)SystemContext.SystemContextIa32->Esp;
} else {
- DumpModuleImageInfo (SystemContext.SystemContextIa32->Eip);
+ Eip = SystemContext.SystemContextIa32->Eip;
+ }
+
+ //
+ // Set current frame pointer address
+ //
+ Ebp = SystemContext.SystemContextIa32->Ebp;
+
+ //
+ // Get initial PE/COFF image base address from current EIP
+ //
+ ImageBase = PeCoffSearchImageBase (Eip);
+ if (ImageBase == 0) {
+ InternalPrintMessage ("!!!! Could not find image module names. !!!!");
+ return;
}
+
+ //
+ // Set last PE/COFF image base address
+ //
+ LastImageBase = ImageBase;
+
+ //
+ // 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%08x, EntryPoint=0x%08x):\n",
+ PdbFileName,
+ ImageBase,
+ (UINTN)EntryPoint
+ );
+ InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
+ }
+
+ //
+ // Walk through call stack and find next module names
+ //
+ for (;;) {
+ //
+ // Set EIP with return address from current stack frame
+ //
+ Eip = *(UINT32 *)((UINTN)Ebp + 4);
+
+ //
+ // Search for the respective PE/COFF image based on Eip
+ //
+ ImageBase = PeCoffSearchImageBase (Eip);
+ if (ImageBase == 0) {
+ //
+ // Stop stack trace
+ //
+ break;
+ }
+
+ //
+ // If EIP points to another PE/COFF image, then find its respective PDB file
+ // name.
+ //
+ if (LastImageBase != ImageBase) {
+ //
+ // 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%08x, EntryPoint=0x%08x):\n",
+ PdbFileName,
+ ImageBase,
+ (UINTN)EntryPoint
+ );
+ InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
+ }
+
+ //
+ // Save last PE/COFF image base address
+ //
+ LastImageBase = ImageBase;
+ }
+
+ //
+ // Unwind the stack
+ //
+ Ebp = *(UINT32 *)(UINTN)Ebp;
+ }
+}
+
+/**
+ Dump stack contents.
+
+ @param[in] CurrentEsp Current stack pointer address.
+ @param[in] UnwoundStacksCount Count of unwound stack frames.
+**/
+STATIC
+VOID
+DumpStackContents (
+ IN UINT32 CurrentEsp,
+ IN INTN UnwoundStacksCount
+ )
+{
+ //
+ // Check for proper stack alignment
+ //
+ if (((UINTN)CurrentEsp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
+ InternalPrintMessage ("!!!! Unaligned stack pointer. !!!!\n");
+ return;
+ }
+
+ //
+ // Dump out stack contents
+ //
+ InternalPrintMessage ("\nStack dump:\n");
+ while (UnwoundStacksCount-- > 0) {
+ InternalPrintMessage (
+ "0x%08x: %08x %08x\n",
+ CurrentEsp,
+ *(UINT32 *)CurrentEsp,
+ *(UINT32 *)((UINTN)CurrentEsp + 4)
+ );
+
+ //
+ // Point to next stack
+ //
+ CurrentEsp += CPU_STACK_ALIGNMENT;
+ }
+}
+
+/**
+ Display CPU information.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+**/
+VOID
+DumpImageAndCpuContent (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ )
+{
+ INTN UnwoundStacksCount;
+
+ //
+ // Dump CPU context
+ //
+ DumpCpuContext (ExceptionType, SystemContext);
+
+ //
+ // Dump stack trace
+ //
+ DumpStacktrace (ExceptionType, SystemContext, &UnwoundStacksCount);
+
+ //
+ // Dump image module names
+ //
+ DumpImageModuleNames (ExceptionType, SystemContext);
+
+ //
+ // Dump stack contents
+ //
+ DumpStackContents (SystemContext.SystemContextIa32->Esp, UnwoundStacksCount);
}
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Introduce IsLinearAddressValid() function that will be used for
validating memory addresses that would get dereferenced during stack
traces in IA32 and X64 CPU exceptions.
Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Requested-by: Brian Johnson <brian.johnson@hpe.com>
Requested-by: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 395 ++++++++++++++++++++
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 16 +
2 files changed, 411 insertions(+)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
index 66892320c8..7ac13640de 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -14,6 +14,8 @@
#include "CpuExceptionCommon.h"
+#include <Register/Msr.h>
+
//
// Error code flag indicating whether or not an error code will be
// pushed on the stack if an exception occurs.
@@ -59,6 +61,24 @@ CONST CHAR8 *mExceptionNameStr[] = {
//
GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "????";
+//
+// IA32 virtual memory bit definitions
+//
+#define IA32_PG_P BIT0
+#define IA32_PG_PS BIT7
+
+//
+// IA32 control register bit definitions
+//
+#define IA32_CR0_PG BIT31
+#define IA32_CR4_PAE BIT5
+#define IA32_CR0_PE BIT0
+
+//
+// IA32 CPUID 01h EDX bit definitions
+//
+#define IA32_CPUID1_EDX_PAE BIT6
+
/**
Get ASCII format string exception name by exception type.
@@ -194,3 +214,378 @@ GetPdbFileName (
}
}
}
+
+/**
+ Check if a linear address is valid by walking the page tables in 4-level
+ paging mode.
+
+ @param[in] Cr3 CR3 control register.
+ @param[in] MaxPhyAddrBits MAXPHYADDR bits.
+ @param[in] LinearAddress Linear address to be checked.
+**/
+STATIC
+BOOLEAN
+Do4LevelPagingModeCheck (
+ IN UINTN Cr3,
+ IN UINT8 MaxPhyAddrBits,
+ IN UINTN LinearAddress
+ )
+{
+ UINT64 PhysicalAddressMask;
+ UINTN IndexMask;
+ UINTN Index;
+ UINT64 *Pml4Table;
+ UINT64 *TableEntry;
+ UINT64 *PageDirPtrTable;
+ UINT64 *PageDirTable;
+ UINT64 *PageTable;
+
+ //
+ // In 4-level paging mode, linear addresses are 48 bits wide
+ //
+ if ((UINT64)LinearAddress > 0xFFFFFFFFFFFFULL) {
+ return FALSE;
+ }
+
+ //
+ // Calculate physical address mask (bits M-1:12)
+ //
+ PhysicalAddressMask = (LShiftU64 (1, MaxPhyAddrBits) - 1) & ~0xFFF;
+ //
+ // 9 bits for masking page table indexes out of linear addresses
+ //
+ IndexMask = 0x1FF;
+
+ //
+ // Calculate physical address of PML4 table and index of PML4E
+ //
+ Pml4Table = (UINT64 *)(UINTN)((UINT64)Cr3 & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 39) & IndexMask);
+
+ //
+ // Select PML4E
+ //
+ TableEntry = &Pml4Table[Index];
+
+ //
+ // Check if a PDPTE is present
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ //
+ // Calculate physical address of page-directory-pointer table and index of
+ // PDPTE.
+ //
+ PageDirPtrTable = (UINT64 *)(UINTN)(*TableEntry & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 30) & IndexMask);
+
+ //
+ // Select PDPTE
+ //
+ TableEntry = &PageDirPtrTable[Index];
+
+ //
+ // Check whether a PDPTE or 1GiB page entry is present
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ //
+ // Check if PDPTE maps an 1GiB page
+ //
+ if ((*TableEntry & IA32_PG_PS) != 0) {
+ return TRUE;
+ }
+
+ //
+ // Calculate physical address of page directory table and index of PDE
+ //
+ PageDirTable = (UINT64 *)(UINTN)(*TableEntry & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 21) & IndexMask);
+
+ //
+ // Select PDE
+ //
+ TableEntry = &PageDirTable[Index];
+
+ //
+ // Check whether a PDE or a 2MiB page entry is present
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ //
+ // Check if PDE maps a 2MiB page
+ //
+ if ((*TableEntry & IA32_PG_PS) != 0) {
+ return TRUE;
+ }
+
+ //
+ // Calculate physical address of page table and index of PTE
+ //
+ PageTable = (UINT64 *)(UINTN)(*TableEntry & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 12) & IndexMask);
+
+ //
+ // Select PTE
+ //
+ TableEntry = &PageTable[Index];
+
+ //
+ // Check if PTE maps a 4KiB page
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Check if a linear address is valid by walking the page tables in 32-bit paging
+ mode.
+
+ NOTE: Current UEFI implementations do not support IA32 non-PAE paging mode.
+
+ @param[in] Cr3 CR3 control register.
+ @param[in] Cr4 CR4 control register.
+ @param[in] LinearAddress Linear address to be checked.
+**/
+STATIC
+BOOLEAN
+Do32BitPagingModeCheck (
+ IN UINTN Cr3,
+ IN UINTN Cr4,
+ IN UINTN LinearAddress
+ )
+{
+ InternalPrintMessage ("!!!! Unsupported IA32 non-PAE paging mode !!!!\n");
+ return FALSE;
+}
+
+/**
+ Check if a linear address is valid by walking the page tables in PAE paging
+ mode.
+
+ @param[in] Cr3 CR3 control register.
+ @param[in] MaxPhyAddrBits MAXPHYADDR bits.
+ @param[in] LinearAddress Linear address to be checked.
+**/
+STATIC
+BOOLEAN
+DoPAEPagingModeCheck (
+ IN UINTN Cr3,
+ IN UINT8 MaxPhyAddrBits,
+ IN UINTN LinearAddress
+ )
+{
+ UINT64 *PageDirPtrTable;
+ UINTN Index;
+ UINT64 *PageDirTable;
+ UINT64 PhysicalAddressMask;
+ UINTN IndexMask;
+ UINT64 *TableEntry;
+ UINT64 *PageTable;
+
+ //
+ // In 32-bit PAE paging mode, linear addresses are 32 bits wide
+ //
+ if (LinearAddress > 0xFFFFFFFF) {
+ return FALSE;
+ }
+
+ //
+ // Calculate physical address of page-directory-pointer table and index of
+ // PDPTE register.
+ //
+ PageDirPtrTable = (UINT64 *)(UINTN)(Cr3 & ~0x1F);
+ Index = (UINTN)((UINT32)LinearAddress >> 30);
+
+ //
+ // Select PDPTE register
+ //
+ TableEntry = &PageDirPtrTable[Index];
+
+ //
+ // Check if PDE is present
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ //
+ // Calculate physical address mask (bits M-1:12)
+ //
+ PhysicalAddressMask = (LShiftU64 (1, MaxPhyAddrBits) - 1) & ~0xFFF;
+ //
+ // 9 bits for masking page table indexes out of linear addresses
+ //
+ IndexMask = 0x1FF;
+
+ //
+ // Calculate physical address of page directory table and index of PDE
+ //
+ PageDirTable = (UINT64 *)(UINTN)(*TableEntry & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 21) & IndexMask);
+
+ //
+ // Select PDE
+ //
+ TableEntry = &PageDirTable[Index];
+
+ //
+ // Check whether a PTE or a 2MiB page is present
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ //
+ // Check if PDE maps a 2MiB page
+ //
+ if ((*TableEntry & IA32_PG_PS) != 0) {
+ return TRUE;
+ }
+
+ //
+ // Calculate physical address of page table and index of PTE
+ //
+ PageTable = (UINT64 *)(UINTN)(*TableEntry & PhysicalAddressMask);
+ Index = (UINTN)(RShiftU64 ((UINT64)LinearAddress, 12) & IndexMask);
+
+ //
+ // Select PTE
+ //
+ TableEntry = &PageTable[Index];
+
+ //
+ // Check if PTE maps a 4KiB page
+ //
+ if ((*TableEntry & IA32_PG_P) == 0) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Check if a linear address is valid.
+
+ @param[in] Cr0 CR0 control register.
+ @param[in] Cr3 CR3 control register.
+ @param[in] Cr4 CR4 control register.
+ @param[in] LinearAddress Linear address to be checked.
+**/
+BOOLEAN
+IsLinearAddressValid (
+ IN UINTN Cr0,
+ IN UINTN Cr3,
+ IN UINTN Cr4,
+ IN UINTN LinearAddress
+ )
+{
+ UINT32 Eax;
+ UINT32 Edx;
+ UINT8 MaxPhyAddrBits;
+ MSR_IA32_EFER_REGISTER Msr;
+ BOOLEAN AddressValid;
+
+ //
+ // Check for valid input parameters
+ //
+ if (Cr0 == 0 || Cr4 == 0 || LinearAddress == 0) {
+ return FALSE;
+ }
+
+ //
+ // Check if paging is disabled
+ //
+ if ((Cr0 & IA32_CR0_PG) == 0) {
+ //
+ // If CR4.PAE bit is set, then the linear (or physical) address supports
+ // only up to 36 bits.
+ //
+ if ((UINT64)LinearAddress > 0xFFFFFFFFFULL ||
+ ((Cr4 & IA32_CR4_PAE) == 0 && LinearAddress > 0xFFFFFFFF)) {
+ return FALSE;
+ }
+
+ return TRUE;
+ }
+
+ //
+ // Paging can be enabled only if CR0.PE bit is set
+ //
+ if ((Cr0 & IA32_CR0_PE) == 0) {
+ return FALSE;
+ }
+
+ //
+ // CR3 register cannot be zero if paging is enabled
+ //
+ if (Cr3 == 0) {
+ return FALSE;
+ }
+
+ //
+ // Get MAXPHYADDR bits
+ //
+ AsmCpuid (0x80000000, &Eax, NULL, NULL, NULL);
+ if (Eax >= 0x80000008) {
+ AsmCpuid (0x80000008, &Eax, NULL, NULL, NULL);
+ MaxPhyAddrBits = (UINT8)Eax;
+ } else {
+ AsmCpuid (1, NULL, NULL, NULL, &Edx);
+ if ((Edx & IA32_CPUID1_EDX_PAE) != 0) {
+ MaxPhyAddrBits = 36;
+ } else {
+ MaxPhyAddrBits = 32;
+ }
+ }
+
+ //
+ // Check if CR4.PAE bit is not set
+ //
+ if ((Cr4 & IA32_CR4_PAE) == 0) {
+ //
+ // Check if linear address is valid in 32-bit paging mode
+ //
+ AddressValid = Do32BitPagingModeCheck (Cr3, Cr4, LinearAddress);
+ } else {
+ //
+ // In either PAE or 4-level paging mode, physical addresses can hold only
+ // up to 52 bits.
+ //
+ if (MaxPhyAddrBits > 52) {
+ return FALSE;
+ }
+
+ //
+ // Read IA32_EFER MSR register
+ //
+ Msr.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
+
+ //
+ // Check if IA32_EFER.LME bit is not set (e.g. PAE paging mode)
+ //
+ if (Msr.Bits.LME == 0) {
+ //
+ // Check if linear address is valid in PAE paging mode
+ //
+ AddressValid = DoPAEPagingModeCheck (Cr3, MaxPhyAddrBits, LinearAddress);
+ } else {
+ //
+ // Check if linear address is valid in 4-level paging mode
+ //
+ AddressValid = Do4LevelPagingModeCheck (Cr3, MaxPhyAddrBits,
+ LinearAddress);
+ }
+ }
+
+ return AddressValid;
+}
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
index ec46c2d9d3..1b51034c25 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -330,5 +330,21 @@ GetPdbFileName (
OUT CHAR8 **PdbFileName
);
+/**
+ Check if a linear address is valid.
+
+ @param[in] Cr0 CR0 control register.
+ @param[in] Cr3 CR3 control register.
+ @param[in] Cr4 CR4 control register.
+ @param[in] LinearAddress Linear address to be checked.
+**/
+BOOLEAN
+IsLinearAddressValid (
+ IN UINTN Cr0,
+ IN UINTN Cr3,
+ IN UINTN Cr4,
+ IN UINTN LinearAddress
+ );
+
#endif
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Validate all possible memory dereferences during stack traces in IA32
and X64 CPU exceptions.
Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Requested-by: Brian Johnson <brian.johnson@hpe.com>
Requested-by: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 149 +++++++++++++++++++-
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 75 +++++++++-
2 files changed, 216 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index c5d6ea0939..3b92512b92 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -14,6 +14,11 @@
#include "CpuExceptionCommon.h"
+//
+// IA32 Segment Selector bit definitions
+//
+#define IA32_SEGSEL_TI BIT2
+
/**
Return address map of exception handler template so that C code can generate
exception tables.
@@ -398,6 +403,97 @@ DumpCpuContext (
);
}
+/**
+ Check if a logical address is valid.
+
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+ @param[in] SegmentSelector Segment selector.
+ @param[in] Offset Offset or logical address.
+**/
+STATIC
+BOOLEAN
+IsLogicalAddressValid (
+ IN EFI_SYSTEM_CONTEXT SystemContext,
+ IN UINT16 SegmentSelector,
+ IN UINTN Offset
+ )
+{
+ IA32_SEGMENT_DESCRIPTOR *SegmentDescriptor;
+ UINT32 SegDescBase;
+ UINT32 SegDescLimit;
+ UINT64 SegDescLimitInBytes;
+
+ //
+ // Check for valid input parameters
+ //
+ if (SegmentSelector == 0 || Offset == 0) {
+ return FALSE;
+ }
+
+ //
+ // Look for a segment descriptor in a GDT or LDT table depending on TI
+ // (Table Indicator) bit in segment selector.
+ //
+ if ((SegmentSelector & IA32_SEGSEL_TI) == 0) {
+ //
+ // Get segment descriptor from GDT table
+ //
+ SegmentDescriptor =
+ (IA32_SEGMENT_DESCRIPTOR *)(
+ (UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
+ (SegmentSelector & ~7)
+ );
+ } else {
+ //
+ // Get segment descriptor from LDT table
+ //
+ SegmentDescriptor =
+ (IA32_SEGMENT_DESCRIPTOR *)(
+ (UINTN)SystemContext.SystemContextIa32->Ldtr +
+ (SegmentSelector & ~7)
+ );
+ }
+
+ //
+ // Get segment descriptor's base address
+ //
+ SegDescBase = SegmentDescriptor->Bits.BaseLow |
+ (SegmentDescriptor->Bits.BaseMid << 16) |
+ (SegmentDescriptor->Bits.BaseHigh << 24);
+
+ //
+ // Get segment descriptor's limit
+ //
+ SegDescLimit = SegmentDescriptor->Bits.LimitLow |
+ (SegmentDescriptor->Bits.LimitHigh << 16);
+
+ //
+ // Calculate segment descriptor's limit in bytes
+ //
+ if (SegmentDescriptor->Bits.G == 1) {
+ SegDescLimitInBytes = (UINT64)SegDescLimit * SIZE_4KB + (SIZE_4KB - 1);
+ } else {
+ SegDescLimitInBytes = SegDescLimit;
+ }
+
+ //
+ // Make sure to not access beyond a segment limit boundary
+ //
+ if ((UINT64)Offset + SegDescBase > SegDescLimitInBytes) {
+ return FALSE;
+ }
+
+ //
+ // Check if the translated logical address (or linear address) is valid
+ //
+ return IsLinearAddressValid (
+ SystemContext.SystemContextIa32->Cr0,
+ SystemContext.SystemContextIa32->Cr3,
+ SystemContext.SystemContextIa32->Cr4,
+ Offset + SegDescBase
+ );
+}
+
/**
Dump stack trace.
@@ -470,6 +566,20 @@ DumpStacktrace (
InternalPrintMessage ("\nCall trace:\n");
for (;;) {
+ //
+ // Check for valid frame pointer
+ //
+ if (!IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)Ebp + 4) ||
+ !IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)Ebp)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+ "pointer at 0x%08x\n", __FUNCTION__, Ebp);
+ break;
+ }
+
//
// Print stack frame in the following format:
//
@@ -610,6 +720,16 @@ DumpImageModuleNames (
// Walk through call stack and find next module names
//
for (;;) {
+ if (!IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)Ebp) ||
+ !IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)Ebp + 4)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+ "pointer at 0x%08x\n", __FUNCTION__, Ebp);
+ }
+
//
// Set EIP with return address from current stack frame
//
@@ -673,16 +793,23 @@ DumpImageModuleNames (
/**
Dump stack contents.
- @param[in] CurrentEsp Current stack pointer address.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
@param[in] UnwoundStacksCount Count of unwound stack frames.
**/
STATIC
VOID
DumpStackContents (
- IN UINT32 CurrentEsp,
- IN INTN UnwoundStacksCount
+ IN EFI_SYSTEM_CONTEXT SystemContext,
+ IN INTN UnwoundStacksCount
)
{
+ UINT32 CurrentEsp;
+
+ //
+ // Get current stack pointer
+ //
+ CurrentEsp = SystemContext.SystemContextIa32->Esp;
+
//
// Check for proper stack alignment
//
@@ -696,6 +823,20 @@ DumpStackContents (
//
InternalPrintMessage ("\nStack dump:\n");
while (UnwoundStacksCount-- > 0) {
+ //
+ // Check for a valid stack pointer address
+ //
+ if (!IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)CurrentEsp) ||
+ !IsLogicalAddressValid (SystemContext,
+ SystemContext.SystemContextIa32->Ss,
+ (UINTN)CurrentEsp + 4)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid stack "
+ "pointer at 0x%08x\n", __FUNCTION__, CurrentEsp);
+ break;
+ }
+
InternalPrintMessage (
"0x%08x: %08x %08x\n",
CurrentEsp,
@@ -742,5 +883,5 @@ DumpImageAndCpuContent (
//
// Dump stack contents
//
- DumpStackContents (SystemContext.SystemContextIa32->Esp, UnwoundStacksCount);
+ DumpStackContents (SystemContext, UnwoundStacksCount);
}
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 523dce95c9..c81f4c00eb 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -401,16 +401,26 @@ DumpCpuContext (
/**
Dump stack contents.
- @param[in] CurrentRsp Current stack pointer address.
+ @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
@param[in] UnwoundStacksCount Count of unwound stack frames.
**/
STATIC
VOID
DumpStackContents (
- IN UINT64 CurrentRsp,
- IN INTN UnwoundStacksCount
+ IN EFI_SYSTEM_CONTEXT SystemContext,
+ IN INTN UnwoundStacksCount
)
{
+ UINT64 CurrentRsp;
+ UINTN Cr0;
+ UINTN Cr3;
+ UINTN Cr4;
+
+ //
+ // Get current stack pointer
+ //
+ CurrentRsp = SystemContext.SystemContextX64->Rsp;
+
//
// Check for proper stack pointer alignment
//
@@ -419,11 +429,28 @@ DumpStackContents (
return;
}
+ //
+ // Get system control registers
+ //
+ Cr0 = SystemContext.SystemContextX64->Cr0;
+ Cr3 = SystemContext.SystemContextX64->Cr3;
+ Cr4 = SystemContext.SystemContextX64->Cr4;
+
//
// Dump out stack contents
//
InternalPrintMessage ("\nStack dump:\n");
while (UnwoundStacksCount-- > 0) {
+ //
+ // Check for a valid stack pointer address
+ //
+ if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp) ||
+ !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp + 8)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid stack "
+ "pointer at 0x%016lx\n", __FUNCTION__, CurrentRsp);
+ break;
+ }
+
InternalPrintMessage (
"0x%016lx: %016lx %016lx\n",
CurrentRsp,
@@ -459,6 +486,9 @@ DumpImageModuleNames (
CHAR8 *PdbFileName;
UINT64 Rbp;
UINTN LastImageBase;
+ UINTN Cr0;
+ UINTN Cr3;
+ UINTN Cr4;
//
// Set current RIP address
@@ -527,10 +557,27 @@ DumpImageModuleNames (
InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
}
+ //
+ // Get system control registers
+ //
+ Cr0 = SystemContext.SystemContextX64->Cr0;
+ Cr3 = SystemContext.SystemContextX64->Cr3;
+ Cr4 = SystemContext.SystemContextX64->Cr4;
+
//
// Walk through call stack and find next module names
//
for (;;) {
+ //
+ // Check for a valid frame pointer
+ //
+ if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) ||
+ !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+ "pointer at 0x%016lx\n", __FUNCTION__, Rbp);
+ break;
+ }
+
//
// Set RIP with return address from current stack frame
//
@@ -617,6 +664,9 @@ DumpStacktrace (
UINT64 Rbp;
UINTN ImageBase;
CHAR8 *PdbFileName;
+ UINTN Cr0;
+ UINTN Cr3;
+ UINTN Cr4;
//
// Set current RIP address
@@ -656,12 +706,29 @@ DumpStacktrace (
//
*UnwoundStacksCount = 1;
+ //
+ // Get system control registers
+ //
+ Cr0 = SystemContext.SystemContextX64->Cr0;
+ Cr3 = SystemContext.SystemContextX64->Cr3;
+ Cr4 = SystemContext.SystemContextX64->Cr4;
+
//
// Print out back trace
//
InternalPrintMessage ("\nCall trace:\n");
for (;;) {
+ //
+ // Check for valid frame pointer
+ //
+ if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) ||
+ !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+ "pointer at 0x%016lx\n", __FUNCTION__, Rbp);
+ break;
+ }
+
//
// Print stack frame in the following format:
//
@@ -749,5 +816,5 @@ DumpImageAndCpuContent (
//
// Dump stack contents
//
- DumpStackContents (SystemContext.SystemContextX64->Rsp, UnwoundStacksCount);
+ DumpStackContents (SystemContext, UnwoundStacksCount);
}
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Remove the supurious '- 1' when calculating the IP addresses during the
stack traces.
Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Requested-by: Jeff Fan <vanjeff_919@hotmail.com>
Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 2 +-
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 3b92512b92..31fbd4a164 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -590,7 +590,7 @@ DumpStacktrace (
*UnwoundStacksCount - 1,
Eip,
ImageBase,
- Eip - ImageBase - 1,
+ Eip - ImageBase,
Ebp,
PdbFileName
);
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index c81f4c00eb..71d2d2f5d4 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -739,7 +739,7 @@ DumpStacktrace (
*UnwoundStacksCount - 1,
Rip,
ImageBase,
- Rip - ImageBase - 1,
+ Rip - ImageBase,
Rbp,
PdbFileName
);
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Introduce a new IsLinearAddressRangeValid() function to validate a given
address range and check whether or not it is valid.
This function is useful for validating ranges of memory addresses during
stack traces in X64.
Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Requested-by: Brian Johnson <brian.johnson@hpe.com>
Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 40 ++++++++++++++++++++
UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h | 18 +++++++++
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 40 ++++++++++++--------
3 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
index 7ac13640de..e1dd054259 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -589,3 +589,43 @@ IsLinearAddressValid (
return AddressValid;
}
+
+/**
+ Check if a linear address range is valid.
+
+ @param[in] Cr0 CR0 control register.
+ @param[in] Cr3 CR3 control register.
+ @param[in] Cr4 CR4 control register.
+ @param[in] LinearAddressStart Linear address start.
+ @param[in] LinearAddressEnd Linear address end.
+**/
+BOOLEAN
+IsLinearAddressRangeValid (
+ IN UINTN Cr0,
+ IN UINTN Cr3,
+ IN UINTN Cr4,
+ IN UINTN LinearAddressStart,
+ IN UINTN LinearAddressEnd
+ )
+{
+ //
+ // Check for valid input parameters
+ //
+ if (LinearAddressStart == 0 || LinearAddressEnd == 0 ||
+ LinearAddressStart > LinearAddressEnd) {
+ return FALSE;
+ }
+
+ //
+ // Validate all linear addresses within the given range
+ //
+ for (LinearAddressStart &= ~(SIZE_4KB - 1);
+ LinearAddressStart <= LinearAddressEnd;
+ LinearAddressStart += SIZE_4KB) {
+ if (!IsLinearAddressValid (Cr0, Cr3, Cr4, LinearAddressStart)) {
+ return FALSE;
+ }
+ }
+
+ return TRUE;
+}
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
index 1b51034c25..075f668290 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -346,5 +346,23 @@ IsLinearAddressValid (
IN UINTN LinearAddress
);
+/**
+ Check if a linear address range is valid.
+
+ @param[in] Cr0 CR0 control register.
+ @param[in] Cr3 CR3 control register.
+ @param[in] Cr4 CR4 control register.
+ @param[in] LinearAddressStart Linear address start.
+ @param[in] LinearAddressEnd Linear address end.
+**/
+BOOLEAN
+IsLinearAddressRangeValid (
+ IN UINTN Cr0,
+ IN UINTN Cr3,
+ IN UINTN Cr4,
+ IN UINTN LinearAddressStart,
+ IN UINTN LinearAddressEnd
+ );
+
#endif
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 71d2d2f5d4..4d8c9b0a89 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -415,6 +415,8 @@ DumpStackContents (
UINTN Cr0;
UINTN Cr3;
UINTN Cr4;
+ UINTN RspAddressStart;
+ UINTN RspAddressEnd;
//
// Get current stack pointer
@@ -436,21 +438,29 @@ DumpStackContents (
Cr3 = SystemContext.SystemContextX64->Cr3;
Cr4 = SystemContext.SystemContextX64->Cr4;
+ //
+ // Calculate address range of the stack pointers
+ //
+ RspAddressStart = (UINTN)CurrentRsp;
+ RspAddressEnd =
+ RspAddressStart + (UINTN)UnwoundStacksCount * CPU_STACK_ALIGNMENT;
+
+ //
+ // Validate address range of stack pointers
+ //
+ if (!IsLinearAddressRangeValid (Cr0, Cr3, Cr4, RspAddressStart,
+ RspAddressEnd)) {
+ InternalPrintMessage ("%a: attempted to dereference an invalid stack "
+ "pointer at 0x%016lx - 0x%016lx\n", __FUNCTION__,
+ RspAddressStart, RspAddressEnd);
+ return;
+ }
+
//
// Dump out stack contents
//
InternalPrintMessage ("\nStack dump:\n");
while (UnwoundStacksCount-- > 0) {
- //
- // Check for a valid stack pointer address
- //
- if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp) ||
- !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp + 8)) {
- InternalPrintMessage ("%a: attempted to dereference an invalid stack "
- "pointer at 0x%016lx\n", __FUNCTION__, CurrentRsp);
- break;
- }
-
InternalPrintMessage (
"0x%016lx: %016lx %016lx\n",
CurrentRsp,
@@ -459,7 +469,7 @@ DumpStackContents (
);
//
- // Point to next stack
+ // Point to next stack pointer
//
CurrentRsp += CPU_STACK_ALIGNMENT;
}
@@ -571,8 +581,8 @@ DumpImageModuleNames (
//
// Check for a valid frame pointer
//
- if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) ||
- !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) {
+ if (!IsLinearAddressRangeValid (Cr0, Cr3, Cr4, (UINTN)Rbp,
+ (UINTN)Rbp + 8)) {
InternalPrintMessage ("%a: attempted to dereference an invalid frame "
"pointer at 0x%016lx\n", __FUNCTION__, Rbp);
break;
@@ -722,8 +732,8 @@ DumpStacktrace (
//
// Check for valid frame pointer
//
- if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) ||
- !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) {
+ if (!IsLinearAddressRangeValid (Cr0, Cr3, Cr4, (UINTN)Rbp,
+ (UINTN)Rbp + 8)) {
InternalPrintMessage ("%a: attempted to dereference an invalid frame "
"pointer at 0x%016lx\n", __FUNCTION__, Rbp);
break;
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Add an early check in DumpStackContens() to abort in case of no unwound
stacks.
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 <paulo@paulo.ac>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 7 +++++++
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 7 +++++++
2 files changed, 14 insertions(+)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 31fbd4a164..ac3801f704 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -805,6 +805,13 @@ DumpStackContents (
{
UINT32 CurrentEsp;
+ //
+ // Do nothing in case there wasn't any unwound stack.
+ //
+ if (UnwoundStacksCount == 0) {
+ return;
+ }
+
//
// Get current stack pointer
//
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 4d8c9b0a89..6c3bad01a6 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -418,6 +418,13 @@ DumpStackContents (
UINTN RspAddressStart;
UINTN RspAddressEnd;
+ //
+ // Do nothing in case there wasn't any unwound stack.
+ //
+ if (UnwoundStacksCount == 0) {
+ return;
+ }
+
//
// Get current stack pointer
//
--
2.14.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.