Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

Projects
None yet
5 participants
@practicalswift
Copy link
Member

commented Jan 6, 2019

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 --git a/src/support/lockedpool.cpp b/src/support/lockedpool.cpp
index 8d577cf52..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;
     }

@practicalswift practicalswift changed the title Fix out-of-bounds write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked Fix invalid memory write write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked Jan 6, 2019

@practicalswift practicalswift changed the title Fix invalid memory write write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked Fix invalid memory write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked Jan 6, 2019

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();

This comment has been minimized.

Copy link
@Empact

Empact Jan 7, 2019

Member

Maybe propagate errno info?

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 7, 2019

Author Member

I don't think bad_alloc have any parameterized ctor:s?

This comment has been minimized.

Copy link
@Empact

Empact Jan 7, 2019

Member

Indeed - I guess the other option would be to log it in the allocate method. (nit only)

@@ -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;
}

This comment has been minimized.

Copy link
@Empact

Empact Jan 7, 2019

Member

Judging from leveldb's handling, and the mmap docs, the null value case is negligible:

void* base = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
if (base != MAP_FAILED) {
*result = new PosixMmapReadableFile(fname, base, size, &mmap_limit_);
} else {
s = IOError(fname, errno);
}

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 7, 2019

Author Member

I'm not sure how to parse TBH: are you suggesting a change here? :-)

This comment has been minimized.

Copy link
@Empact

Empact Jan 7, 2019

Member

Just that the if (addr) is strictly unnecessary.

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

TIL. Always thought it returned NULL on error.

RETURN VALUE
       On success, mmap() returns a pointer to the mapped area.  On error, the
       value MAP_FAILED (that is, (void *) -1) is returned, and errno  is  set
       to indicate the cause of the error.

utACK ca126d4

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Concept ACK. I too assumed mmap returned NULL on failure.

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Concept ACK - good spot!

@laanwj laanwj merged commit ca126d4 into bitcoin:master Jan 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 9, 2019

Merge #15117: Fix invalid memory write in case of failing mmap(...) i…
…n 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 8d577cf52..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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.