Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11857007:
TCMalloc: restrict maximum size of memory ranges (Closed)
|
Created:
7 years, 11 months ago by jln (very slow on Chromium) Modified:
7 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Dai Mikurube (NOT FULLTIME) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTCMalloc: restrict maximum size of memory allocations
For security purposes, we restrict the maximum size of memory allocations under
what can be indexed by an int.
BUG=169327
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176961
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Fix compilation on windows (being annoying with its max() macro) #Patch Set 3 : Handle NO_TCMALLOC in test. #Patch Set 4 : Disable test in ASAN. #
Total comments: 11
Patch Set 5 : Don't modify any system allocator, just GrowHeap. #
Total comments: 1
Patch Set 6 : Patch the tcmalloc interface instead of GrowHeap. #Patch Set 7 : Detect configurations where tcmalloc is disabled better. #Patch Set 8 : Detect Valgrind bypassing tcmalloc. #Patch Set 9 : Drive-by patch of int to size_t. #
Total comments: 11
Patch Set 10 : Nits from Cevans. #
Total comments: 3
Messages
Total messages: 28 (0 generated)
Not fully ready yet, but a good starting point. Note that by default Linux uses a brk() allocator. https://codereview.chromium.org/11857007/diff/2001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/page_heap.cc (right): https://codereview.chromium.org/11857007/diff/2001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/page_heap.cc:473: if (!IsContiguousAllocSizePermitted(n << kPageShift)) It's a bit ugly to have the check here *and* in the Linux Brk allocator. An alternative would be to either patch every allocator implementation individually. https://codereview.chromium.org/11857007/diff/2001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/11857007/diff/2001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/system-alloc.cc:495: We could also add the limit check here, instead of page_heap.cc, but we would also need to add it to windows/port.cc
Also as you can see, I added a new test in base (it was about time we had a place holder for some security tests in base/). The problem is that if the test fails, it'll allocate 2GB of memory. If that becomes a problem we could add some kind of hack to reduce the maximum limit for the security test.
The strategy seems fine, but you really want to add @jar to this CL for review, and get him up to speed on the full context.
Jim, would be able to take a look ? We want to prevent contiguous mappings of more than 2GB from existing to mitigate some bugs in code that uses int instead of size_t. The linked bug has more information.
On 2013/01/11 18:15:23, Julien Tinnes wrote: > Jim, would be able to take a look ? > > We want to prevent contiguous mappings of more than 2GB from existing to > mitigate some bugs in code that uses int instead of size_t. The linked bug has > more information. For a quantity of "some" that more likely equals "many."
https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); The behaviour of tcmalloc within Chromium is to abort on an allocation failure. It's an important security guarantee. Does this test imply that this CL changes that invariant?
On 2013/01/11 19:17:28, Chris Evans wrote: > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc > File base/security_unittest.cc (right): > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... > base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); > The behaviour of tcmalloc within Chromium is to abort on an allocation failure. > It's an important security guarantee. > > Does this test imply that this CL changes that invariant? No, this CL doesn't change this at all, it operates at a lower level. I don't think do_malloc() ever aborts cpp_malloc() (which is used by new etc..) does however.
On 2013/01/11 19:26:00, Julien Tinnes wrote: > On 2013/01/11 19:17:28, Chris Evans wrote: > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc > > File base/security_unittest.cc (right): > > > > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... > > base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); > > The behaviour of tcmalloc within Chromium is to abort on an allocation > failure. > > It's an important security guarantee. > > > > Does this test imply that this CL changes that invariant? > > No, this CL doesn't change this at all, it operates at a lower level. I don't > think do_malloc() ever aborts cpp_malloc() (which is used by new etc..) does > however. See http://code.google.com/p/chromium/issues/detail?id=51286 Something changed? Or it's implemented by some "malloc fail hook" we register in Chrome code?
On 2013/01/11 19:32:27, Chris Evans wrote: > On 2013/01/11 19:26:00, Julien Tinnes wrote: > > On 2013/01/11 19:17:28, Chris Evans wrote: > > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc > > > File base/security_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... > > > base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); > > > The behaviour of tcmalloc within Chromium is to abort on an allocation > > failure. > > > It's an important security guarantee. > > > > > > Does this test imply that this CL changes that invariant? > > > > No, this CL doesn't change this at all, it operates at a lower level. I don't > > think do_malloc() ever aborts cpp_malloc() (which is used by new etc..) does > > however. > > See http://code.google.com/p/chromium/issues/detail?id=51286 > > Something changed? Or it's implemented by some "malloc fail hook" we register in > Chrome code? Ah, that bug I linked seems to indicate we _do_ have a hook, OnNoMemory :)
On 2013/01/11 19:32:58, Chris Evans wrote: > On 2013/01/11 19:32:27, Chris Evans wrote: > > On 2013/01/11 19:26:00, Julien Tinnes wrote: > > > On 2013/01/11 19:17:28, Chris Evans wrote: > > > > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc > > > > File base/security_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... > > > > base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); > > > > The behaviour of tcmalloc within Chromium is to abort on an allocation > > > failure. > > > > It's an important security guarantee. > > > > > > > > Does this test imply that this CL changes that invariant? > > > > > > No, this CL doesn't change this at all, it operates at a lower level. I > don't > > > think do_malloc() ever aborts cpp_malloc() (which is used by new etc..) does > > > however. > > > > See http://code.google.com/p/chromium/issues/detail?id=51286 > > > > Something changed? Or it's implemented by some "malloc fail hook" we register > in > > Chrome code? > > Ah, that bug I linked seems to indicate we _do_ have a hook, OnNoMemory :) Yeah, we have EnableTerminationOnOutOfMemory in process_util.h which Chrome uses. But tcmalloc doesn't do that automatically, so this unit test is not affected.
Justin / Chris: could you do a first path code review ? It would be nice if Jim only has to approve.
https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc File base/security_unittest.cc (right): https://codereview.chromium.org/11857007/diff/3009/base/security_unittest.cc#... base/security_unittest.cc:26: ptr(static_cast<char*>(malloc((std::numeric_limits<int>::max)()))); Isn't it jusr std::numeric_limits<int>::max() ? Too many parens! Confusing :P https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/common.cc (right): https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/common.cc:51: return alloc_size <= ((std::numeric_limits<int>::max)() - kPageSize); Unusual parens used again. https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/system-alloc.cc:194: static const char mmap_name[] = "MmapSysAllocator"; Don't you need a similar change for the MmapSysAllocator? https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/system-alloc.cc:238: if (reinterpret_cast<intptr_t>(current_brk_address) + size < size) { Ooh... this was here before, but intptr_t is a signed long int usually. It'll get cast correctly to unsigned due to addition to a size_t, but still... maybe use uintptr_t ? https://codereview.chromium.org/11857007/diff/3009/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/system-alloc.cc:249: return NULL; This seems like a very low-level way to accomplish this. In terms of the pattern of calls during the vulnerability we're worried about, we'll likely see a realloc() with a size parameter > INT_MAX. Why aren't we just adding a check for the individual size parameter at malloc() and realloc() etc. It seems simpler?
https://chromiumcodereview.appspot.com/11857007/diff/3009/base/security_unitt... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/3009/base/security_unitt... base/security_unittest.cc:26: ptr(static_cast<char*>(malloc((std::numeric_limits<int>::max)()))); On 2013/01/11 19:51:51, Chris Evans wrote: > Isn't it jusr std::numeric_limits<int>::max() ? > Too many parens! Confusing :P That's because of Windows. Windows defines a macro called max() which breaks compilation. https://chromiumcodereview.appspot.com/11857007/diff/3009/base/security_unitt... base/security_unittest.cc:27: ASSERT_TRUE(ptr == NULL); On 2013/01/11 19:17:28, Chris Evans wrote: > The behaviour of tcmalloc within Chromium is to abort on an allocation failure. > It's an important security guarantee. > > Does this test imply that this CL changes that invariant? No, as discussed on the thread (I suspect this comment was re-sent inadvertently. https://chromiumcodereview.appspot.com/11857007/diff/3009/third_party/tcmallo... File third_party/tcmalloc/chromium/src/common.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/3009/third_party/tcmallo... third_party/tcmalloc/chromium/src/common.cc:51: return alloc_size <= ((std::numeric_limits<int>::max)() - kPageSize); On 2013/01/11 19:51:51, Chris Evans wrote: > Unusual parens used again. Windows, again ;) https://chromiumcodereview.appspot.com/11857007/diff/3009/third_party/tcmallo... File third_party/tcmalloc/chromium/src/system-alloc.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/3009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:194: static const char mmap_name[] = "MmapSysAllocator"; On 2013/01/11 19:51:51, Chris Evans wrote: > Don't you need a similar change for the MmapSysAllocator? No, the mmap allocator will not try to grow an existing area. https://chromiumcodereview.appspot.com/11857007/diff/3009/third_party/tcmallo... third_party/tcmalloc/chromium/src/system-alloc.cc:249: return NULL; On 2013/01/11 19:51:51, Chris Evans wrote: > This seems like a very low-level way to accomplish this. > > In terms of the pattern of calls during the vulnerability we're worried about, > we'll likely see a realloc() with a size parameter > INT_MAX. > > Why aren't we just adding a check for the individual size parameter at malloc() > and realloc() etc. It seems simpler? The change to GrowHeap should take care of that in pageheap.cc. The reason for dealing with the brk() heap directly is that it's the only allocator that will generate contiguous mappings. I didn't land the test for this yet, but I thought we didn't want multiple small malloc() to generate a 4GB slab in any case. This would make sure that array[negative_int] has a great change to segfault right away.
On 2013/01/11 20:02:04, Julien Tinnes wrote: > This would make sure that array[negative_int] has a great > change to segfault right away. I mean array[negative_int_casted_to_unsigned] obviously. In general I thought not having big slabs was exactly what we wanted.
Chris, Justin, would you mind writing down precisely in the bug the cases we want to handle? If we're not concerned about large contiguous slabs, but just want very large malloc to fail, we can patch the interface functions instead.
Alright, I've simplified the patch. Now we restrict large mapping requests to the operating system via a check in GrowHeap. We could also have the check in TCMalloc_SystemAlloc, but we'd need to do it in the Windows specific port as well. (A check in TCMalloc_SystemAlloc would also cover meta data allocations, but we don't need to cover these). The patch is now fairly trivial. If we ever want to restrict large adjacent heap areas from existing, we'd need to modify both the mmap and brk allocators.
Changes in TCMalloc LGTM... but I'm counting on others to be sure the test is optimally placed in the code to achieve your purpose. (also see nit below). https://codereview.chromium.org/11857007/diff/22001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/page_heap.cc (right): https://codereview.chromium.org/11857007/diff/22001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/page_heap.cc:472: if (!IsContiguousAllocSizePermitted(n << kPageShift)) nit: test should come next to similar test in line 468, as it does not depend on |ask|, and probably should use "return false" on same line to match local formatting.
Thanks Jim. I'll discuss a bit more with Justin and Chris to see if we might want to patch at the interface level instead (do_malloc) etc, and ask you for a last review if we end-up making changes.
This is a new iteration, patching the tcmalloc interface instead of the internals (GrowHeap), since now we only want to handle a simpler problem. Justin / Cevans: please approve and I'll ask Jim for a last look afterwards.
https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:60: ptr(static_cast<char*>(malloc(kMaxAllocSize))); It's weird to have a test that "kMaxAllocSize" actually fails (because of the page_size accounting tweak I know). Maybe call it kTooBigAllocSize ? https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:61: ASSERT_TRUE(ptr == NULL); Indentation. https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:61: ASSERT_TRUE(ptr == NULL); Indentation https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:105: // Implementing this requires more effort and the test is disabled. I think we're better off not landing to be honest? https://chromiumcodereview.appspot.com/11857007/diff/29003/third_party/tcmall... File third_party/tcmalloc/chromium/src/common.h (right): https://chromiumcodereview.appspot.com/11857007/diff/29003/third_party/tcmall... third_party/tcmalloc/chromium/src/common.h:149: bool IsAllocSizePermitted(size_t alloc_size); Mark it inline. Any maybe have the implementation inline too? It's no larger than the pages() function above.
Thanks, PTAL! https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... File base/security_unittest.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/15 01:53:08, Chris Evans wrote: > 2013 Done. https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:60: ptr(static_cast<char*>(malloc(kMaxAllocSize))); On 2013/01/15 01:53:08, Chris Evans wrote: > It's weird to have a test that "kMaxAllocSize" actually fails (because of the > page_size accounting tweak I know). Maybe call it kTooBigAllocSize ? Good point, done. https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:61: ASSERT_TRUE(ptr == NULL); On 2013/01/15 01:53:08, Chris Evans wrote: > Indentation. Done. https://chromiumcodereview.appspot.com/11857007/diff/29003/base/security_unit... base/security_unittest.cc:105: // Implementing this requires more effort and the test is disabled. On 2013/01/15 01:53:08, Chris Evans wrote: > I think we're better off not landing to be honest? Done. https://chromiumcodereview.appspot.com/11857007/diff/29003/third_party/tcmall... File third_party/tcmalloc/chromium/src/common.h (right): https://chromiumcodereview.appspot.com/11857007/diff/29003/third_party/tcmall... third_party/tcmalloc/chromium/src/common.h:149: bool IsAllocSizePermitted(size_t alloc_size); On 2013/01/15 01:53:08, Chris Evans wrote: > Mark it inline. Any maybe have the implementation inline too? It's no larger > than the pages() function above. Let me know how strongly you feel about it. Since it does straddle compilation units, if we really want it inline, we should put the implementation in the header. I don't feel strongly either way.
https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1092: if (IsAllocSizePermitted(size)) { Move the test so it's in the "else" branch of "size <= kMaxSize" It'll avoid a test/branch on the hot path.
On 2013/01/15 02:13:32, Chris Evans wrote: > https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... > File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): > > https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... > third_party/tcmalloc/chromium/src/tcmalloc.cc:1092: if > (IsAllocSizePermitted(size)) { > Move the test so it's in the "else" branch of "size <= kMaxSize" > It'll avoid a test/branch on the hot path. LGTM from security. Defer to jar@ on where and how to structure the new "if" branch. FWIW, my vote remains the same as I suggested above but certainly defer to jar@ :)
https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1092: if (IsAllocSizePermitted(size)) { On 2013/01/15 02:13:32, Chris Evans wrote: > Move the test so it's in the "else" branch of "size <= kMaxSize" > It'll avoid a test/branch on the hot path. In release mode, here is the assembly: cmp r14, 7FFFEFFFh ja loc_14F4 cmp r14, 8000h ja loc_1206 As you can see, it does get inlined, but we don't issue branch prediction hints. (Apparently modern Intel processors don't even honor such hints anymore). If we really care about this I think we should rather give hints to the compiler as to which one is the likely branch (and as you can see, as compiled, we already don't take any branch anyways). But it seems very unlikely to have any difference on a modern architecture. And if we care about that level of optimization, we should optimize GetCache() above, which has more impact.
Jim, could you please do one last review for final approval ? As you can see the patch to tcmalloc is now really trivial (there is one outstanding remark from Chris about branch optimization). In addition, there is some difficulty in the unit test because detecting if tcmalloc is being used is hard. I didn't implement querying the allocator_shim in base/allocator_shim.cc because at the moment it doesn't seem to be used in base_unittests anyways. For now there is nothing special to do on Windows, since it's 32 bits only, but at some point we may want to link the allocator_shim to base_unittests to properly test for that. Or I can just exclude Windows from this test if you prefer. I would like to land this in a reasonably simple version, but at some point, we'll really want a similar browser_test anyways, given the complexity of our run-time environment.
lgtm https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/11857007/diff/19015/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1250: const size_t lower_bound_to_grow = old_size + old_size / 4; These two lines should be upstreamed to google3.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/11857007/19015
Message was sent while issue was closed.
Change committed as 176961 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
