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

practicalswift commented at 4:51 pm on January 6, 2019: contributor

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):

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

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

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

After this patch (with failing mmap call):

 0$ src/bitcoind
 1…
 22019-01-06T15:50:18Z Using the 'sse4(1way),sse41(4way)' SHA256 implementation
 32019-01-06T15:50:18Z Using RdRand as an additional entropy source
 42019-01-06T15:50:18Z
 5
 6************************
 7EXCEPTION: St9bad_alloc
 8std::bad_alloc
 9bitcoin in AppInit()
10
11
12
13************************
14EXCEPTION: St9bad_alloc
15std::bad_alloc
16bitcoin in AppInit()
17
182019-01-06T15:50:18Z Shutdown: In progress...
192019-01-06T15:50:18Z Shutdown: done

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

 0diff --git a/src/support/lockedpool.cpp b/src/support/lockedpool.cpp
 1index 8d577cf52..ce79e569b 100644
 2--- a/src/support/lockedpool.cpp
 3+++ b/src/support/lockedpool.cpp
 4@@ -247,7 +247,8 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
 5 {
 6     void *addr;
 7     len = align_up(len, page_size);
 8-    addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
 9+    // addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
10+    addr = MAP_FAILED;
11     if (addr) {
12         *lockingSuccess = mlock(addr, len) == 0;
13     }

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

practicalswift renamed this:
~~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
on Jan 6, 2019

practicalswift renamed this:
~~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
on Jan 6, 2019

in src/support/allocators/secure.h:45 in ca126d490b

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

Empact commented at 2:22 am on January 7, 2019:

Maybe propagate errno info?

practicalswift commented at 3:55 pm on January 7, 2019:

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

Empact commented at 6:45 pm on January 7, 2019:

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

in src/support/lockedpool.cpp:253 in ca126d490b

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

Empact commented at 2:23 am on January 7, 2019:

Judging from leveldb’s handling, and the mmap docs, the null value case is negligible: https://github.com/bitcoin/bitcoin/blob/9c719987718d9fcc3a689e50f5212acc7ead7606/src/leveldb/util/env_posix.cc#L367-L372

practicalswift commented at 3:49 pm on January 7, 2019:

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

Empact commented at 6:47 pm on January 7, 2019:

Just that the if (addr) is strictly unnecessary.

Empact commented at 2:24 am on January 7, 2019: member

Concept ACK

laanwj commented at 4:23 pm on January 7, 2019: member

TIL. Always thought it returned NULL on error.

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

utACK ca126d490b0ff6960e135f3c77b2b2d4892a5744

gmaxwell commented at 8:29 pm on January 7, 2019: contributor

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

donaloconnor commented at 11:57 pm on January 7, 2019: contributor

Concept ACK - good spot!

laanwj merged this on Jan 9, 2019

laanwj closed this on Jan 9, 2019

laanwj referenced this in commit 699d0bd9fe on Jan 9, 2019

zkbot referenced this in commit 5ef5d8d268 on Jul 31, 2020

zkbot referenced this in commit 7d94064616 on Sep 29, 2020

jasonbcox referenced this in commit 6456edaff6 on Nov 11, 2020

practicalswift deleted the branch on Apr 10, 2021

PastaPastaPasta referenced this in commit ce0eeaae23 on Jun 26, 2021

PastaPastaPasta referenced this in commit 461a6c0eb1 on Jun 28, 2021

furszy referenced this in commit 19d00a6ed4 on Sep 11, 2021

gades referenced this in commit 26325c7f0e on Apr 29, 2022

DrahtBot locked this on Aug 18, 2022

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