Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11783104:
Linux sandbox: allow 32GB address space size (Closed)
|
Created:
7 years, 11 months ago by jln (very slow on Chromium) Modified:
7 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln+watch_chromium.org, jschuh, Ken Russell (switch to Gerrit) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux sandbox: limit the address space
- We limit the address space to 16GB.
(We relax a previous restriction of 4GB of address space mapping).
- We limit the brk() heap to 2GB.
BUG=v8:2473, 169327, 157177
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177037
Patch Set 1 : #Patch Set 2 : Limit the brk() heap. #
Total comments: 6
Patch Set 3 : Address remarks from Chris. #Messages
Total messages: 11 (0 generated)
NACK. This introduces a security bug as far as I know. i.e. we're relying on this defense for a known issue that requires a huge VM size to trigger. I recommend a separate e-mail thread to discuss options. On Thu, Jan 10, 2013 at 1:20 PM, <jln@chromium.org> wrote: > Reviewers: Chris Evans, Sven Panne, > > Description: > Linux sandbox: allow 32GB address space size > > We relax a previous restriction of 4GB of address space mapping. > > BUG=v8:2473 > > > Please review this at https://chromiumcodereview.**appspot.com/11783104/<https://chromiumcodereview... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M content/common/sandbox_linux.**cc > > > Index: content/common/sandbox_linux.**cc > diff --git a/content/common/sandbox_**linux.cc b/content/common/sandbox_** > linux.cc > index b62100ca40dc16c99c3c855cbeeaa1**6dcf0f8180..** > db85251a426226273e4b961c5bef45**abe19fd06b 100644 > --- a/content/common/sandbox_**linux.cc > +++ b/content/common/sandbox_**linux.cc > @@ -262,8 +262,13 @@ bool LinuxSandbox::**LimitAddressSpace(const > std::string& process_type) { > if (command_line->HasSwitch(**switches::kNoSandbox)) { > return false; > } > - // Limit the address space to 4GB. > - const rlim_t kNewAddressSpaceMaxSize = 0x100000000L; > + // Limit the address space to 32GB. This is in the hope of making some > + // kernel exploits more complex and less reliable. > + // This limit has to be very high because V8 and possibly others will > + // reserve memory ranges and rely on on-demand paging for allocation. > + // Unfortunately, even MADV_DONTNEED ranges count towards RLIMIT_AS so > + // this is not an option. > + const rlim_t kNewAddressSpaceMaxSize = 1L << 35; > struct rlimit old_address_space_limit; > if (getrlimit(RLIMIT_AS, &old_address_space_limit)) > return false; > > >
PTAL! I added a restriction to the heap size, as discussed. Will land once https://codereview.chromium.org/11857007 lands.
On Tue, Jan 15, 2013 at 12:14 AM, <jln@chromium.org> wrote: > PTAL! > > I added a restriction to the heap size, as discussed. Will land once > https://codereview.chromium.**org/11857007<https://codereview.chromium.org/11.... > > https://chromiumcodereview.**appspot.com/11783104/<https://chromiumcodereview... > From v8's point of view the combined changes look fine. Are there plans to merge this back to 3.15? Otherwise web workers stay severly limited and http://www.chaostoperfection.com/ won't work.
On Tue, Jan 15, 2013 at 7:15 AM, Sven Panne <svenpanne@chromium.org> wrote: > On Tue, Jan 15, 2013 at 12:14 AM, <jln@chromium.org> wrote: > >> PTAL! >> >> I added a restriction to the heap size, as discussed. Will land once >> https://codereview.chromium.**org/11857007<https://codereview.chromium.org/11.... >> >> https://chromiumcodereview.**appspot.com/11783104/<https://chromiumcodereview... >> > > From v8's point of view the combined changes look fine. Are there plans to > merge this back to 3.15? Otherwise web workers stay severly limited and > http://www.chaostoperfection.com/ won't work. > We'll get all of these changes into M25, if that's the release you're referring to?
On Tue, Jan 15, 2013 at 8:37 AM, Chris Evans <cevans@google.com> wrote: > We'll get all of these changes into M25, if that's the release you're > referring to? > Ooops, yes, I meant M25. FYI: I just tested a handful of web worker demos (from Mozilla etc.), and a lot of them fail with the current state of affairs, so merging back is really crucial, I think...
https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... File content/common/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:280: #if defined(__x86_64__) I wonder if this can be done more generically so that we get ARM64 correct out-of-the-box? BITS_PER_LONG maybe? https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:285: // MADV_DONTNEED ranges count towards RLIMIT_AS so this is not an option. Also, we believe this is a good defence to a know WebKit issue with ref count overflowing, for the case where an object needs to be allocated per ref. On 64-bit, the smallest tcmalloc allocation is 16 bytes, so 2^31 * 16 == 32GB address space. Not sure if that nuance is worth capturing in this comment :) https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:298: bool limited_data = AddResourceLimit(RLIMIT_DATA, kNewDataSegmentMaxSize); I wonder if this causes a subtle interaction? Really interested in your thoughts. With a 4GB brk() limit but a 16GB address space limit, something interesting will happen with a e.g. 8GB heap spray. After the first 4GB of heap is filled, brk() will start failing with ENOMEM due to the RLIMIT_DATA. At this moment, tcmalloc will start falling back to the mmap() allocator. How does mmap() decide where to put mappings on 64-bit Linux? For normal kernels, does it just stack them after the last successful mapping? If so, does this mean it might be placed inappropriately close to a v8 code mapping? Does the situation (i.e. mmap address allocation strategy) change on a grsecurity kernel? etc.?
PTAL! https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... File content/common/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:280: #if defined(__x86_64__) On 2013/01/15 09:07:56, Chris Evans wrote: > I wonder if this can be done more generically so that we get ARM64 correct > out-of-the-box? > > BITS_PER_LONG maybe? I hesitated a bit when I originally wrote this, but I then thought it was proper to list full architectures instead. But since I was on the fence, you convinced me, and I check for LP64 now. https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:285: // MADV_DONTNEED ranges count towards RLIMIT_AS so this is not an option. On 2013/01/15 09:07:56, Chris Evans wrote: > Also, we believe this is a good defence to a know WebKit issue with ref count > overflowing, for the case where an object needs to be allocated per ref. On > 64-bit, the smallest tcmalloc allocation is 16 bytes, so 2^31 * 16 == 32GB > address space. > Not sure if that nuance is worth capturing in this comment :) This should probably be captured in the bug. I added an explicit link to the bug in the comment. https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... content/common/sandbox_linux.cc:298: bool limited_data = AddResourceLimit(RLIMIT_DATA, kNewDataSegmentMaxSize); On 2013/01/15 09:07:56, Chris Evans wrote: > I wonder if this causes a subtle interaction? Really interested in your > thoughts. > > With a 4GB brk() limit but a 16GB address space limit, something interesting > will happen with a e.g. 8GB heap spray. After the first 4GB of heap is filled, > brk() will start failing with ENOMEM due to the RLIMIT_DATA. At this moment, > tcmalloc will start falling back to the mmap() allocator. > > How does mmap() decide where to put mappings on 64-bit Linux? For normal > kernels, does it just stack them after the last successful mapping? If so, does > this mean it might be placed inappropriately close to a v8 code mapping? Does > the situation (i.e. mmap address allocation strategy) change on a grsecurity > kernel? etc.? In release mode we try the brk allocator first, then mmap (in debug we do the reverse). I think there are plenty of things we could do in this area (but I would reserve them for another CL). I'm not too worried about v8, because I think v8 gives hints to mmap mappings to add ASLR (we should check). My inclination would be to disable the brk allocator and improve the mmap one. You are correct that on stock Linux, mmap without a hint will just "pile down". I would either add hints to the mmap allocator orn even bettern create guard mappings after every mmap. We should discuss this in another security improvement bug though.
On 2013/01/15 19:44:19, Julien Tinnes wrote: > PTAL! > > https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... > File content/common/sandbox_linux.cc (right): > > https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... > content/common/sandbox_linux.cc:280: #if defined(__x86_64__) > On 2013/01/15 09:07:56, Chris Evans wrote: > > I wonder if this can be done more generically so that we get ARM64 correct > > out-of-the-box? > > > > BITS_PER_LONG maybe? > > I hesitated a bit when I originally wrote this, but I then thought it was proper > to list full architectures instead. > > But since I was on the fence, you convinced me, and I check for LP64 now. > > https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... > content/common/sandbox_linux.cc:285: // MADV_DONTNEED ranges count towards > RLIMIT_AS so this is not an option. > On 2013/01/15 09:07:56, Chris Evans wrote: > > Also, we believe this is a good defence to a know WebKit issue with ref count > > overflowing, for the case where an object needs to be allocated per ref. On > > 64-bit, the smallest tcmalloc allocation is 16 bytes, so 2^31 * 16 == 32GB > > address space. > > Not sure if that nuance is worth capturing in this comment :) > > This should probably be captured in the bug. I added an explicit link to the bug > in the comment. > > https://chromiumcodereview.appspot.com/11783104/diff/12001/content/common/san... > content/common/sandbox_linux.cc:298: bool limited_data = > AddResourceLimit(RLIMIT_DATA, kNewDataSegmentMaxSize); > On 2013/01/15 09:07:56, Chris Evans wrote: > > I wonder if this causes a subtle interaction? Really interested in your > > thoughts. > > > > With a 4GB brk() limit but a 16GB address space limit, something interesting > > will happen with a e.g. 8GB heap spray. After the first 4GB of heap is filled, > > brk() will start failing with ENOMEM due to the RLIMIT_DATA. At this moment, > > tcmalloc will start falling back to the mmap() allocator. > > > > How does mmap() decide where to put mappings on 64-bit Linux? For normal > > kernels, does it just stack them after the last successful mapping? If so, > does > > this mean it might be placed inappropriately close to a v8 code mapping? Does > > the situation (i.e. mmap address allocation strategy) change on a grsecurity > > kernel? etc.? > > In release mode we try the brk allocator first, then mmap (in debug we do the > reverse). > > I think there are plenty of things we could do in this area (but I would reserve > them for another CL). I'm not too worried about v8, because I think v8 gives > hints to mmap mappings to add ASLR (we should check). > > My inclination would be to disable the brk allocator and improve the mmap one. > You are correct that on stock Linux, mmap without a hint will just "pile down". > I would either add hints to the mmap allocator orn even bettern create guard > mappings after every mmap. > > We should discuss this in another security improvement bug though. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/11783104/18001
Message was sent while issue was closed.
Change committed as 177037 |
