From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=34.238.86.106; helo=mail.paulo.ac; envelope-from=paulo@paulo.ac; receiver=edk2-devel@lists.01.org Received: from mail.paulo.ac (mail.paulo.ac [34.238.86.106]) (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 7F6252034A8C2 for ; Thu, 18 Jan 2018 15:57:30 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by mail.paulo.ac (Postfix) with ESMTP id D896BC78F08; Fri, 19 Jan 2018 00:02:50 +0000 (UTC) X-Virus-Scanned: amavisd-new at paulo.ac X-Spam-Flag: NO X-Spam-Score: -1.099 X-Spam-Level: X-Spam-Status: No, score=-1.099 tagged_above=-999 required=6.31 tests=[ALL_TRUSTED=-1, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no Authentication-Results: mail.paulo.ac (amavisd-new); dkim=pass (1024-bit key) header.d=paulo.ac Received: from mail.paulo.ac ([127.0.0.1]) by localhost (mail.paulo.ac [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jD8Z-MuXqC7a; Fri, 19 Jan 2018 00:02:29 +0000 (UTC) Received: from localhost (200.175.82.165.dynamic.adsl.gvt.net.br [200.175.82.165]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.paulo.ac (Postfix) with ESMTPSA id E4760C78F07; Fri, 19 Jan 2018 00:02:28 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.paulo.ac E4760C78F07 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=paulo.ac; s=default; t=1516320149; bh=3Mv/BC4p1/Q32Xil2j5nhkdq8Jx7IqSyv4iVjg27Ulw=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Eak0OJ7OEdinsUKn6dl+hQOjy3ocPaoWyLLb6/bGWevpkp/nsWbEm0MOacMe3T612 dLm3obGEPPY1BdE/x1Sxaql6bnwomGyzA7UuMJyOhMLqRnO5XV3ch9WLp6KOpjtXrz ALXYNz+q3kIvGwukGHzYT/TwNFuI/hIrqp94gk6o= From: Paulo Alcantara To: "Yao\, Jiewen" , "edk2-devel\@lists.01.org" Cc: Rick Bramley , "Dong\, Eric" , Kimon Berlin , Andrew Fish , Diego Medaglia , Laszlo Ersek In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA8057C@shsmsx102.ccr.corp.intel.com> References: <74D8A39837DF1E4DA445A8C0B3885C503AA8057C@shsmsx102.ccr.corp.intel.com> Date: Thu, 18 Jan 2018 22:02:19 -0200 Message-ID: <87shb2af78.fsf@paulo.ac> 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: Thu, 18 Jan 2018 23:57:31 -0000 Content-Type: text/plain "Yao, Jiewen" 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 ; Dong, Eric >> ; Kimon Berlin ; Andrew Fish >> ; Yao, Jiewen ; Diego Medaglia >> ; Laszlo Ersek >> 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 >> 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 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