Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12093035:
TCMalloc: support userland ASLR on Linux (Closed)
|
Created:
7 years, 10 months ago by jln (very slow on Chromium) Modified:
7 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, dmikurube+memory_chromium.org, Jorge Lucangeli Obes Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTCMalloc: support userland ASLR on Linux and Chrome OS
On Linux and Chrome OS, we implement user-land ASLR in TCMalloc
on 64 bits Intel architecture.
In this configuration, we are not constrained by the address space
and we don't mind fragmentation.
But to be on the safe side, we only ever fragment half of the
address space.
BUG=170133
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179776
Patch Set 1 #Patch Set 2 : Better comments in test. #
Total comments: 16
Patch Set 3 : Only use "half" of the address space for ASLR. #
Total comments: 14
Patch Set 4 : Address comments from Chris. #
Total comments: 8
Patch Set 5 : Address more comments by Chris. #
Total comments: 1
Patch Set 6 : Address comments from Marius. #Patch Set 7 : Fix buggy ASSERT(). Ohhh, do I miss CHECK(). #
Total comments: 4
Patch Set 8 : Don't assert that urandom is available. #Patch Set 9 : Fix typo in seed computation. #Patch Set 10 : Remove "using" statement to make Windows happy. #
Total comments: 24
Patch Set 11 : Address comments from Jim. #
Total comments: 2
Patch Set 12 : ifdef PRNG code #
Total comments: 14
Patch Set 13 : Address Jim comments. #
Total comments: 4
Patch Set 14 : Address more nits. #
Messages
Total messages: 31 (0 generated)
Chris, please take a look at this userland ASLR implementation.
Jorge, would you be a savior and test it on Chrome OS ?
Marius, do you mind stamping raninit() and ranval() as "good enough"? It's not cryptographically secure, but for what we need I think it's perfectly suited.
https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:134: (defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(__x86_64__) Put in ARM64 to be future proof? :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:152: uint32_t seed = reinterpret_cast<unsigned long>(&c); I think I see what you're doing here -- using the entropy in a stack address just in case the /dev/urandom thing fails. On 64-bit Linux, the best 8 bits of entropy are often in the upper 4 bytes of the pointer (with some more in the lower 4 bytes). But the unsigned long -> uint32_t truncation loses it. How about xor'ing the upper and lower 32-bits of the pointer address for form "seed"? Also add comment saying "grab a little entropy from a stack pointer in case /dev/urandom fails" https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:154: #if !defined(NDEBUG) Do you need this ifdef? Doesn't ASSERT() compile out in NDEBUG builds automatically? https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:161: ASSERT(len == sizeof(seed)); Same NDEBUG vs. ASSERT question. And indentation issue -- should be two to the right? https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:163: close(seed); I think you mean close(urandom_fd)? :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:181: random_address &= 0x3ffffffff000; Postfix constant with ULL? Some compilers might otherwise throw a fit.
I'd love to see a really simple test. Hopefully my suggestion is workable. https://codereview.chromium.org/12093035/diff/12003/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/12093035/diff/12003/base/security_unittest.cc... base/security_unittest.cc:120: TEST(SecurityTest, ALLOC_TEST(RandomMemoryAllocations)) { This test seems complicated to me. Can you just do: - Malloc 1GB - Malloc another GB - Assert not contiguous. It should work because both massive allocations should fall through to a raw mmap() due to the lack of buckets of that size ;-)
Thanks, PTAL! https://codereview.chromium.org/12093035/diff/12003/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/12093035/diff/12003/base/security_unittest.cc... base/security_unittest.cc:120: TEST(SecurityTest, ALLOC_TEST(RandomMemoryAllocations)) { On 2013/01/29 06:10:58, Chris Evans wrote: > This test seems complicated to me. > > Can you just do: > - Malloc 1GB > - Malloc another GB > - Assert not contiguous. > > It should work because both massive allocations should fall through to a raw > mmap() due to the lack of buckets of that size ;-) 1GB is a bit special. That's where we would magically switch from BRK to MMAP() before this patch. I know the test looks a bit complicated, but I think it's pretty good. It's reliably failing when there is no randomization, and reliably not failing otherwise :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:134: (defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(__x86_64__) On 2013/01/29 06:08:08, Chris Evans wrote: > Put in ARM64 to be future proof? :) ARM will be very different in terms of the canonical address space and how the kernel lays everything out. I'll be happy to work on ARM64 version when Chrome supports that architecture :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:152: uint32_t seed = reinterpret_cast<unsigned long>(&c); On 2013/01/29 06:08:08, Chris Evans wrote: > I think I see what you're doing here -- using the entropy in a stack address > just in case the /dev/urandom thing fails. > > On 64-bit Linux, the best 8 bits of entropy are often in the upper 4 bytes of > the pointer (with some more in the lower 4 bytes). But the unsigned long -> > uint32_t truncation loses it. > How about xor'ing the upper and lower 32-bits of the pointer address for form > "seed"? > > Also add comment saying "grab a little entropy from a stack pointer in case > /dev/urandom fails" It was a quick stopgap measure, but it doesn't hurt to make it better indeed. Done :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:154: #if !defined(NDEBUG) On 2013/01/29 06:08:08, Chris Evans wrote: > Do you need this ifdef? Doesn't ASSERT() compile out in NDEBUG builds > automatically? Yeah I didn't really know where ASSERT came from in tcmalloc. But I checked and you're right. Done. https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:161: ASSERT(len == sizeof(seed)); On 2013/01/29 06:08:08, Chris Evans wrote: > Same NDEBUG vs. ASSERT question. > And indentation issue -- should be two to the right? Done. https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:163: close(seed); On 2013/01/29 06:08:08, Chris Evans wrote: > I think you mean close(urandom_fd)? :) Ooch. And I *always* check return values normally. Thanks :) https://codereview.chromium.org/12093035/diff/12003/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:181: random_address &= 0x3ffffffff000; On 2013/01/29 06:08:08, Chris Evans wrote: > Postfix constant with ULL? Some compilers might otherwise throw a fit. Given that it's a &=, I think it's guaranteed ok, but it doesn't hurt. Done.
https://codereview.chromium.org/12093035/diff/11002/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/12093035/diff/11002/base/security_unittest.cc... base/security_unittest.cc:141: const size_t kHighOrderMask = 0xff0000000000; Add ULL postfix again. And maybe uintptr_t is the proper type? https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:150: volatile char c; I don't think volatile is necessarily relevant here? Taking the address has to always succeed. volatile is more about reads and writes to that address, which you don't do. https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:161: ASSERT(close(urandom_fd) == 0); Hang on, won't this potentially optimize out the call to close() in a release build? !! https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:179: random_address &= 0x3ffffffff000; Where's my ULL?
Thanks, PTAL! https://codereview.chromium.org/12093035/diff/11002/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/12093035/diff/11002/base/security_unittest.cc... base/security_unittest.cc:141: const size_t kHighOrderMask = 0xff0000000000; On 2013/01/29 06:36:19, Chris Evans wrote: > Add ULL postfix again. And maybe uintptr_t is the proper type? We're not supposed to use C++11 goodness in theory, but done :) https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:150: volatile char c; On 2013/01/29 06:36:19, Chris Evans wrote: > I don't think volatile is necessarily relevant here? Taking the address has to > always succeed. volatile is more about reads and writes to that address, which > you don't do. Yeah, this was actually an attempt to shut-off compiler optimizations. Which as you know have been annoying me lately. In that case since we have the call to open, I really don't think anything could happen. But I'd rather be safe. https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:161: ASSERT(close(urandom_fd) == 0); On 2013/01/29 06:36:19, Chris Evans wrote: > Hang on, won't this potentially optimize out the call to close() in a release > build? !! Nope, (that would be a really bad ASSERT!). I didn't know tcmalloc's assert and I checked at the same time that I checked it would be disabled in non debug builds. https://codereview.chromium.org/12093035/diff/11002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/system-alloc.cc:179: random_address &= 0x3ffffffff000; On 2013/01/29 06:36:19, Chris Evans wrote: > Where's my ULL? Done.
https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... base/security_unittest.cc:208: fprintf(stdout, "%s\n", buffer); close(fd)? https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... base/security_unittest.cc:215: // Two successive calls to mmap() have roughly one chance out of 2^7 to be With the 0x6f mask, there's only 6 bits of random there, no? https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:109: #define rot(x,k) ((x<<(k))|(x>>(32-(k)))) parens around x? https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:121: static u4 raninit(ranctx* x, u4 seed) { static void? https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:123: x->a = x->b = x->c = 0xf1ea5eed; Where does this sequence come from? From the reference cited above it should be a = 0xf1..ed, b = c = d = seed; https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:163: close(seed); close(urandom_fd) instead https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:168: uint64_t random_address = static_cast<uint64_t>(ranval(&ctx)) << 32 | parens around << expression? Is there no suitable macro to combine two u32 into u64 and avoid this verbose construct? https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:178: random_address &= 0x6ffffffff000; 0x6ff..00 so you're sure it's sufficiently less so hint + alloc_size stays below limit? Either way, 34 bits of play.
https://codereview.chromium.org/12093035/diff/4010/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/12093035/diff/4010/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/system-alloc.cc:161: ASSERT(close(urandom_fd) == 0); Dunno why but I'm still not comfortable relying upon a side effect inside an ASSERT. Ehhhhh! And here we go, in internal_logging.h #ifndef NDEBUG ... #else #define ASSERT(cond) ((void) 0) So how about int close_ret = close(...) ASSERT(close_ret == 0)
Thanks, PTAL! PS: some of your comments were already addressed in later revisions (I know, the Chromium review tool is confusing..). https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... base/security_unittest.cc:208: fprintf(stdout, "%s\n", buffer); On 2013/01/29 06:45:21, Marius wrote: > close(fd)? Arg. Thanks. Done. https://chromiumcodereview.appspot.com/12093035/diff/10001/base/security_unit... base/security_unittest.cc:215: // Two successive calls to mmap() have roughly one chance out of 2^7 to be On 2013/01/29 06:45:21, Marius wrote: > With the 0x6f mask, there's only 6 bits of random there, no? This had been patched in a previous revision already :) https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:109: #define rot(x,k) ((x<<(k))|(x>>(32-(k)))) On 2013/01/29 06:45:21, Marius wrote: > parens around x? Done. https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:121: static u4 raninit(ranctx* x, u4 seed) { On 2013/01/29 06:45:21, Marius wrote: > static void? Done. https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:123: x->a = x->b = x->c = 0xf1ea5eed; On 2013/01/29 06:45:21, Marius wrote: > Where does this sequence come from? > From the reference cited above it should be > a = 0xf1..ed, b = c = d = seed; From the code in the repository. I changed it back to the reference. https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:163: close(seed); On 2013/01/29 06:45:21, Marius wrote: > close(urandom_fd) instead Yeah, Chris caught it as well. Done. https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:168: uint64_t random_address = static_cast<uint64_t>(ranval(&ctx)) << 32 | On 2013/01/29 06:45:21, Marius wrote: > parens around << expression? > Is there no suitable macro to combine two u32 into u64 and avoid this verbose > construct? Not that I know of. Done. https://chromiumcodereview.appspot.com/12093035/diff/10001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:178: random_address &= 0x6ffffffff000; On 2013/01/29 06:45:21, Marius wrote: > 0x6ff..00 so you're sure it's sufficiently less so hint + alloc_size stays below > limit? > Either way, 34 bits of play. Significantly reduced to keep half of address space untouched in a later revision.
> Ehhhhh! And here we go, in internal_logging.h > > #ifndef NDEBUG > ... > #else > #define ASSERT(cond) ((void) 0) Where on Earth did you find that ? I specifically checked before and I have: #ifndef NDEBUG #define ASSERT(cond) CHECK_CONDITION(cond) #define ASSERT_PRINT(cond, str) CHECK_CONDITION_PRINT(cond, str) #else #define ASSERT(cond) ((void) 0) #define ASSERT_PRINT(cond, str) ((void) 0) #endif I'll change it anyway, because it's scary.
On 2013/01/29 07:06:15, Julien Tinnes wrote: > > Ehhhhh! And here we go, in internal_logging.h > > > > #ifndef NDEBUG > > ... > > #else > > #define ASSERT(cond) ((void) 0) > > Where on Earth did you find that ? I specifically checked before and I have: > > #ifndef NDEBUG > #define ASSERT(cond) CHECK_CONDITION(cond) > #define ASSERT_PRINT(cond, str) CHECK_CONDITION_PRINT(cond, str) > #else > #define ASSERT(cond) ((void) 0) > #define ASSERT_PRINT(cond, str) ((void) 0) > #endif > > I'll change it anyway, because it's scary. LGTM
Jim, do you mind approving this new hardening feature?
lgtm https://chromiumcodereview.appspot.com/12093035/diff/4014/base/security_unitt... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/4014/base/security_unitt... base/security_unittest.cc:116: ret = read(fd, buffer, sizeof(buffer) - 1); where's the close()? https://chromiumcodereview.appspot.com/12093035/diff/4014/third_party/tcmallo... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/4014/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:151: uint32_t seed = (reinterpret_cast<uint64_t>(&c) >> 32) | ^ instead of | ?
https://chromiumcodereview.appspot.com/12093035/diff/4014/base/security_unitt... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/4014/base/security_unitt... base/security_unittest.cc:116: ret = read(fd, buffer, sizeof(buffer) - 1); On 2013/01/29 07:41:27, Marius wrote: > where's the close()? I added a ScopedFD instead. https://chromiumcodereview.appspot.com/12093035/diff/4014/third_party/tcmallo... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/4014/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:151: uint32_t seed = (reinterpret_cast<uint64_t>(&c) >> 32) | On 2013/01/29 07:41:27, Marius wrote: > ^ instead of | ? Ooch. Decidedly, I should stop working for today. Thanks.
https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:105: // Not cryptographically secure, but good enough for what we need. Why are we using this, rather than more standard crypto functions?? What is this adding to the mix? We already called with a result from devrand. IF that fails, this just produces a predictable constant, based on the &c (I think). If devrand works, this mixing doesn't appear to add much IMO. What am I missing? https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:131: #define ASLR_IS_SUPPORTED \\ I doubt you meant to postpend a double backslash... Didn't you just want to continue the #define? https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:132: (defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(__x86_64__) "defined" is only meaningful in the context of a "#if" line. You can't play games and #define something to be a conditional, hoping it will be expanded and evaluated inside a #if. I'm pretty sure you need to #if defined... #defined ASLR_IS_SUPPORTED #endif [I could be mistaken about this... but when I wrote a C preprocessor, back in the old "ANSI C" days, this form was not supported as I *think* you intended. If you can reference newer doc, that is fine.] https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:135: // mmap fail , as the kernel will simply not follow the hint if it can't. nit: Remove extra space: "fail , as" ---> "fail, as" https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:140: #if defined(ASLR_IS_SUPPORTED) I'm pretty sure this will always evaluate to true, since you *did* define this macro. Note that whether you had said: #define foo 1 or #define foo 0 you would still get a true from: #if defined(foo) As a result, it appears that your "conditional code" is really not conditional at all :-(. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:148: volatile char c; Why did you use the keyword volatile here? What did you think you were getting that was different?? https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:162: initialized = true; FWIW: the compiler can optimize this line to perform it around line 148. Why not list it there? Your protection via a lock is probably critical to make this code reviewable (without concerns for the race to init the static), so I'd suggest changing the comment on line 142 to better appreciate the lock. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:180: return NULL; nit: With if's (and certainly #if), it is much easier to read if the short branch (in this case, return NULL) appears first). https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:225: #if defined(ASLR_IS_SUPPORTED) As noted, this is always true here :-(. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:226: true), nit: breaking coded fragments (single short lines) across ifdefs makes things much less readable. Please just write teh two lines out separately. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:548: sdef->SetChildAllocator(mmap, 0, mmap_name); Instead of moving line at 455 up to line 542 above, why not put this block (with the ifdef) down in front of line 557? Merges are always a pain, and the less code motion you create, the easier things will be. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:559: sdef->SetChildAllocator(sbrk, 1, sbrk_name); Why is this line not included in the new ASLR code?
Thanks, PTAL! https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:105: // Not cryptographically secure, but good enough for what we need. On 2013/01/30 02:29:13, jar wrote: > Why are we using this, rather than more standard crypto functions?? What is > this adding to the mix? > > We already called with a result from devrand. IF that fails, this just produces > a predictable constant, based on the &c (I think). If devrand works, this > mixing doesn't appear to add much IMO. > > What am I missing? What would be a more standard crypto function ? We could add a better crypto PRNG here, but we don't need it, this is good enough. /dev/urandom is only used once, to feed the seed of the PRNG. When it's not available (for instance PPAPI plugins), we use &c. &C has some randomness due to STACK ASLR and a few other factors. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:131: #define ASLR_IS_SUPPORTED \\ On 2013/01/30 02:29:13, jar wrote: > I doubt you meant to postpend a double backslash... Didn't you just want to > continue the #define? Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:132: (defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(__x86_64__) On 2013/01/30 02:29:13, jar wrote: > "defined" is only meaningful in the context of a "#if" line. You can't play > games and #define something to be a conditional, hoping it will be expanded and > evaluated inside a #if. I'm pretty sure you need to > #if defined... > #defined ASLR_IS_SUPPORTED > #endif > > > [I could be mistaken about this... but when I wrote a C preprocessor, back in > the old "ANSI C" days, this form was not supported as I *think* you intended. > If you can reference newer doc, that is fine.] Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:135: // mmap fail , as the kernel will simply not follow the hint if it can't. On 2013/01/30 02:29:13, jar wrote: > nit: Remove extra space: "fail , as" ---> "fail, as" Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:140: #if defined(ASLR_IS_SUPPORTED) On 2013/01/30 02:29:13, jar wrote: > I'm pretty sure this will always evaluate to true, since you *did* define this > macro. Note that whether you had said: > #define foo 1 > or > #define foo 0 > you would still get a true from: > #if defined(foo) > > As a result, it appears that your "conditional code" is really not conditional > at all :-(. Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:148: volatile char c; On 2013/01/30 02:29:13, jar wrote: > Why did you use the keyword volatile here? What did you think you were getting > that was different?? I'm trying to tame a potential processor optimization. As you know, I got burned recently :) I really want the address of c, as I expect to get from randomness from it. I don't want the compiler to think that the C standard allows it to virtually put it at address 0 or something since I don't use the address for real. Or something along these lines. In practice it doesn't matter, given that we call read() with something that depends on this.. But I thought it would be nicer to assert with volatile. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:162: initialized = true; On 2013/01/30 02:29:13, jar wrote: > FWIW: the compiler can optimize this line to perform it around line 148. Why not > list it there? > > Your protection via a lock is probably critical to make this code reviewable > (without concerns for the race to init the static), so I'd suggest changing the > comment on line 142 to better appreciate the lock. Yes, that comment was for the ranctx, but you're right about initialized. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:180: return NULL; On 2013/01/30 02:29:13, jar wrote: > nit: With if's (and certainly #if), it is much easier to read if the short > branch (in this case, return NULL) appears first). Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:225: #if defined(ASLR_IS_SUPPORTED) On 2013/01/30 02:29:13, jar wrote: > As noted, this is always true here :-(. Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:226: true), On 2013/01/30 02:29:13, jar wrote: > nit: breaking coded fragments (single short lines) across ifdefs makes things > much less readable. Please just write teh two lines out separately. Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:548: sdef->SetChildAllocator(mmap, 0, mmap_name); On 2013/01/30 02:29:13, jar wrote: > Instead of moving line at 455 up to line 542 above, why not put this block (with > the ifdef) down in front of line 557? > > Merges are always a pain, and the less code motion you create, the easier things > will be. Done. https://chromiumcodereview.appspot.com/12093035/diff/1009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:559: sdef->SetChildAllocator(sbrk, 1, sbrk_name); On 2013/01/30 02:29:13, jar wrote: > Why is this line not included in the new ASLR code? At first, I thought we didn't want a fallback to the brk() heap. But in the x86_64 case (our case), it's really unlikely that an attacker would be able to put pressure on the address space to get a brk() heap fallback. There are also more fringe reasons: with a randomized mmap() allocator, in theory, if we got really unlucky, we could collide with the brk() heap. However, if we're concerned about that, I could just tune the ASLR zone to make sure it can never happen. WDYT ?
https://chromiumcodereview.appspot.com/12093035/diff/13008/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/13008/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:129: // End PRNG code. nit: all this PRNG code can drop into a if defined(ASLR_IS_SUPPORTED) block. Just move up lines 131-133.
https://chromiumcodereview.appspot.com/12093035/diff/13008/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/13008/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:129: // End PRNG code. On 2013/01/30 03:33:26, jar wrote: > nit: all this PRNG code can drop into a if defined(ASLR_IS_SUPPORTED) block. > > Just move up lines 131-133. Done.
Mostly nits and a few questions about the test. I'm convinced that the meat of the code is correct. https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:218: // detected as having the same order. With 32 allocations, we see ~16 that I didn't understand the argument. Down below, you define "order" to mean "same high order bits." If that is what you meant here, you should spell that out, otherwise "order" tends to mean "sequentially related." Down below, high order is the top 8 bits, so I'd expect a pair to have the same "order" with probability 1 in 256, or as you might say, 1 in 2^8. Where does the power of 6 come from? https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:219: // trigger a call to mmap, so the chances of this test flaking is roughly I'm not sure why these calls would trigger mmap calls. I suspect this test is being run after tons of other tests and activities, and hence tons of allocations and frees have been done in random order. As a result, this would appear to be querying the existing cache of allocations, so too if available allocations have different high-order bytes. Why do you think this will cause mmap to be called? https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:241: is_contiguous = false; This test will break out of this loop (causing the test to pass) as soon as two allocations have different high-order bits. Given the sparsity of random allocations, I would have expected something much stricter would still be true. With 32 allocations randomly out of a 256 bucket (8 bit) mask, I'd be a tad surprised (for example) if more than 2 allocations were from one bucket. I wouldn't be as surprised if two allocations were from different buckets. Nicer might be to strengthen this test, to tally how many allocations came out of any one bucket, and perchance fail if more than k (8 out of 32? 16 out of 32?) were in a single bucket. The current test fails if *all* 32 out of 32 were in a single bucket. I guess you're getting a nice low failure rate... but you currently don't (probably) test to see if the PRNG has more than about two significantly different outputs. https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:112: typedef struct ranctx { u4 a; u4 b; u4 c; u4 d; } ranctx; nit: Please reformat the structure definition with one declaration per line, and you don't need the typedef since you have the struct declaration here in C++. https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:118: u4 e = x->a - rot(x->b, 27); I was bothered by the use of single character names for slots and locals, but perchance this is easier to read and compare with the web code if left this way... so I won't ask for it to be made compliant. https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:128: x->a = 0xf1ea5eed, x->b = x->c = x->d = seed; nit: Please reformat that line to comply with style guide. Don't use comma; put one assignment per line.
https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:242: break; nit: if you keep this test as is, it is much more readable to replace the "break;" with: return; // Test passes. and not bother with the is_contiguous variable. Line 246 would then be a failure point.
Thanks Jim. PTAL! https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:218: // detected as having the same order. With 32 allocations, we see ~16 that On 2013/01/30 17:20:48, jar wrote: > I didn't understand the argument. > > Down below, you define "order" to mean "same high order bits." If that is what > you meant here, you should spell that out, otherwise "order" tends to mean > "sequentially related." > > Down below, high order is the top 8 bits, so I'd expect a pair to have the same > "order" with probability 1 in 256, or as you might say, 1 in 2^8. > > Where does the power of 6 come from? 6 comes from the smallest mask that we use in the implementation (0x3f...); I've edited the comment. https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:219: // trigger a call to mmap, so the chances of this test flaking is roughly On 2013/01/30 17:20:48, jar wrote: > I'm not sure why these calls would trigger mmap calls. I suspect this test is > being run after tons of other tests and activities, and hence tons of > allocations and frees have been done in random order. As a result, this would > appear to be querying the existing cache of allocations, so too if available > allocations have different high-order bytes. > > Why do you think this will cause mmap to be called? I did check the number of mmaps by adding some instrumentation. I thought we were re-executing at every test (i.e. the threadsafe option of gtest, which also looks more similar than Windows). But your argument is valid, it's not reliable / defined behavior. I've changed the comment to mention different buckets. https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:241: is_contiguous = false; On 2013/01/30 17:20:48, jar wrote: > This test will break out of this loop (causing the test to pass) as soon as two > allocations have different high-order bits. > > Given the sparsity of random allocations, I would have expected something much > stricter would still be true. With 32 allocations randomly out of a 256 bucket > (8 bit) mask, I'd be a tad surprised (for example) if more than 2 allocations > were from one bucket. I wouldn't be as surprised if two allocations were from > different buckets. > > Nicer might be to strengthen this test, to tally how many allocations came out > of any one bucket, and perchance fail if more than k (8 out of 32? 16 out of > 32?) were in a single bucket. The current test fails if *all* 32 out of 32 were > in a single bucket. > > I guess you're getting a nice low failure rate... but you currently don't > (probably) test to see if the PRNG has more than about two significantly > different outputs. You make good points, but I am quite happy with where we are in the false-positives / false-negeatives trade-offs. As-is: - The test will never flake - The chances of false positive are still very, very low: We are looking at the the high 6 bits of a 46 bits address (we force bit 47 to 0). For a kernel allocator (brk() or mmap()) to cross such a zone is very unlikely as this represents a 1TB zone. Moreover, what we worry about is "sustained false positives". The chances of someone breaking ASLR and landing a CL that will with very bad luck pass on all Linux bots, repeatedly are tiny. In a nutshell, I think what we want is to have almost no chances of flake and small chances of false positives, and I think that's where we are. https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:242: break; On 2013/01/30 17:35:29, jar wrote: > nit: if you keep this test as is, it is much more readable to replace the > "break;" with: > > return; // Test passes. > > and not bother with the is_contiguous variable. > > Line 246 would then be a failure point. Done. https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:112: typedef struct ranctx { u4 a; u4 b; u4 c; u4 d; } ranctx; On 2013/01/30 17:20:48, jar wrote: > nit: Please reformat the structure definition with one declaration per line, and > you don't need the typedef since you have the struct declaration here in C++. Done. https://chromiumcodereview.appspot.com/12093035/diff/15001/third_party/tcmall... third_party/tcmalloc/chromium/src/system-alloc.cc:128: x->a = 0xf1ea5eed, x->b = x->c = x->d = seed; On 2013/01/30 17:20:48, jar wrote: > nit: Please reformat that line to comply with style guide. Don't use comma; put > one assignment per line. Done.
https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15001/base/security_unit... base/security_unittest.cc:108: // The tests bellow check for overflows in new[] and calloc(). Note: don't worry about this stuff disappearing in this revision, it's just due to a git rebase and how our review tool works.
LGTM to land with caveats: a) Fix nits below. b) I'd like your test to be better, as it currently only tests for 1 bit of entropy from the PRNG. Not a show stopper... but it would be much better to see if (for example) running the test 1024 times, we got pretty nearly every bin to have an allocation (out of 64 possible bins). c) Do tests to see how much meta-info is used up due to fragmentation of the series of spans. Overnight test in CrOS with a lot of work would be real nice. Looking at about:tcmalloc to see if you can spot a difference would be valuable. The worst case fear is that you're causing fragmentation, precluding coalescing of spans, leading to either a ton of spans, or a ton of waste, or a ton of (otherwise) unnecessary calls to mmap (perf issue). https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... base/security_unittest.cc:146: const uintptr_t kHighOrderMask = 0xff0000000000ULL; nit: if the top two bits don't ever change (reason for you hot get to 6 bits), you may as well mask: 0x3f000.... https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... base/security_unittest.cc:152: is_contiguous = false; nit: remove dead variable
https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... base/security_unittest.cc:146: const uintptr_t kHighOrderMask = 0xff0000000000ULL; On 2013/01/31 00:39:28, jar wrote: > nit: if the top two bits don't ever change (reason for you hot get to 6 bits), > you may as well mask: > > 0x3f000.... Done. https://chromiumcodereview.appspot.com/12093035/diff/15002/base/security_unit... base/security_unittest.cc:152: is_contiguous = false; On 2013/01/31 00:39:28, jar wrote: > nit: remove dead variable Done.
On 2013/01/31 00:39:27, jar wrote: > LGTM to land with caveats: > > a) Fix nits below. Done. > b) I'd like your test to be better, as it currently only tests for 1 bit of > entropy from the PRNG. Not a show stopper... but it would be much better to see > if (for example) running the test 1024 times, we got pretty nearly every bin to > have an allocation (out of 64 possible bins). > > c) Do tests to see how much meta-info is used up due to fragmentation of the > series of spans. Overnight test in CrOS with a lot of work would be real nice. > Looking at about:tcmalloc to see if you can spot a difference would be valuable. > > The worst case fear is that you're causing fragmentation, precluding coalescing > of spans, leading to either a ton of spans, or a ton of waste, or a ton of > (otherwise) unnecessary calls to mmap (perf issue). As discussed in person, I'll land this patch now. I'll send a better test next week and find a way to watch for additional waste in long-running tests. Julien
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/12093035/8010
As a quick sanity check, I went through the "page cycler" burn test, with and without the patch and checked that I didn't see any obvious differences in about:tcmalloc. Nothing noticeable so far. Committing and I'll try to find a more comprehensive way to test. I talked to a few Chrome OS people already and we don't seem to have anything readily available that does that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/12093035/8010
Message was sent while issue was closed.
Change committed as 179776 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
