From 90fd3a315a5300c4c72086e541d35a73694e176b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=9D=B4=EC=B6=98=EC=84=9D/Developer=20Experience=20Lab?= =?UTF-8?q?=28S/W=EC=84=BC=ED=84=B0=29/Senior=20Engineer/=EC=82=BC?= =?UTF-8?q?=EC=84=B1=EC=A0=84=EC=9E=90?= Date: Thu, 24 Aug 2017 07:27:23 +0900 Subject: [PATCH 10/23] Use 'udf 0xff' instead of 'bkpt 0xbe' as a poison (#13) * Fill freed loader heap chunk with non-zero value (#12731) * Add FEATURE_LOADER_HEAP_GUARD feature * Invoke memset only for reclaimed regions * Enable FEATURE_LOADER_HEAP_GUARD by default * Insert trap inside UMEntryThunk::Terminate * Make all exectuable heaps not to zero-initialize itself Use fZeroInit (instead of fMakeRelazed) * Add comment * Revert unnecessary changes * Add and use 'Poison' method to insert a trap * Do NOT invoke FlushInstructionCache * Update comment * Add comment on ARM Poisoning instruction * Use X86_INSTR_INT3 instead of 0xCC * Use 'udf 0xff' instead of 'bkpt 0xbe' as a poison (#13152) --- src/inc/loaderheap.h | 23 +++++++++++++---------- src/utilcode/loaderheap.cpp | 35 ++++++++++++++++++++++++++--------- src/vm/amd64/cgenamd64.cpp | 13 +++++++++++++ src/vm/amd64/cgencpu.h | 1 + src/vm/arm/cgencpu.h | 1 + src/vm/arm/stubs.cpp | 6 ++++++ src/vm/arm64/cgencpu.h | 1 + src/vm/arm64/stubs.cpp | 4 ++++ src/vm/dllimportcallback.cpp | 12 +++--------- src/vm/dllimportcallback.h | 4 ---- src/vm/i386/cgencpu.h | 1 + src/vm/i386/cgenx86.cpp | 7 +++++++ src/vm/loaderallocator.cpp | 4 +++- 13 files changed, 79 insertions(+), 33 deletions(-) diff --git a/src/inc/loaderheap.h b/src/inc/loaderheap.h index 7d4c48f..4333505 100644 --- a/src/inc/loaderheap.h +++ b/src/inc/loaderheap.h @@ -217,7 +217,7 @@ private: size_t * m_pPrivatePerfCounter_LoaderBytes; - DWORD m_flProtect; + DWORD m_Options; LoaderHeapFreeBlock *m_pFirstFreeBlock; @@ -288,7 +288,8 @@ protected: SIZE_T dwReservedRegionSize, size_t *pPrivatePerfCounter_LoaderBytes = NULL, RangeList *pRangeList = NULL, - BOOL fMakeExecutable = FALSE); + BOOL fMakeExecutable = FALSE, + BOOL fZeroInit = TRUE); ~UnlockedLoaderHeap(); #endif @@ -398,10 +399,8 @@ public: return m_dwTotalAlloc; } - BOOL IsExecutable() - { - return (PAGE_EXECUTE_READWRITE == m_flProtect); - } + BOOL IsExecutable(); + BOOL IsZeroInit(); public: @@ -447,14 +446,16 @@ public: DWORD dwCommitBlockSize, size_t *pPrivatePerfCounter_LoaderBytes = NULL, RangeList *pRangeList = NULL, - BOOL fMakeExecutable = FALSE + BOOL fMakeExecutable = FALSE, + BOOL fZeroInit = TRUE ) : UnlockedLoaderHeap(dwReserveBlockSize, dwCommitBlockSize, NULL, 0, pPrivatePerfCounter_LoaderBytes, pRangeList, - fMakeExecutable) + fMakeExecutable, + fZeroInit) { WRAPPER_NO_CONTRACT; m_CriticalSection = NULL; @@ -469,7 +470,8 @@ public: SIZE_T dwReservedRegionSize, size_t *pPrivatePerfCounter_LoaderBytes = NULL, RangeList *pRangeList = NULL, - BOOL fMakeExecutable = FALSE + BOOL fMakeExecutable = FALSE, + BOOL fZeroInit = TRUE ) : UnlockedLoaderHeap(dwReserveBlockSize, dwCommitBlockSize, @@ -477,7 +479,8 @@ public: dwReservedRegionSize, pPrivatePerfCounter_LoaderBytes, pRangeList, - fMakeExecutable) + fMakeExecutable, + fZeroInit) { WRAPPER_NO_CONTRACT; m_CriticalSection = NULL; diff --git a/src/utilcode/loaderheap.cpp b/src/utilcode/loaderheap.cpp index a005ac8..21aa150 100644 --- a/src/utilcode/loaderheap.cpp +++ b/src/utilcode/loaderheap.cpp @@ -10,6 +10,9 @@ #define DONOT_DEFINE_ETW_CALLBACK #include "eventtracebase.h" +#define LHF_EXECUTABLE 0x1 +#define LHF_ZEROINIT 0x2 + #ifndef DACCESS_COMPILE INDEBUG(DWORD UnlockedLoaderHeap::s_dwNumInstancesOfLoaderHeaps = 0;) @@ -903,7 +906,8 @@ UnlockedLoaderHeap::UnlockedLoaderHeap(DWORD dwReserveBlockSize, SIZE_T dwReservedRegionSize, size_t *pPrivatePerfCounter_LoaderBytes, RangeList *pRangeList, - BOOL fMakeExecutable) + BOOL fMakeExecutable, + BOOL fZeroInit) { CONTRACTL { @@ -939,10 +943,11 @@ UnlockedLoaderHeap::UnlockedLoaderHeap(DWORD dwReserveBlockSize, m_pPrivatePerfCounter_LoaderBytes = pPrivatePerfCounter_LoaderBytes; + m_Options = 0; if (fMakeExecutable) - m_flProtect = PAGE_EXECUTE_READWRITE; - else - m_flProtect = PAGE_READWRITE; + m_Options |= LHF_EXECUTABLE; + if (fZeroInit) + m_Options |= LHF_ZEROINIT; m_pFirstFreeBlock = NULL; @@ -1133,7 +1138,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit) } // Commit first set of pages, since it will contain the LoaderHeapBlock - void *pTemp = ClrVirtualAlloc(pData, dwSizeToCommit, MEM_COMMIT, m_flProtect); + void *pTemp = ClrVirtualAlloc(pData, dwSizeToCommit, MEM_COMMIT, (m_Options & LHF_EXECUTABLE) ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE); if (pTemp == NULL) { //_ASSERTE(!"Unable to ClrVirtualAlloc commit in a loaderheap"); @@ -1225,7 +1230,7 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize) dwSizeToCommit = ALIGN_UP(dwSizeToCommit, PAGE_SIZE); // Yes, so commit the desired number of reserved pages - void *pData = ClrVirtualAlloc(m_pPtrToEndOfCommittedRegion, dwSizeToCommit, MEM_COMMIT, m_flProtect); + void *pData = ClrVirtualAlloc(m_pPtrToEndOfCommittedRegion, dwSizeToCommit, MEM_COMMIT, (m_Options & LHF_EXECUTABLE) ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE); if (pData == NULL) return FALSE; @@ -1351,7 +1356,7 @@ again: // Don't fill the memory we allocated - it is assumed to be zeroed - fill the memory after it memset(pAllocatedBytes + dwRequestedSize, 0xEE, LOADER_HEAP_DEBUG_BOUNDARY); #endif - if (dwRequestedSize > 0) + if ((dwRequestedSize > 0) && (m_Options & LHF_ZEROINIT)) { _ASSERTE_MSG(pAllocatedBytes[0] == 0 && memcmp(pAllocatedBytes, pAllocatedBytes + 1, dwRequestedSize - 1) == 0, "LoaderHeap must return zero-initialized memory"); @@ -1529,7 +1534,8 @@ void UnlockedLoaderHeap::UnlockedBackoutMem(void *pMem, { // Cool. This was the last block allocated. We can just undo the allocation instead // of going to the freelist. - memset(pMem, 0, dwSize); // Must zero init this memory as AllocMem expect it + if (m_Options & LHF_ZEROINIT) + memset(pMem, 0x00, dwSize); // Fill freed region with 0 m_pAllocPtr = (BYTE*)pMem; } else @@ -1639,6 +1645,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t dwRequestedSiz ((BYTE*&)pResult) += extra; + #ifdef _DEBUG BYTE *pAllocatedBytes = (BYTE *)pResult; #if LOADER_HEAP_DEBUG_BOUNDARY > 0 @@ -1646,7 +1653,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t dwRequestedSiz memset(pAllocatedBytes + dwRequestedSize, 0xee, LOADER_HEAP_DEBUG_BOUNDARY); #endif - if (dwRequestedSize != 0) + if ((dwRequestedSize != 0) && (m_Options & LHF_ZEROINIT)) { _ASSERTE_MSG(pAllocatedBytes[0] == 0 && memcmp(pAllocatedBytes, pAllocatedBytes + 1, dwRequestedSize - 1) == 0, "LoaderHeap must return zero-initialized memory"); @@ -1766,6 +1773,16 @@ void *UnlockedLoaderHeap::UnlockedAllocMemForCode_NoThrow(size_t dwHeaderSize, s #endif // #ifndef DACCESS_COMPILE +BOOL UnlockedLoaderHeap::IsExecutable() +{ + return (m_Options & LHF_EXECUTABLE); +} + +BOOL UnlockedLoaderHeap::IsZeroInit() +{ + return (m_Options & LHF_ZEROINIT); +} + #ifdef DACCESS_COMPILE void UnlockedLoaderHeap::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) diff --git a/src/vm/amd64/cgenamd64.cpp b/src/vm/amd64/cgenamd64.cpp index 497abcd..20dca22 100644 --- a/src/vm/amd64/cgenamd64.cpp +++ b/src/vm/amd64/cgenamd64.cpp @@ -670,6 +670,19 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam) _ASSERTE(DbgIsExecutable(&m_movR10[0], &m_jmpRAX[3]-&m_movR10[0])); } +void UMEntryThunkCode::Poison() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + m_movR10[0] = X86_INSTR_INT3; +} + UMEntryThunk* UMEntryThunk::Decode(LPVOID pCallback) { LIMITED_METHOD_CONTRACT; diff --git a/src/vm/amd64/cgencpu.h b/src/vm/amd64/cgencpu.h index 2d4dce0..8a27525 100644 --- a/src/vm/amd64/cgencpu.h +++ b/src/vm/amd64/cgencpu.h @@ -466,6 +466,7 @@ struct DECLSPEC_ALIGN(8) UMEntryThunkCode BYTE m_padding2[5]; void Encode(BYTE* pTargetCode, void* pvSecretParam); + void Poison(); LPCBYTE GetEntryPoint() const { diff --git a/src/vm/arm/cgencpu.h b/src/vm/arm/cgencpu.h index 181d5f1..6f128f6 100644 --- a/src/vm/arm/cgencpu.h +++ b/src/vm/arm/cgencpu.h @@ -988,6 +988,7 @@ struct DECLSPEC_ALIGN(4) UMEntryThunkCode TADDR m_pvSecretParam; void Encode(BYTE* pTargetCode, void* pvSecretParam); + void Poison(); LPCBYTE GetEntryPoint() const { diff --git a/src/vm/arm/stubs.cpp b/src/vm/arm/stubs.cpp index 3088761..c832911 100644 --- a/src/vm/arm/stubs.cpp +++ b/src/vm/arm/stubs.cpp @@ -2523,6 +2523,12 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam) FlushInstructionCache(GetCurrentProcess(),&m_code,sizeof(m_code)); } +void UMEntryThunkCode::Poison() +{ + // Insert 'udf 0xff' at the entry point + m_code[0] = 0xdeff; +} + ///////////////////////////// UNIMPLEMENTED ////////////////////////////////// #ifndef DACCESS_COMPILE diff --git a/src/vm/arm64/cgencpu.h b/src/vm/arm64/cgencpu.h index d8bbcf7..5c522c5 100644 --- a/src/vm/arm64/cgencpu.h +++ b/src/vm/arm64/cgencpu.h @@ -481,6 +481,7 @@ struct DECLSPEC_ALIGN(16) UMEntryThunkCode TADDR m_pvSecretParam; void Encode(BYTE* pTargetCode, void* pvSecretParam); + void Poison(); LPCBYTE GetEntryPoint() const { diff --git a/src/vm/arm64/stubs.cpp b/src/vm/arm64/stubs.cpp index 40d2749..df2124d 100644 --- a/src/vm/arm64/stubs.cpp +++ b/src/vm/arm64/stubs.cpp @@ -1244,6 +1244,10 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam) FlushInstructionCache(GetCurrentProcess(),&m_code,sizeof(m_code)); } +void UMEntryThunkCode::Poison() +{ + +} #ifdef PROFILING_SUPPORTED #include "proftoeeinterfaceimpl.h" diff --git a/src/vm/dllimportcallback.cpp b/src/vm/dllimportcallback.cpp index 90c01a4..8684c12 100644 --- a/src/vm/dllimportcallback.cpp +++ b/src/vm/dllimportcallback.cpp @@ -1111,13 +1111,8 @@ UMEntryThunk* UMEntryThunk::CreateUMEntryThunk() UMEntryThunk * p; -#ifdef FEATURE_WINDOWSPHONE // On the phone, use loader heap to save memory commit of regular executable heap p = (UMEntryThunk *)(void *)SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->AllocMem(S_SIZE_T(sizeof(UMEntryThunk))); -#else - p = new (executable) UMEntryThunk; - memset (p, 0, sizeof(*p)); -#endif RETURN p; } @@ -1126,11 +1121,10 @@ void UMEntryThunk::Terminate() { WRAPPER_NO_CONTRACT; -#ifdef FEATURE_WINDOWSPHONE + _ASSERTE(!SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->IsZeroInit()); + m_code.Poison(); + SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->BackoutMem(this, sizeof(UMEntryThunk)); -#else - DeleteExecutable(this); -#endif } VOID UMEntryThunk::FreeUMEntryThunk(UMEntryThunk* p) diff --git a/src/vm/dllimportcallback.h b/src/vm/dllimportcallback.h index af2a0b1..e79c5f0 100644 --- a/src/vm/dllimportcallback.h +++ b/src/vm/dllimportcallback.h @@ -326,10 +326,6 @@ public: { DestroyLongWeakHandle(GetObjectHandle()); } - -#ifdef _DEBUG - FillMemory(this, sizeof(*this), 0xcc); -#endif } void Terminate(); diff --git a/src/vm/i386/cgencpu.h b/src/vm/i386/cgencpu.h index ff76d99..e4a623b 100644 --- a/src/vm/i386/cgencpu.h +++ b/src/vm/i386/cgencpu.h @@ -504,6 +504,7 @@ struct DECLSPEC_ALIGN(4) UMEntryThunkCode const BYTE * m_execstub; // pointer to destination code // make sure the backpatched portion is dword aligned. void Encode(BYTE* pTargetCode, void* pvSecretParam); + void Poison(); LPCBYTE GetEntryPoint() const { diff --git a/src/vm/i386/cgenx86.cpp b/src/vm/i386/cgenx86.cpp index 0a276c0..ca81bb7 100644 --- a/src/vm/i386/cgenx86.cpp +++ b/src/vm/i386/cgenx86.cpp @@ -1587,6 +1587,13 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam) FlushInstructionCache(GetCurrentProcess(),GetEntryPoint(),sizeof(UMEntryThunkCode)); } +void UMEntryThunkCode::Poison() +{ + LIMITED_METHOD_CONTRACT; + + m_movEAX = X86_INSTR_INT3; +} + UMEntryThunk* UMEntryThunk::Decode(LPVOID pCallback) { LIMITED_METHOD_CONTRACT; diff --git a/src/vm/loaderallocator.cpp b/src/vm/loaderallocator.cpp index 70c8cab..5a3f8f5 100644 --- a/src/vm/loaderallocator.cpp +++ b/src/vm/loaderallocator.cpp @@ -1005,7 +1005,9 @@ void LoaderAllocator::Init(BaseDomain *pDomain, BYTE *pExecutableHeapMemory) dwExecutableHeapReserveSize, LOADERHEAP_PROFILE_COUNTER, NULL, - TRUE /* Make heap executable */); + TRUE /* Make heap executable */, + FALSE /* Disable zero-initialization (needed by UMEntryThunkCode::Poison) */ + ); initReservedMem += dwExecutableHeapReserveSize; } -- 1.9.1