From 0b687c8b31fdb65795d3d7a4e82585ef7b64e1b3 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 12 Jul 2024 20:19:42 +0200 Subject: [PATCH 1/5] Fix SSP restoring in edge cases There are edge cases when the SSP restoring for continuation after a catch handler completes doesn't work correctly. The problem is caused by the fact that we scan for the Rip of the frame handling the exception on the shadow stack to find where to restore it, and in those edge cases, the same address can be there multiple times and the first occurence is not the right one. For example, when an exception is thrown from a catch handler, it escapes the handler and the handler for the escaped exception is in the same method as the one that invoked the handler. This change fixes it by finding the SSP of the first managed frame where we search for the handler and then updating the SSP with every unwind. The SSP is stored in the REGDISPLAY. So when we reach the CallCatchFunclet, the REGDISPLAY contains the SSP to restore. There was also one more issue with restoring the SSP. I turned out that the incsspq instruction uses only the lowest 8 bits of the argument to increment the SSP, so the ClrRestoreNonVolatileContextWorker needs to have a loop that repeats that instruction in case we need to move it by more than 255 slots. --- src/coreclr/inc/regdisp.h | 10 ++++++++ src/coreclr/vm/amd64/Context.S | 9 ++++++- src/coreclr/vm/amd64/Context.asm | 9 ++++++- src/coreclr/vm/amd64/cgenamd64.cpp | 9 ++++++- src/coreclr/vm/exceptionhandling.cpp | 37 +++++++++++++++++++--------- src/coreclr/vm/stackwalk.cpp | 9 +++++++ 6 files changed, 68 insertions(+), 15 deletions(-) diff --git a/src/coreclr/inc/regdisp.h b/src/coreclr/inc/regdisp.h index 47a67237fb8f9..2d29717ab3e66 100644 --- a/src/coreclr/inc/regdisp.h +++ b/src/coreclr/inc/regdisp.h @@ -45,6 +45,10 @@ struct REGDISPLAY_BASE { TADDR SP; TADDR ControlPC; // LOONGARCH: use RA for PC + +#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) + TADDR SSP; +#endif }; inline PCODE GetControlPC(const REGDISPLAY_BASE *pRD) { @@ -510,6 +514,12 @@ inline void FillRegDisplay(const PREGDISPLAY pRD, PT_CONTEXT pctx, PT_CONTEXT pC // This will setup the PC and SP SyncRegDisplayToCurrentContext(pRD); +#if !defined(DACCESS_COMPILE) +#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) + pRD->SSP = GetSSP(pctx); +#endif +#endif // !DACCESS_COMPILE + if (fLightUnwind) return; diff --git a/src/coreclr/vm/amd64/Context.S b/src/coreclr/vm/amd64/Context.S index 1eda47f0f9dc3..79c8c67a16e5a 100644 --- a/src/coreclr/vm/amd64/Context.S +++ b/src/coreclr/vm/amd64/Context.S @@ -44,7 +44,14 @@ NESTED_ENTRY ClrRestoreNonvolatileContextWorker, _TEXT, NoHandler rdsspq rax sub r11, rax shr r11, 3 - incsspq r11 + ; the incsspq instruction uses only the lowest 8 bits of the argument, so we need to loop in case the increment is larger than 255 + mov rax, 255 + Update_Loop: + cmp r11, rax + cmovb rax, r11 + incsspq rax + sub r11, rax + ja Update_Loop No_Ssp_Update: // When user-mode shadow stacks are enabled, and for example the intent is to continue execution in managed code after diff --git a/src/coreclr/vm/amd64/Context.asm b/src/coreclr/vm/amd64/Context.asm index a2dbbda0b3099..8edafc38f63d0 100644 --- a/src/coreclr/vm/amd64/Context.asm +++ b/src/coreclr/vm/amd64/Context.asm @@ -53,7 +53,14 @@ NESTED_ENTRY ClrRestoreNonvolatileContextWorker, _TEXT rdsspq rax sub r11, rax shr r11, 3 - incsspq r11 + ; the incsspq instruction uses only the lowest 8 bits of the argument, so we need to loop in case the increment is larger than 255 + mov rax, 255 + Update_Loop: + cmp r11, rax + cmovb rax, r11 + incsspq rax + sub r11, rax + ja Update_Loop No_Ssp_Update: ; When user-mode shadow stacks are enabled, and for example the intent is to continue execution in managed code after diff --git a/src/coreclr/vm/amd64/cgenamd64.cpp b/src/coreclr/vm/amd64/cgenamd64.cpp index 91982948b8b7e..6959899e8b89c 100644 --- a/src/coreclr/vm/amd64/cgenamd64.cpp +++ b/src/coreclr/vm/amd64/cgenamd64.cpp @@ -108,13 +108,20 @@ void InlinedCallFrame::UpdateRegDisplay(const PREGDISPLAY pRD, bool updateFloats if (updateFloats) { UpdateFloatingPointRegisters(pRD); + // The float updating unwinds the stack so the pRD->pCurrentContext->Rip contains correct unwound Rip + // This is used for exception handling and the Rip extracted from m_pCallerReturnAddress is slightly + // off, which causes problem with searching for the return address on shadow stack on x64, so + // we keep the value from the unwind. } + else #endif // DACCESS_COMPILE + { + pRD->pCurrentContext->Rip = *(DWORD64 *)&m_pCallerReturnAddress; + } pRD->IsCallerContextValid = FALSE; pRD->IsCallerSPValid = FALSE; // Don't add usage of this field. This is only temporary. - pRD->pCurrentContext->Rip = *(DWORD64 *)&m_pCallerReturnAddress; pRD->pCurrentContext->Rsp = *(DWORD64 *)&m_pCallSiteSP; pRD->pCurrentContext->Rbp = *(DWORD64 *)&m_pCalleeSavedFP; diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 33d0f060386a4..f5b34a3d6cd28 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -7673,27 +7673,23 @@ UINT_PTR GetEstablisherFrame(REGDISPLAY* pvRegDisplay, ExInfo* exInfo) #endif } -size_t GetSSPForFrameOnCurrentStack(CONTEXT *pContext) -{ - size_t *targetSSP = 0; - #if defined(HOST_AMD64) && defined(HOST_WINDOWS) - targetSSP = (size_t *)_rdsspq(); - // Find the shadow stack pointer for the frame we are going to restore to. +size_t GetSSPForFrameOnCurrentStack(TADDR ip) +{ + size_t *targetSSP = (size_t *)_rdsspq(); // The SSP we search is pointing to the return address of the frame represented - // by the passed in context. So we search for the instruction pointer from + // by the passed in IP. So we search for the instruction pointer from // the context and return one slot up from there. if (targetSSP != NULL) { - size_t ip = GetIP(pContext); while (*targetSSP++ != ip) { } } -#endif // HOST_AMD64 && HOST_WINDOWS return (size_t)targetSSP; } +#endif // HOST_AMD64 && HOST_WINDOWS extern "C" void * QCALLTYPE CallCatchFunclet(QCall::ObjectHandleOnStack exceptionObj, BYTE* pHandlerIP, REGDISPLAY* pvRegDisplay, ExInfo* exInfo) { @@ -7711,7 +7707,12 @@ extern "C" void * QCALLTYPE CallCatchFunclet(QCall::ObjectHandleOnStack exceptio exInfo->m_ScannedStackRange.ExtendUpperBound(exInfo->m_frameIter.m_crawl.GetRegisterSet()->SP); DWORD_PTR dwResumePC = 0; UINT_PTR callerTargetSp = 0; - size_t targetSSP = GetSSPForFrameOnCurrentStack(pvRegDisplay->pCurrentContext); +#if defined(HOST_AMD64) && defined(HOST_WINDOWS) + size_t targetSSP = exInfo->m_frameIter.m_crawl.GetRegisterSet()->SSP; + _ASSERTE(targetSSP == 0 || (*(size_t*)(targetSSP-8) == exInfo->m_frameIter.m_crawl.GetRegisterSet()->ControlPC)); +#else + size_t targetSSP = 0; +#endif if (pHandlerIP != NULL) { @@ -7929,6 +7930,12 @@ extern "C" void QCALLTYPE ResumeAtInterceptionLocation(REGDISPLAY* pvRegDisplay) pThread->GetExceptionState()->GetDebuggerState()->GetDebuggerInterceptInfo(&pInterceptMD, NULL, (PBYTE*)&(sfInterceptStackFrame.SP), &ulRelOffset, NULL); +#if defined(HOST_AMD64) && defined(HOST_WINDOWS) + TADDR targetSSP = pExInfo->m_frameIter.m_crawl.GetRegisterSet()->SSP; +#else + TADDR targetSSP = 0; +#endif + ExInfo::PopExInfos(pThread, (void*)targetSp); PCODE pStartAddress = pInterceptMD->GetNativeCode(); @@ -7941,8 +7948,6 @@ extern "C" void QCALLTYPE ResumeAtInterceptionLocation(REGDISPLAY* pvRegDisplay) _ASSERTE(FitsIn(ulRelOffset)); uResumePC = codeInfo.GetJitManager()->GetCodeAddressForRelOffset(codeInfo.GetMethodToken(), static_cast(ulRelOffset)); - size_t targetSSP = GetSSPForFrameOnCurrentStack(pvRegDisplay->pCurrentContext); - SetIP(pvRegDisplay->pCurrentContext, uResumePC); ClrRestoreNonvolatileContext(pvRegDisplay->pCurrentContext, targetSSP); } @@ -8416,6 +8421,14 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk if (result) { TADDR controlPC = pThis->m_crawl.GetRegisterSet()->ControlPC; + +#if defined(HOST_AMD64) && defined(HOST_WINDOWS) + // Get the SSP for the first managed frame. It is incremented during the stack walk so that + // when we reach the handling frame, it contains correct SSP to set when resuming after + // the catch handler. + pThis->m_crawl.GetRegisterSet()->SSP = GetSSPForFrameOnCurrentStack(controlPC); +#endif + if (!pThis->m_crawl.HasFaulted() && !pThis->m_crawl.IsIPadjusted()) { controlPC -= STACKWALK_CONTROLPC_ADJUST_OFFSET; diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 309c90b596c82..817963c819561 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -524,6 +524,12 @@ UINT_PTR Thread::VirtualUnwindCallFrame(PREGDISPLAY pRD, EECodeInfo* pCodeInfo / VirtualUnwindCallFrame(pRD->pCurrentContext, pRD->pCurrentContextPointers, pCodeInfo); } +#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) + if (pRD->SSP != 0) + { + pRD->SSP += 8; + } +#endif // TARGET_AMD64 && TARGET_WINDOWS SyncRegDisplayToCurrentContext(pRD); pRD->IsCallerContextValid = FALSE; pRD->IsCallerSPValid = FALSE; // Don't add usage of this field. This is only temporary. @@ -1555,6 +1561,9 @@ void StackFrameIterator::SkipTo(StackFrameIterator *pOtherStackFrameIterator) *pRD->pCurrentContextPointers = *pOtherRD->pCurrentContextPointers; SetIP(pRD->pCurrentContext, GetIP(pOtherRD->pCurrentContext)); SetSP(pRD->pCurrentContext, GetSP(pOtherRD->pCurrentContext)); +#ifdef TARGET_AMD64 + pRD->SSP = pOtherRD->SSP; +#endif #define CALLEE_SAVED_REGISTER(regname) pRD->pCurrentContext->regname = (pRD->pCurrentContextPointers->regname == NULL) ? pOtherRD->pCurrentContext->regname : *pRD->pCurrentContextPointers->regname; ENUM_CALLEE_SAVED_REGISTERS(); From 565f9ab4973deb774be9ab9abc50db1452481be8 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 13 Jul 2024 00:22:18 +0200 Subject: [PATCH 2/5] Fix linux x64 build --- src/coreclr/vm/amd64/Context.S | 2 +- src/coreclr/vm/stackwalk.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/amd64/Context.S b/src/coreclr/vm/amd64/Context.S index 79c8c67a16e5a..62ea993df9faf 100644 --- a/src/coreclr/vm/amd64/Context.S +++ b/src/coreclr/vm/amd64/Context.S @@ -44,7 +44,7 @@ NESTED_ENTRY ClrRestoreNonvolatileContextWorker, _TEXT, NoHandler rdsspq rax sub r11, rax shr r11, 3 - ; the incsspq instruction uses only the lowest 8 bits of the argument, so we need to loop in case the increment is larger than 255 + // the incsspq instruction uses only the lowest 8 bits of the argument, so we need to loop in case the increment is larger than 255 mov rax, 255 Update_Loop: cmp r11, rax diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 817963c819561..b333249637d37 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1561,7 +1561,7 @@ void StackFrameIterator::SkipTo(StackFrameIterator *pOtherStackFrameIterator) *pRD->pCurrentContextPointers = *pOtherRD->pCurrentContextPointers; SetIP(pRD->pCurrentContext, GetIP(pOtherRD->pCurrentContext)); SetSP(pRD->pCurrentContext, GetSP(pOtherRD->pCurrentContext)); -#ifdef TARGET_AMD64 +#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) pRD->SSP = pOtherRD->SSP; #endif From c54cf5daf08854725a8e2ee43c58a3b08ad974b6 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 16 Jul 2024 10:17:04 +0200 Subject: [PATCH 3/5] Fix release build REGDISPLAY size constant --- .../src/System/Runtime/ExceptionServices/AsmOffsets.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs index 9276457b269c0..9e7999a7bc9e4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs @@ -72,7 +72,7 @@ class AsmOffsets public const int OFFSETOF__REGDISPLAY__SP = 0x1a70; public const int OFFSETOF__REGDISPLAY__ControlPC = 0x1a78; #else // TARGET_UNIX - public const int SIZEOF__REGDISPLAY = 0xbe0; + public const int SIZEOF__REGDISPLAY = 0xbf0; public const int OFFSETOF__REGDISPLAY__SP = 0xbd0; public const int OFFSETOF__REGDISPLAY__ControlPC = 0xbd8; #endif // TARGET_UNIX From a195d54c5884ea782bcd26ee95df604c7fb4e656 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 16 Jul 2024 16:54:22 +0200 Subject: [PATCH 4/5] Add regression test --- .../coreclr/GitHub_104820/test104820.cs | 45 +++++++++++++++++++ .../coreclr/GitHub_104820/test104820.csproj | 11 +++++ 2 files changed, 56 insertions(+) create mode 100644 src/tests/Regressions/coreclr/GitHub_104820/test104820.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_104820/test104820.csproj diff --git a/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs b/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs new file mode 100644 index 0000000000000..e8cc128190c98 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Test104820 +{ + // Test that SSP is updated properly when resuming after catch when the shadow stack + // contains the instruction pointer of the resume frame multiple times and the SSP needs + // to be restored to the location of one that's not the first one found on the shadow + // stack. + // This test fails with stack overflow of the shadow stack if the SSP is updated incorrectly. + static void ShadowStackPointerUpdateTest(int depth) + { + if (depth == 0) + { + throw new Exception(); + } + + for (int i = 0; i < 1000; i++) + { + try + { + try + { + ShadowStackPointerUpdateTest(depth - 1); + } + catch (Exception) + { + throw new Exception(); + } + } + catch (Exception) when (depth == 100) + { + } + } + } + + [Fact] + public static void TestEntryPoint() + { + ShadowStackPointerUpdateTest(100); + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_104820/test104820.csproj b/src/tests/Regressions/coreclr/GitHub_104820/test104820.csproj new file mode 100644 index 0000000000000..7ab24bb080853 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_104820/test104820.csproj @@ -0,0 +1,11 @@ + + + 1 + + + + + + + + From e611fc44ede5077a899abf8e71d10532dd8ff90e Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 16 Jul 2024 08:55:30 -0700 Subject: [PATCH 5/5] Update src/tests/Regressions/coreclr/GitHub_104820/test104820.cs --- src/tests/Regressions/coreclr/GitHub_104820/test104820.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs b/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs index e8cc128190c98..30092ceaa7bb5 100644 --- a/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs +++ b/src/tests/Regressions/coreclr/GitHub_104820/test104820.cs @@ -40,6 +40,6 @@ static void ShadowStackPointerUpdateTest(int depth) [Fact] public static void TestEntryPoint() { - ShadowStackPointerUpdateTest(100); + ShadowStackPointerUpdateTest(100); } }