From e95568b78ddead0173339ca98df6cd92131ceb62 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 22 Aug 2012 11:34:32 +0200 Subject: [PATCH 1/2] Handle locked pages more robustly (Fixes issue #1462) Memory locks do not stack, that is, pages which have been locked several times by calls to mlock() will be unlocked by a single call to munlock(). This can result in keying material ending up in swap when those functions are used naively. In this commit a class "LockedPageManager" is added that simulates stacking memory locks by keeping a counter per page. --- src/allocators.h | 165 +++++++++++++++++++++++++++++++---- src/test/allocator_tests.cpp | 115 ++++++++++++++++++++++++ src/util.cpp | 2 + 3 files changed, 267 insertions(+), 15 deletions(-) create mode 100644 src/test/allocator_tests.cpp diff --git a/src/allocators.h b/src/allocators.h index ddeabc48..99afa10c 100644 --- a/src/allocators.h +++ b/src/allocators.h @@ -7,6 +7,8 @@ #include #include +#include +#include #ifdef WIN32 #ifdef _WIN32_WINNT @@ -22,23 +24,156 @@ // Note that VirtualLock does not provide this as a guarantee on Windows, // but, in practice, memory that has been VirtualLock'd almost never gets written to // the pagefile except in rare circumstances where memory is extremely low. -#define mlock(p, n) VirtualLock((p), (n)); -#define munlock(p, n) VirtualUnlock((p), (n)); #else #include -#include -/* This comes from limits.h if it's not defined there set a sane default */ -#ifndef PAGESIZE -#include -#define PAGESIZE sysconf(_SC_PAGESIZE) +#include // for PAGESIZE +#include // for sysconf #endif -#define mlock(a,b) \ - mlock(((void *)(((size_t)(a)) & (~((PAGESIZE)-1)))),\ - (((((size_t)(a)) + (b) - 1) | ((PAGESIZE) - 1)) + 1) - (((size_t)(a)) & (~((PAGESIZE) - 1)))) -#define munlock(a,b) \ - munlock(((void *)(((size_t)(a)) & (~((PAGESIZE)-1)))),\ - (((((size_t)(a)) + (b) - 1) | ((PAGESIZE) - 1)) + 1) - (((size_t)(a)) & (~((PAGESIZE) - 1)))) + +/** + * Thread-safe class to keep track of locked (ie, non-swappable) memory pages. + * + * Memory locks do not stack, that is, pages which have been locked several times by calls to mlock() + * will be unlocked by a single call to munlock(). This can result in keying material ending up in swap when + * those functions are used naively. This class simulates stacking memory locks by keeping a counter per page. + * + * @note By using a map from each page base address to lock count, this class is optimized for + * small objects that span up to a few pages, mostly smaller than a page. To support large allocations, + * something like an interval tree would be the preferred data structure. + */ +template class LockedPageManagerBase +{ +public: + LockedPageManagerBase(size_t page_size): + page_size(page_size) + { + // Determine bitmask for extracting page from address + assert(!(page_size & (page_size-1))); // size must be power of two + page_mask = ~(page_size - 1); + } + + // For all pages in affected range, increase lock count + void LockRange(void *p, size_t size) + { + boost::mutex::scoped_lock lock(mutex); + if(!size) return; + const size_t base_addr = reinterpret_cast(p); + const size_t start_page = base_addr & page_mask; + const size_t end_page = (base_addr + size - 1) & page_mask; + for(size_t page = start_page; page <= end_page; page += page_size) + { + Histogram::iterator it = histogram.find(page); + if(it == histogram.end()) // Newly locked page + { + locker.Lock(reinterpret_cast(page), page_size); + histogram.insert(std::make_pair(page, 1)); + } + else // Page was already locked; increase counter + { + it->second += 1; + } + } + } + + // For all pages in affected range, decrease lock count + void UnlockRange(void *p, size_t size) + { + boost::mutex::scoped_lock lock(mutex); + if(!size) return; + const size_t base_addr = reinterpret_cast(p); + const size_t start_page = base_addr & page_mask; + const size_t end_page = (base_addr + size - 1) & page_mask; + for(size_t page = start_page; page <= end_page; page += page_size) + { + Histogram::iterator it = histogram.find(page); + assert(it != histogram.end()); // Cannot unlock an area that was not locked + // Decrease counter for page, when it is zero, the page will be unlocked + it->second -= 1; + if(it->second == 0) // Nothing on the page anymore that keeps it locked + { + // Unlock page and remove the count from histogram + locker.Unlock(reinterpret_cast(page), page_size); + histogram.erase(it); + } + } + } + + // Get number of locked pages for diagnostics + int GetLockedPageCount() + { + boost::mutex::scoped_lock lock(mutex); + return histogram.size(); + } + +private: + Locker locker; + boost::mutex mutex; + size_t page_size, page_mask; + // map of page base address to lock count + typedef std::map Histogram; + Histogram histogram; +}; + +/** Determine system page size in bytes */ +static inline size_t GetSystemPageSize() +{ + size_t page_size; +#if defined(WIN32) + SYSTEM_INFO sSysInfo; + GetSystemInfo(&sSysInfo); + page_size = sSysInfo.dwPageSize; +#elif defined(PAGESIZE) // defined in limits.h + page_size = PAGESIZE; +#else // assume some POSIX OS + page_size = sysconf(_SC_PAGESIZE); +#endif + return page_size; +} + +/** + * OS-dependent memory page locking/unlocking. + * Defined as policy class to make stubbing for test possible. + */ +class MemoryPageLocker +{ +public: + /** Lock memory pages. + * addr and len must be a multiple of the system page size + */ + bool Lock(const void *addr, size_t len) + { +#ifdef WIN32 + return VirtualLock(const_cast(addr), len); +#else + return mlock(addr, len) == 0; +#endif + } + /** Unlock memory pages. + * addr and len must be a multiple of the system page size + */ + bool Unlock(const void *addr, size_t len) + { +#ifdef WIN32 + return VirtualUnlock(const_cast(addr), len); +#else + return munlock(addr, len) == 0; #endif + } +}; + +/** + * Singleton class to keep track of locked (ie, non-swappable) memory pages, for use in + * std::allocator templates. + */ +class LockedPageManager: public LockedPageManagerBase +{ +public: + static LockedPageManager instance; // instantiated in util.cpp +private: + LockedPageManager(): + LockedPageManagerBase(GetSystemPageSize()) + {} +}; // // Allocator that locks its contents from being paged @@ -69,7 +204,7 @@ struct secure_allocator : public std::allocator T *p; p = std::allocator::allocate(n, hint); if (p != NULL) - mlock(p, sizeof(T) * n); + LockedPageManager::instance.LockRange(p, sizeof(T) * n); return p; } @@ -78,7 +213,7 @@ struct secure_allocator : public std::allocator if (p != NULL) { memset(p, 0, sizeof(T) * n); - munlock(p, sizeof(T) * n); + LockedPageManager::instance.UnlockRange(p, sizeof(T) * n); } std::allocator::deallocate(p, n); } diff --git a/src/test/allocator_tests.cpp b/src/test/allocator_tests.cpp new file mode 100644 index 00000000..d5cb8e81 --- /dev/null +++ b/src/test/allocator_tests.cpp @@ -0,0 +1,115 @@ +#include + +#include "init.h" +#include "main.h" +#include "util.h" + +BOOST_AUTO_TEST_SUITE(allocator_tests) + +// Dummy memory page locker for platform independent tests +static const void *last_lock_addr, *last_unlock_addr; +static size_t last_lock_len, last_unlock_len; +class TestLocker +{ +public: + bool Lock(const void *addr, size_t len) + { + last_lock_addr = addr; + last_lock_len = len; + return true; + } + bool Unlock(const void *addr, size_t len) + { + last_unlock_addr = addr; + last_unlock_len = len; + return true; + } +}; + +BOOST_AUTO_TEST_CASE(test_LockedPageManagerBase) +{ + const size_t test_page_size = 4096; + LockedPageManagerBase lpm(test_page_size); + size_t addr; + last_lock_addr = last_unlock_addr = 0; + last_lock_len = last_unlock_len = 0; + + /* Try large number of small objects */ + addr = 0; + for(int i=0; i<1000; ++i) + { + lpm.LockRange(reinterpret_cast(addr), 33); + addr += 33; + } + /* Try small number of page-sized objects, straddling two pages */ + addr = test_page_size*100 + 53; + for(int i=0; i<100; ++i) + { + lpm.LockRange(reinterpret_cast(addr), test_page_size); + addr += test_page_size; + } + /* Try small number of page-sized objects aligned to exactly one page */ + addr = test_page_size*300; + for(int i=0; i<100; ++i) + { + lpm.LockRange(reinterpret_cast(addr), test_page_size); + addr += test_page_size; + } + /* one very large object, straddling pages */ + lpm.LockRange(reinterpret_cast(test_page_size*600+1), test_page_size*500); + BOOST_CHECK(last_lock_addr == reinterpret_cast(test_page_size*(600+500))); + /* one very large object, page aligned */ + lpm.LockRange(reinterpret_cast(test_page_size*1200), test_page_size*500-1); + BOOST_CHECK(last_lock_addr == reinterpret_cast(test_page_size*(1200+500-1))); + + BOOST_CHECK(lpm.GetLockedPageCount() == ( + (1000*33+test_page_size-1)/test_page_size + // small objects + 101 + 100 + // page-sized objects + 501 + 500)); // large objects + BOOST_CHECK((last_lock_len & (test_page_size-1)) == 0); // always lock entire pages + BOOST_CHECK(last_unlock_len == 0); // nothing unlocked yet + + /* And unlock again */ + addr = 0; + for(int i=0; i<1000; ++i) + { + lpm.UnlockRange(reinterpret_cast(addr), 33); + addr += 33; + } + addr = test_page_size*100 + 53; + for(int i=0; i<100; ++i) + { + lpm.UnlockRange(reinterpret_cast(addr), test_page_size); + addr += test_page_size; + } + addr = test_page_size*300; + for(int i=0; i<100; ++i) + { + lpm.UnlockRange(reinterpret_cast(addr), test_page_size); + addr += test_page_size; + } + lpm.UnlockRange(reinterpret_cast(test_page_size*600+1), test_page_size*500); + lpm.UnlockRange(reinterpret_cast(test_page_size*1200), test_page_size*500-1); + + /* Check that everything is released */ + BOOST_CHECK(lpm.GetLockedPageCount() == 0); + + /* A few and unlocks of size zero (should have no effect) */ + addr = 0; + for(int i=0; i<1000; ++i) + { + lpm.LockRange(reinterpret_cast(addr), 0); + addr += 1; + } + BOOST_CHECK(lpm.GetLockedPageCount() == 0); + addr = 0; + for(int i=0; i<1000; ++i) + { + lpm.UnlockRange(reinterpret_cast(addr), 0); + addr += 1; + } + BOOST_CHECK(lpm.GetLockedPageCount() == 0); + BOOST_CHECK((last_unlock_len & (test_page_size-1)) == 0); // always unlock entire pages +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util.cpp b/src/util.cpp index d6d9a368..461f42d1 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -86,6 +86,8 @@ void locking_callback(int mode, int i, const char* file, int line) } } +LockedPageManager LockedPageManager::instance; + // Init class CInit { From 0b886ad1bda23c01a68d0ebeac9ec480ca5690fd Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 22 Aug 2012 13:39:21 +0200 Subject: [PATCH 2/2] Make CCrypter use LockedPageManager to manage locked pages Replace direct calls to mlock. Also, change the class to lock the memory areas in the constructor and unlock them again in the destructor. This makes sure that locked pages won't leak. --- src/crypter.cpp | 12 ------------ src/crypter.h | 11 +++++++++-- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/crypter.cpp b/src/crypter.cpp index 411a1ee4..181b8fa0 100644 --- a/src/crypter.cpp +++ b/src/crypter.cpp @@ -17,12 +17,6 @@ bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::v if (nRounds < 1 || chSalt.size() != WALLET_CRYPTO_SALT_SIZE) return false; - // Try to keep the key data out of swap (and be a bit over-careful to keep the IV that we don't even use out of swap) - // Note that this does nothing about suspend-to-disk (which will put all our key data on disk) - // Note as well that at no point in this program is any attempt made to prevent stealing of keys by reading the memory of the running process. - mlock(&chKey[0], sizeof chKey); - mlock(&chIV[0], sizeof chIV); - int i = 0; if (nDerivationMethod == 0) i = EVP_BytesToKey(EVP_aes_256_cbc(), EVP_sha512(), &chSalt[0], @@ -44,12 +38,6 @@ bool CCrypter::SetKey(const CKeyingMaterial& chNewKey, const std::vector