From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A5B352232BE1F for ; Wed, 17 Jan 2018 04:52:48 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jan 2018 04:58:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,372,1511856000"; d="scan'208";a="166807734" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga004.jf.intel.com with ESMTP; 17 Jan 2018 04:58:08 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 17 Jan 2018 04:58:08 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 17 Jan 2018 04:57:06 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Wed, 17 Jan 2018 20:57:05 +0800 From: "Yao, Jiewen" To: Paulo Alcantara , "edk2-devel@lists.01.org" CC: Rick Bramley , "Dong, Eric" , Kimon Berlin , Andrew Fish , "Diego Medaglia" , Laszlo Ersek Thread-Topic: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling Thread-Index: AQHTjZc4uDNA9nHm1kuOPH2nB41jpqN3+5Ww Date: Wed, 17 Jan 2018 12:57:04 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AA8057C@shsmsx102.ccr.corp.intel.com> References: In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTUzNDY4MDctYjRjYS00NDUxLTlhMDEtMWYyMzRiY2IyYWExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJaTnJXcVkwOFpUdVh6MVVxd3ZsVFpIWDFlZ0cwelZLZlY4MysxR2RVZitaMEVxeVBKOGFoSDRwdk4rVitFRzd4In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [RFC v5 0/8] Stack trace support in X64 exception handling X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jan 2018 12:52:49 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 co= mment below is for the final code, not for a specific patch. 1) CpuExceptionCommon.c: IsLinearAddressRangeValid(). // // Check for valid input parameters // if (LinearAddressStart =3D=3D 0 || LinearAddressEnd =3D=3D 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 protectio= n, 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 =3D=3D EXCEPT_IA32_PAGE_FAULT) && ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != =3D 0)) { // // The EIP in SystemContext could not be used // if it is page fault with I/D set. // Eip =3D *(UINT32 *)(UINTN)SystemContext.SystemContextIa32->Esp; } else { Eip =3D 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 =3D PeCoffSearchImageBase (Eip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch= ExceptionHandler.c(613): ImageBase =3D PeCoffSearchImageBase (Eip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch= ExceptionHandler.c(682): ImageBase =3D PeCoffSearchImageBase (Eip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch= ExceptionHandler.c(741): ImageBase =3D PeCoffSearchImageBase (Eip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchE= xceptionHandler.c(540): ImageBase =3D PeCoffSearchImageBase (Rip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchE= xceptionHandler.c(613): ImageBase =3D PeCoffSearchImageBase (Rip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchE= xceptionHandler.c(710): ImageBase =3D PeCoffSearchImageBase (Rip); C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\ArchE= xceptionHandler.c(779): ImageBase =3D PeCoffSearchImageBase (Rip); The EIP from SystemContext seems good. I assume there is not a problem here= . if ((ExceptionType =3D=3D EXCEPT_IA32_PAGE_FAULT) && ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != =3D 0)) { // // The EIP in SystemContext could not be used // if it is page fault with I/D set. // Eip =3D *(UINT32 *)(UINTN)SystemContext.SystemContextIa32->Esp; } else { Eip =3D SystemContext.SystemContextIa32->Eip; } // // Get initial PE/COFF image base address from current EIP // ImageBase =3D 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 entr= ypoint->SMM C code. Should we add a check for EIP here? // // Set EIP with return address from current stack frame // Eip =3D *(UINT32 *)((UINTN)Ebp + 4); // // If EIP is zero, then stop unwinding the stack // if (Eip =3D=3D 0) { break; } // // Search for the respective PE/COFF image based on EIP // ImageBase =3D 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 PeC= offSearchImageBase(). :-) Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pa= ulo > Alcantara > Sent: Monday, January 15, 2018 8:23 AM > To: edk2-devel@lists.01.org > Cc: Rick Bramley ; Dong, Eric > ; Kimon Berlin ; Andrew Fish > ; Yao, Jiewen ; Diego Medaglia > ; Laszlo Ersek > Subject: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handlin= g >=20 > Hi, >=20 > This series adds stack trace support during IA32 and X64 CPU exceptions. >=20 > Informations like back trace, stack contents and image module names > (that were part of the call stack) will be dumped out. >=20 > The current limitation is that it relies on available frame pointers > (GCC only) in order to successfully unwind the stack. >=20 > Jiewen, >=20 > 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. >=20 > 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. >=20 > Do you know how we could fix that issue? Perhaps introducing a > AddressValidationLib (as Brian suggested previously) and use it within > PeCoffSearchImageBase()? >=20 > I'd also like to thank Brian & Jeff for all the support! >=20 > Thanks > Paulo >=20 > Repo: https://github.com/pcacjr/edk2.git > Branch: stacktrace_v5 >=20 > Cc: Rick Bramley > Cc: Kimon Berlin > Cc: Diego Medaglia > Cc: Andrew Fish > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Brian Johnson > Cc: Jeff Fan > Cc: Jiewen Yao > Cc: Paulo Alcantara > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Paulo Alcantara > --- >=20 > 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. >=20 > 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) >=20 > 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) >=20 > 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. >=20 > 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 >=20 > 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(-) >=20 > -- > 2.14.3 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel