Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SSP restoring in edge cases #104820

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Fix SSP restoring in edge cases #104820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b687c8
Fix SSP restoring in edge cases
Jul 12, 2024
565f9ab
Fix linux x64 build
Jul 12, 2024
c54cf5d
Fix release build REGDISPLAY size constant
Jul 16, 2024
a195d54
Add regression test
Jul 16, 2024
e611fc4
Update src/tests/Regressions/coreclr/GitHub_104820/test104820.cs
jkotas Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/coreclr/inc/regdisp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;

Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/vm/amd64/Context.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed documented as only bits 0-7 are used, regardless of operand size.

Perhaps there is a way to save a byte of assembly by doing INCSSPD eax :-)

Copy link
Member Author

@janvorli janvorli Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The incsspd increments the SSP by multiples of 4, not by 8 :-)

Copy link
Member

@VSadov VSadov Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also make it use bits 0-3, because why not ... :-)

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
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/vm/amd64/Context.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/vm/amd64/cgenamd64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it a subtle bug that the Rip was off, or we just did not care about the correct IP in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually expected due to the way how the InlinedCallFrame is created in some cases. Neither the GC stack walk nor the new EH cared, but it caused problem with the newly added search for the first managed frame, as it sometimes is one that called the pinvoke (QCALL).

}
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;

Expand Down
37 changes: 25 additions & 12 deletions src/coreclr/vm/exceptionhandling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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();
Expand All @@ -7941,8 +7948,6 @@ extern "C" void QCALLTYPE ResumeAtInterceptionLocation(REGDISPLAY* pvRegDisplay)
_ASSERTE(FitsIn<DWORD>(ulRelOffset));
uResumePC = codeInfo.GetJitManager()->GetCodeAddressForRelOffset(codeInfo.GetMethodToken(), static_cast<DWORD>(ulRelOffset));

size_t targetSSP = GetSSPForFrameOnCurrentStack(pvRegDisplay->pCurrentContext);

SetIP(pvRegDisplay->pCurrentContext, uResumePC);
ClrRestoreNonvolatileContext(pvRegDisplay->pCurrentContext, targetSSP);
}
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/vm/stackwalk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down
Loading