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 061792034A8B3 for ; Wed, 17 Jan 2018 14:42:59 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jan 2018 14:48:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,374,1511856000"; d="scan'208";a="196433461" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga005.fm.intel.com with ESMTP; 17 Jan 2018 14:48:20 -0800 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 17 Jan 2018 14:48:20 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 17 Jan 2018 14:48:20 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Thu, 18 Jan 2018 06:48:17 +0800 From: "Yao, Jiewen" To: "Yao, Jiewen" , 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+5WwgACxljA= Date: Wed, 17 Jan 2018 22:48:17 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AA80BD7@shsmsx102.ccr.corp.intel.com> References: <74D8A39837DF1E4DA445A8C0B3885C503AA8057C@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA8057C@shsmsx102.ccr.corp.intel.com> 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 22:43:00 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 add= ress might not be enough to guarantee the address is inside of an *executab= le* page. I am thinking if we need check the non-executable bit (BIT63) as well for D= umpImageModuleNames(). (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 fu= nction for the PeCoffLib, instead of let PeCoffLib depend on another librar= y. For example: PeCoffSearchImageBaseEx (Address, ValidateAddressFunc) Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ya= o, > Jiewen > Sent: Wednesday, January 17, 2018 8:57 PM > To: Paulo Alcantara ; edk2-devel@lists.01.org > Cc: Rick Bramley ; Dong, Eric > ; Kimon Berlin ; Andrew Fish > ; Diego Medaglia ; Laszlo Ersek > > Subject: Re: [edk2] [RFC v5 0/8] Stack trace support in X64 exception han= dling >=20 > Thanks Paulo. >=20 > The series looks really good. I give it thumb up. :-) >=20 > 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. >=20 > 1) CpuExceptionCommon.c: IsLinearAddressRangeValid(). > // > // Check for valid input parameters > // > if (LinearAddressStart =3D=3D 0 || LinearAddressEnd =3D=3D 0 || > LinearAddressStart > LinearAddressEnd) { > return FALSE; > } >=20 > 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 protect= ion, it > can be excluded in page table parsing. >=20 > 2) I found below logic appears in 2 functions - DumpImageModuleNames() an= d > DumpStacktrace(). Is that possible to merge them? > We can calculate in DumpImageAndCpuContent() and use Eip as parameter. Ju= st > in case there is 3rd function need use EIP, it won't miss the calculation= . > (It is a bug fix we did recently.) >=20 > // > // 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; > } >=20 > 3) I am a little surprised on PeCoffSearchImageBase() issue. >=20 > 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? >=20 >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch > ExceptionHandler.c(547): ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch > ExceptionHandler.c(613): ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch > ExceptionHandler.c(682): ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\Ia32\Arch > ExceptionHandler.c(741): ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch > ExceptionHandler.c(540): ImageBase =3D PeCoffSearchImageBase (Rip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch > ExceptionHandler.c(613): ImageBase =3D PeCoffSearchImageBase (Rip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch > ExceptionHandler.c(710): ImageBase =3D PeCoffSearchImageBase (Rip); >=20 > C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\CpuExceptionHandlerLib\X64\Arch > ExceptionHandler.c(779): ImageBase =3D PeCoffSearchImageBase (Rip); >=20 > The EIP from SystemContext seems good. I assume there is not a problem he= re. >=20 > 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; > } >=20 > // > // Get initial PE/COFF image base address from current EIP > // > ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > But the EIP from stack frame is unknown and risky. Especially for the cod= e 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? >=20 > // > // Set EIP with return address from current stack frame > // > Eip =3D *(UINT32 *)((UINTN)Ebp + 4); >=20 > // > // If EIP is zero, then stop unwinding the stack > // > if (Eip =3D=3D 0) { > break; > } >=20 > // > // Search for the respective PE/COFF image based on EIP > // > ImageBase =3D PeCoffSearchImageBase (Eip); >=20 > If you can help us do some more investigation on the root-cause and narro= w > down the issue, I will appreciate that. >=20 > We can decide how to fix once it is root-caused. >=20 > Maybe we just do a simple IsLogicalAddressValid () check before we call > PeCoffSearchImageBase(). :-) >=20 >=20 > Thank you > Yao Jiewen >=20 > > -----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 ; Dong, Eric > > ; Kimon Berlin ; Andrew Fish > > ; Yao, Jiewen ; Diego Medaglia > > ; Laszlo Ersek > > Subject: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handl= ing > > > > 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 > > 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 > > --- > > > > 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 the= m > > 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