Capture Reverse P/Invoke frame offset in x86 GC info and disallow ret…

…urn hijacking of reverse P/Invokes on x86. (#49066) * Capture Reverse P/Invoke frame offset in x86 GC info and disallow return hijacking of reverse P/Invokes on x86. * Check reverse p/invoke frame in exception handling. * Fix formatting. * Change sentinel values since 0 is a valid offset for the reverse P/Invoke frame variable. * Fix decoding setting flip to actually flip the new sentinel values.
dotnet · Mar 5, 2021 · 197ce0f · 197ce0f
1 parent a6ac510
commit 197ce0f
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 8 deletions.
diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h
@@ -654,6 +654,10 @@ bool UnwindStackFrame(PREGDISPLAY     pContext,
                       unsigned        flags,
                       CodeManState   *pState,
                       StackwalkCacheUnwindInfo  *pUnwindInfo);
+
+size_t DecodeGCHdrInfo(GCInfoToken gcInfoToken,
+                       unsigned    curOffset,
+                       hdrInfo   * infoPtr);
 #endif
 
 /*****************************************************************************

diff --git a/src/coreclr/inc/gcdecoder.cpp b/src/coreclr/inc/gcdecoder.cpp
@@ -197,7 +197,7 @@ PTR_CBYTE FASTCALL decodeHeader(PTR_CBYTE table, UINT32 version, InfoHdr* header
                 header->syncStartOffset ^= HAS_SYNC_OFFSET;
                 break;
             case FLIP_REV_PINVOKE_FRAME:
-                header->revPInvokeOffset ^= HAS_REV_PINVOKE_FRAME_OFFSET;
+                header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET ? HAS_REV_PINVOKE_FRAME_OFFSET : INVALID_REV_PINVOKE_OFFSET;
                 break;
 
             case NEXT_OPCODE:

diff --git a/src/coreclr/inc/gcinfotypes.h b/src/coreclr/inc/gcinfotypes.h
@@ -421,8 +421,10 @@ enum infoHdrAdjust2 {
 #define HAS_UNTRACKED               ((unsigned int) -1)
 #define HAS_VARPTR                  ((unsigned int) -1)
 
-#define INVALID_REV_PINVOKE_OFFSET   0
-#define HAS_REV_PINVOKE_FRAME_OFFSET ((unsigned int) -1)
+// 0 is a valid offset for the Reverse P/Invoke block
+// So use -1 as the sentinel for invalid and -2 as the sentinel for present.
+#define INVALID_REV_PINVOKE_OFFSET   ((unsigned int) -1)
+#define HAS_REV_PINVOKE_FRAME_OFFSET ((unsigned int) -2)
 // 0 is not a valid offset for EBP-frames as all locals are at a negative offset
 // For ESP frames, the cookie is above (at a higher address than) the buffers,
 // and so cannot be at offset 0.

diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp
@@ -1623,6 +1623,13 @@ size_t GCInfo::gcInfoBlockHdrSave(
 #endif
 
     header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET;
+    if (compiler->opts.IsReversePInvoke())
+    {
+        assert(compiler->lvaReversePInvokeFrameVar != BAD_VAR_NUM);
+        int stkOffs              = compiler->lvaTable[compiler->lvaReversePInvokeFrameVar].GetStackOffset();
+        header->revPInvokeOffset = compiler->isFramePointerUsed() ? -stkOffs : stkOffs;
+        assert(header->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET);
+    }
 
     assert((compiler->compArgSize & 0x3) == 0);
 
@@ -1726,6 +1733,15 @@ size_t GCInfo::gcInfoBlockHdrSave(
         }
     }
 
+    if (header->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET)
+    {
+        assert(mask == 0 || state.revPInvokeOffset == HAS_REV_PINVOKE_FRAME_OFFSET);
+        unsigned offset = header->revPInvokeOffset;
+        unsigned sz     = encodeUnsigned(mask ? dest : NULL, offset);
+        size += sz;
+        dest += (sz & mask);
+    }
+
     if (header->epilogCount)
     {
         /* Generate table unless one epilog at the end of the method */

diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp
@@ -157,9 +157,9 @@ __forceinline int decodeSigned(PTR_CBYTE& src)
  *  computation of PrologOffs/EpilogOffs.
  *  Returns the size of the header (number of bytes decoded).
  */
-static size_t   DecodeGCHdrInfo(GCInfoToken gcInfoToken,
-                                unsigned    curOffset,
-                                hdrInfo   * infoPtr)
+size_t DecodeGCHdrInfo(GCInfoToken gcInfoToken,
+                       unsigned    curOffset,
+                       hdrInfo   * infoPtr)
 {
     CONTRACTL {
         NOTHROW;
@@ -5892,6 +5892,12 @@ bool EECodeManager::GetReturnAddressHijackInfo(GCInfoToken gcInfoToken, ReturnKi
 
     DecodeGCHdrInfo(gcInfoToken, 0, &info);
 
+    if (info.revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET)
+    {
+        // Hijacking of UnmanagedCallersOnly method is not allowed
+        return false;
+    }
+
     *returnKind = info.returnKind;
     return true;
 #else // !USE_GC_INFO_DECODER

diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp
@@ -1202,12 +1202,12 @@ lExit: ;
 
     if ((ExceptionContinueSearch == returnDisposition))
     {
-#ifdef USE_GC_INFO_DECODER
         if (dwExceptionFlags & EXCEPTION_UNWINDING)
         {
             EECodeInfo codeInfo(pDispatcherContext->ControlPc);
             if (codeInfo.IsValid())
             {
+#ifdef USE_GC_INFO_DECODER
                 GcInfoDecoder gcInfoDecoder(codeInfo.GetGCInfoToken(), DECODE_REVERSE_PINVOKE_VAR);
                 if (gcInfoDecoder.GetReversePInvokeFrameStackSlot() != NO_REVERSE_PINVOKE_FRAME)
                 {
@@ -1216,9 +1216,21 @@ lExit: ;
                     bool fIsSO = pExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW;
                     CleanUpForSecondPass(pThread, fIsSO, (void*)MemoryStackFp, (void*)MemoryStackFp);
                 }
+#else // USE_GC_INFO_DECODER
+                hdrInfo gcHdrInfo;
+
+                DecodeGCHdrInfo(gcInfoToken, 0, &gcHdrInfo);
+
+                if (gcHdrInfo.revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET)
+                {
+                    // Exception is being propagated from a method marked UnmanagedCallersOnlyAttribute into its native caller.
+                    // The explicit frame chain needs to be unwound at this boundary.
+                    bool fIsSO = pExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW;
+                    CleanUpForSecondPass(pThread, fIsSO, (void*)MemoryStackFp, (void*)MemoryStackFp);
+                }
+#endif // USE_GC_INFO_DECODER
             }
         }
-#endif // USE_GC_INFO_DECODER
 
         GCX_PREEMP_NO_DTOR();
     }
@@ -4571,6 +4583,22 @@ VOID DECLSPEC_NORETURN UnwindManagedExceptionPass1(PAL_SEHException& ex, CONTEXT
             CrashDumpAndTerminateProcess(1);
             UNREACHABLE();
         }
+#else // USE_GC_INFO_DECODER
+        hdrInfo gcHdrInfo;
+
+        DecodeGCHdrInfo(gcInfoToken, 0, &gcHdrInfo);
+
+        if (gcHdrInfo.revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET)
+        {
+            // Propagating exception from a method marked by UnmanagedCallersOnly attribute is prohibited on Unix
+            if (!GetThread()->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
+            {
+                LONG disposition = InternalUnhandledExceptionFilter_Worker(&ex.ExceptionPointers);
+                _ASSERTE(disposition == EXCEPTION_CONTINUE_SEARCH);
+            }
+            CrashDumpAndTerminateProcess(1);
+            UNREACHABLE();
+        }
 #endif // USE_GC_INFO_DECODER
 
         // Check whether we are crossing managed-to-native boundary