Linux Sandbox: add resource limits to NaCl

Linux Sandbox: add resource limits to NaCl

Add address space resource limits to NaCl processes.

BUG=455839
Committed: https://crrev.com/97718598d764d4798333044869c7639737bf88bf
Cr-Commit-Position: refs/heads/master@{#316286}

[jln (very slow on Chromium), 2015-02-13 00:14:34]

jln@chromium.org changed reviewers:
+ mseaborn@chromium.org

[jln (very slow on Chromium), 2015-02-13 00:14:41]

Mark: PTAL!

[jln (very slow on Chromium), 2015-02-13 05:20:10]

Mark: so nacl_loader builds with ASAN? I'm surprised (see the failure in PS#2):
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

[Mark Seaborn, 2015-02-13 17:11:54]

LGTM

https://codereview.chromium.org/919203002/diff/40001/components/nacl/loader/s...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/919203002/diff/40001/components/nacl/loader/s...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:82: // Add a limit to
the brk() heap that would prevent allocations that can't be
This probably has no benefit, because glibc will fall back to mmap() when brk()
allocations start failing.  You could perhaps add a comment along the lines of
"what the hell, might as well do this just in case it helps". :-)

https://codereview.chromium.org/919203002/diff/40001/components/nacl/loader/s...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:88: // 128 GB.
Can you add some explanation of how we picked this, using my explanation from
the issue, please?  i.e. Mention the 88GB minimum for x86-64 NaCl.

https://codereview.chromium.org/919203002/diff/40001/components/nacl/loader/s...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:91: const rlim_t
kNewAddressSpaceLimit = std::numeric_limits<uint32_t>::max();
Well, this is no limit at all. :-)  You might as well leave out the call in this
case and save a few bytes of code size.

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
File sandbox/linux/services/resource_limits.cc (right):

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
sandbox/linux/services/resource_limits.cc:8: #include <sys/resource.h>
Nit: sort #includes

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
File sandbox/linux/services/resource_limits.h (right):

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
sandbox/linux/services/resource_limits.h:18: class SANDBOX_EXPORT ResourceLimits
{
Aside: In my opinion, using a class that will never be instantiated as a
namespace is a slightly odd style.  But I suppose this is a common style in
sandbox/?

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
File sandbox/linux/services/resource_limits_unittests.cc (right):

https://codereview.chromium.org/919203002/diff/40001/sandbox/linux/services/r...
sandbox/linux/services/resource_limits_unittests.cc:21: // Not being able to
fork breaks LSAN, so disable on
LSAN -> LeakSanitizer perhaps.  (Mentioning LSAN and then ASAN looks like it
could be a typo if you don't know how they relate.)

[jln (very slow on Chromium), 2015-02-13 17:48:07]

New patchsets have been uploaded after l-g-t-m from mseaborn@chromium.org

[jln (very slow on Chromium), 2015-02-13 18:00:06]

Thanks Mark!

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:82: // Add a limit to
the brk() heap that would prevent allocations that can't be
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> This probably has no benefit, because glibc will fall back to mmap() when
brk()
> allocations start failing.  You could perhaps add a comment along the lines of
> "what the hell, might as well do this just in case it helps". :-)

Does NaCl always uses glibc malloc and not tcmalloc?

In tcmalloc I tuned that a while back and it was different between 32 bits and
64 bits and Release and Debug.

Done in any case!

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:88: // 128 GB.
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> Can you add some explanation of how we picked this, using my explanation from
> the issue, please?  i.e. Mention the 88GB minimum for x86-64 NaCl.

Done.

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:91: const rlim_t
kNewAddressSpaceLimit = std::numeric_limits<uint32_t>::max();
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> Well, this is no limit at all. :-)  You might as well leave out the call in
this
> case and save a few bytes of code size.

This is true because we're enabling this as part of the "layer-2" sandbox (with
seccomp).

But if we didn't have seccomp, this is definitely useful on IA32 if running
under a 64 bits kernel. I'll keep itn as it's nice to not have to rely on
seccomp and have some security in depth.

I added a comment.

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
File sandbox/linux/services/resource_limits.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
sandbox/linux/services/resource_limits.cc:8: #include <sys/resource.h>
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> Nit: sort #includes

Done.

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
File sandbox/linux/services/resource_limits.h (right):

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
sandbox/linux/services/resource_limits.h:18: class SANDBOX_EXPORT ResourceLimits
{
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> Aside: In my opinion, using a class that will never be instantiated as a
> namespace is a slightly odd style.  But I suppose this is a common style in
> sandbox/?

I think it's even the prevailing style in all of Chromium. base/ is definitely
the same and I see this all over the place. It's rare to see namespaces used
with file scopes.

Moreover "exporting" is much more concise with classes.

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
File sandbox/linux/services/resource_limits_unittests.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/40001/sandbox/linux/ser...
sandbox/linux/services/resource_limits_unittests.cc:21: // Not being able to
fork breaks LSAN, so disable on
On 2015/02/13 17:11:54, Mark Seaborn wrote:
> LSAN -> LeakSanitizer perhaps.  (Mentioning LSAN and then ASAN looks like it
> could be a typo if you don't know how they relate.)

Done.

[Mark Seaborn, 2015-02-13 18:33:41]

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/40001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:91: const rlim_t
kNewAddressSpaceLimit = std::numeric_limits<uint32_t>::max();
On 2015/02/13 18:00:05, jln wrote:
> On 2015/02/13 17:11:54, Mark Seaborn wrote:
> > Well, this is no limit at all. :-)  You might as well leave out the call in
> this
> > case and save a few bytes of code size.
> 
> This is true because we're enabling this as part of the "layer-2" sandbox
(with
> seccomp).
> 
> But if we didn't have seccomp, this is definitely useful on IA32 if running
> under a 64 bits kernel. I'll keep itn as it's nice to not have to rely on
> seccomp and have some security in depth.

Ah, good point.  I didn't think of that.

https://chromiumcodereview.appspot.com/919203002/diff/80001/components/nacl/l...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/80001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:100: // bits when
running under 64 bits kerneks. Set a limit in case this happens.
Typo: "kernels"

[jln (very slow on Chromium), 2015-02-13 18:58:30]

https://chromiumcodereview.appspot.com/919203002/diff/80001/components/nacl/l...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/919203002/diff/80001/components/nacl/l...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:100: // bits when
running under 64 bits kerneks. Set a limit in case this happens.
On 2015/02/13 18:33:41, Mark Seaborn wrote:
> Typo: "kernels"

Done.

[jln (very slow on Chromium), 2015-02-13 18:58:52]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2015-02-13 19:00:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919203002/100001

[commit-bot: I haz the power, 2015-02-13 20:58:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-13 20:59:51]

Patchset 6 (id:??) landed as
https://crrev.com/97718598d764d4798333044869c7639737bf88bf
Cr-Commit-Position: refs/heads/master@{#316286}

[Mark Seaborn, 2015-03-09 17:18:19]

Adding some context now that the rowhammer blog post is published (at
http://googleprojectzero.blogspot.com/2015/03/exploiting-dram-rowhammer-bug-t...):

This change is a rowhammer mitigation, albeit a relatively weak one.  This is a
mitigation to limit the PTE-spraying attack described in the blog post.

This change just brings the NaCl loader process into line with other Chromium
sandboxed processes, which had address space limits before.