Merge #15117: Fix invalid memory write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked

ca126d4 Fix out-of-bounds write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked (practicalswift)

Pull request description:

  `mmap(...)` returns `MAP_FAILED` (`(void *) -1`) in case of allocation failure.

  `PosixLockedPageAllocator::AllocateLocked(...)` did not check for allocation failures prior to this PR.

  Instead the invalid memory address `(void *) -1` (`0xffffffffffffffff`) was passed to the caller as if it was a valid address.

  After some operations the address is wrapped around from `0xffffffffffffffff` to `0x00000003ffdf` (`0xffffffffffffffff + 262112 == 0x00000003ffdf`);

  The resulting address `0x00000003ffdf` is then written to.

  Before this patch (with failing `mmap` call):

  ```
  $ src/bitcoind
  …
  2019-01-06T16:28:14Z Using the 'sse4(1way),sse41(4way)' SHA256 implementation
  2019-01-06T16:28:14Z Using RdRand as an additional entropy source
  Segmentation fault (core dumped)
  ```

  Before this patch (under `valgrind` with failing `mmap` call):

  ```
  $ valgrind src/bitcoind
  …
  2019-01-06T16:28:51Z Using the 'sse4(1way),sse41(4way)' SHA256 implementation
  ==17812== Invalid write of size 1
  ==17812==    at 0x500B7E: void __gnu_cxx::new_allocator<unsigned char>::construct<unsigned char>(unsigned char*) (new_allocator.h:136)
  ==17812==    by 0x500B52: _ZNSt16allocator_traitsI16secure_allocatorIhEE12_S_constructIhJEEENSt9enable_ifIXsr6__and_INS2_18__construct_helperIT_JDpT0_EE4typeEEE5valueEvE4typeERS1_PS6_DpOS7_ (alloc_traits.h:243)
  ==17812==    by 0x500B22: _ZNSt16allocator_traitsI16secure_allocatorIhEE9constructIhJEEEDTcl12_S_constructfp_fp0_spclsr3stdE7forwardIT0_Efp1_EEERS1_PT_DpOS4_ (alloc_traits.h:344)
  ==17812==    by 0x500982: unsigned char* std::__uninitialized_default_n_a<unsigned char*, unsigned long, secure_allocator<unsigned char> >(unsigned char*, unsigned long, secure_allocator<unsigned char>&) (stl_uninitialized.h:631)
  ==17812==    by 0x60BFC2: std::vector<unsigned char, secure_allocator<unsigned char> >::_M_default_initialize(unsigned long) (stl_vector.h:1347)
  ==17812==    by 0x60BD86: std::vector<unsigned char, secure_allocator<unsigned char> >::vector(unsigned long, secure_allocator<unsigned char> const&) (stl_vector.h:285)
  ==17812==    by 0x60BB55: ECC_Start() (key.cpp:351)
  ==17812==    by 0x16AC90: AppInitSanityChecks() (init.cpp:1162)
  ==17812==    by 0x15BAC9: AppInit(int, char**) (bitcoind.cpp:138)
  ==17812==    by 0x15B6C8: main (bitcoind.cpp:201)
  ==17812==  Address 0x3ffdf is not stack'd, malloc'd or (recently) free'd
  …
  Segmentation fault (core dumped)
  ```

  After this patch (with failing `mmap` call):

  ```
  $ src/bitcoind
  …
  2019-01-06T15:50:18Z Using the 'sse4(1way),sse41(4way)' SHA256 implementation
  2019-01-06T15:50:18Z Using RdRand as an additional entropy source
  2019-01-06T15:50:18Z

  ************************
  EXCEPTION: St9bad_alloc
  std::bad_alloc
  bitcoin in AppInit()

  ************************
  EXCEPTION: St9bad_alloc
  std::bad_alloc
  bitcoin in AppInit()

  2019-01-06T15:50:18Z Shutdown: In progress...
  2019-01-06T15:50:18Z Shutdown: done
  ```

  To simulate the failing `mmap` call apply the following to `master`:

  ```diff
  diff --git a/src/support/lockedpool.cpp b/src/support/lockedpool.cpp
  index 8d577cf..ce79e569b 100644
  --- a/src/support/lockedpool.cpp
  +++ b/src/support/lockedpool.cpp
  @@ -247,7 +247,8 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
   {
       void *addr;
       len = align_up(len, page_size);
  -    addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
  +    // addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
  +    addr = MAP_FAILED;
       if (addr) {
           *lockingSuccess = mlock(addr, len) == 0;
       }
  ```

Tree-SHA512: 66947f5fc0fbb19afb3e1edbd51df07df9d16b77018cff3d48d30f378a53d6a0dc62bc36622b3966b7e374e61edbcca114ef4ac8ae8d725022c1a597edcbf7c7

Author: laanwj

src/support/allocators/secure.h

@@ -40,7 +40,11 @@ struct secure_allocator : public std::allocator<T> {
 
     T* allocate(std::size_t n, const void* hint = 0)
     {
-        return static_cast<T*>(LockedPoolManager::Instance().alloc(sizeof(T) * n));
+        T* allocation = static_cast<T*>(LockedPoolManager::Instance().alloc(sizeof(T) * n));
+        if (!allocation) {
+            throw std::bad_alloc();
+        }
+        return allocation;
     }
 
     void deallocate(T* p, std::size_t n)

src/support/lockedpool.cpp

@@ -248,6 +248,9 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
     void *addr;
     len = align_up(len, page_size);
     addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+    if (addr == MAP_FAILED) {
+        return nullptr;
+    }
     if (addr) {
         *lockingSuccess = mlock(addr, len) == 0;
     }

src/support/lockedpool.h

@@ -22,7 +22,7 @@ class LockedPageAllocator
     virtual ~LockedPageAllocator() {}
     /** Allocate and lock memory pages.
      * If len is not a multiple of the system page size, it is rounded up.
-     * Returns 0 in case of allocation failure.
+     * Returns nullptr in case of allocation failure.
      *
      * If locking the memory pages could not be accomplished it will still
      * return the memory, however the lockingSuccess flag will be false.