diff options
author | Pieter Wuille <pieter.wuille@gmail.com> | 2012-08-24 04:38:57 -0700 |
---|---|---|
committer | Pieter Wuille <pieter.wuille@gmail.com> | 2012-08-24 04:38:57 -0700 |
commit | af1c6b93b70d2e404c5fc7ed4e1af084a0231eb9 (patch) | |
tree | c60b532da0f15f8e9cb6d46fe21f6c1f5f9f9064 | |
parent | c68c4bc7a45529735c868428632906d3c20a175a (diff) | |
parent | 0b886ad1bda23c01a68d0ebeac9ec480ca5690fd (diff) | |
download | bitcoin-af1c6b93b70d2e404c5fc7ed4e1af084a0231eb9.tar.xz |
Merge pull request #1699 from laanwj/2012_08_securealloc
Handle locked pages more robustly (Fixes issue #1462)
-rw-r--r-- | src/allocators.h | 165 | ||||
-rw-r--r-- | src/crypter.cpp | 12 | ||||
-rw-r--r-- | src/crypter.h | 11 | ||||
-rw-r--r-- | src/test/allocator_tests.cpp | 115 | ||||
-rw-r--r-- | src/util.cpp | 2 |
5 files changed, 276 insertions, 29 deletions
diff --git a/src/allocators.h b/src/allocators.h index ddeabc48c5..99afa10c25 100644 --- a/src/allocators.h +++ b/src/allocators.h @@ -7,6 +7,8 @@ #include <string.h> #include <string> +#include <boost/thread/mutex.hpp> +#include <map> #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 <sys/mman.h> -#include <limits.h> -/* This comes from limits.h if it's not defined there set a sane default */ -#ifndef PAGESIZE -#include <unistd.h> -#define PAGESIZE sysconf(_SC_PAGESIZE) +#include <limits.h> // for PAGESIZE +#include <unistd.h> // 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 Locker> 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<size_t>(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<void*>(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<size_t>(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<void*>(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<size_t,int> 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<void*>(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<void*>(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<MemoryPageLocker> +{ +public: + static LockedPageManager instance; // instantiated in util.cpp +private: + LockedPageManager(): + LockedPageManagerBase<MemoryPageLocker>(GetSystemPageSize()) + {} +}; // // Allocator that locks its contents from being paged @@ -69,7 +204,7 @@ struct secure_allocator : public std::allocator<T> T *p; p = std::allocator<T>::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<T> if (p != NULL) { memset(p, 0, sizeof(T) * n); - munlock(p, sizeof(T) * n); + LockedPageManager::instance.UnlockRange(p, sizeof(T) * n); } std::allocator<T>::deallocate(p, n); } diff --git a/src/crypter.cpp b/src/crypter.cpp index 411a1ee4c1..181b8fa00a 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<unsigne if (chNewKey.size() != WALLET_CRYPTO_KEY_SIZE || chNewIV.size() != WALLET_CRYPTO_KEY_SIZE) return false; - // Try to keep the key data 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); - memcpy(&chKey[0], &chNewKey[0], sizeof chKey); memcpy(&chIV[0], &chNewIV[0], sizeof chIV); diff --git a/src/crypter.h b/src/crypter.h index d1bdb92c91..04538a3fa5 100644 --- a/src/crypter.h +++ b/src/crypter.h @@ -78,19 +78,26 @@ public: { memset(&chKey, 0, sizeof chKey); memset(&chIV, 0, sizeof chIV); - munlock(&chKey, sizeof chKey); - munlock(&chIV, sizeof chIV); fKeySet = false; } CCrypter() { fKeySet = 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. + LockedPageManager::instance.LockRange(&chKey[0], sizeof chKey); + LockedPageManager::instance.LockRange(&chIV[0], sizeof chIV); } ~CCrypter() { CleanKey(); + + LockedPageManager::instance.UnlockRange(&chKey[0], sizeof chKey); + LockedPageManager::instance.UnlockRange(&chIV[0], sizeof chIV); } }; diff --git a/src/test/allocator_tests.cpp b/src/test/allocator_tests.cpp new file mode 100644 index 0000000000..d5cb8e8101 --- /dev/null +++ b/src/test/allocator_tests.cpp @@ -0,0 +1,115 @@ +#include <boost/test/unit_test.hpp> + +#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<TestLocker> 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<void*>(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<void*>(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<void*>(addr), test_page_size); + addr += test_page_size; + } + /* one very large object, straddling pages */ + lpm.LockRange(reinterpret_cast<void*>(test_page_size*600+1), test_page_size*500); + BOOST_CHECK(last_lock_addr == reinterpret_cast<void*>(test_page_size*(600+500))); + /* one very large object, page aligned */ + lpm.LockRange(reinterpret_cast<void*>(test_page_size*1200), test_page_size*500-1); + BOOST_CHECK(last_lock_addr == reinterpret_cast<void*>(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<void*>(addr), 33); + addr += 33; + } + addr = test_page_size*100 + 53; + for(int i=0; i<100; ++i) + { + lpm.UnlockRange(reinterpret_cast<void*>(addr), test_page_size); + addr += test_page_size; + } + addr = test_page_size*300; + for(int i=0; i<100; ++i) + { + lpm.UnlockRange(reinterpret_cast<void*>(addr), test_page_size); + addr += test_page_size; + } + lpm.UnlockRange(reinterpret_cast<void*>(test_page_size*600+1), test_page_size*500); + lpm.UnlockRange(reinterpret_cast<void*>(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<void*>(addr), 0); + addr += 1; + } + BOOST_CHECK(lpm.GetLockedPageCount() == 0); + addr = 0; + for(int i=0; i<1000; ++i) + { + lpm.UnlockRange(reinterpret_cast<void*>(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 d6d9a368f0..461f42d177 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 { |