Seccomp sandbox changes (performance and correctness fixes, primarily targetting x86-32)

- Add a custom allocator for STL objects. This fixes sandbox failures that
 were observed on some machines (in particular in 32bit mode).

- Some more changes to avoid calling into glibc when we can make a direct
 system call, instead. These particular call sites were unlikely to cause
 any problems. But it makes the code easier to audit if we avoid all
 unnecessary calls into glibc.

- In 64bit mode, gettimeofday() is handled by vsyscalls and tends to be cheap.
 In 32bit mode, it is just a regular system call. Some users rely on being
 able to call gettimeofday() at a very high rate (up to thousands of
 consecutive calls). Recognize this system call pattern and optimize for it.

- Add debugging option that allows us to warn about expensive system calls.
 In many cases, these warnings can then be used to optimize the sandboxed
 application.

- Fix compilation on newer versions of gcc.

- Changed the x86-32 version of the code that we use when intercepting
 system calls. Previously, we would use CALL to jump to the set of
 instructions that we had relocated. But we made the mistake of allowing
 relocation of instructions that reference %esp. This doesn't work, as
 CALL modifies the stack. We now avoid using CALL and instead jump
 directly. On x86-32 that requires the use of a PUSH/RET combination as
 there is no 32bit wide JMP instruction.
 The x86-64 version of the code was already written in a way that would
 avoid this particular problem.
 (I would like to thank Craig Schlenter for his exceptional detective
  work in tracking down the root cause of this bug!)

- For debugging purposes, injected a really small library (less than 4kB)
 and discovered that some of our memory map manipulations implicitly
 relied on mappings to be at least two pages long. Fixed the code that
 made this incorrect assumption.

- For really small libraries, the runtime linker can choose a different
 more compact layout. Our computation of the ASR offset did not know how
 to deal with that. Fixed by explicitly looking for a ".text" segment
 instead of looking for a PT_DYNAMIC section.

- Closed a file descriptor that we kept open longer than needed.

- Removed some unused code.

- Added copyright headers

TEST=tested on i386 and x86-64
BUG=36133
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40900

[Markus (顧孟勤), 2010-03-03 02:01:24]

Yesterday night, I finally found a bug that might explain the problems that
Craig has been seeing.

In the past, I had used STL container objects for some of the data collection
that was needed when finding where we need to patch glibc. I knew that this was
pushing my luck a little bit, but it had always seemed to work in all of my
tests.

I was finally able to isolate an example (on 32bit only for some reason), where
this wouldn't work. We could end up patching the system calls in the glibc
memory allocator while we needed to allocate some more temporary memory.

This obviously wasn't a good idea. I now explicitly provide my own (rather
simple-minded) allocator for all STL container objects. And things work much
better. No more failures in 32bit mode for me.

Craig, can you let me know whether this fixes the problems that you were seeing?

Also, I finally managed to clean up my performance and my debugging patches that
I had pending for a while. Again, these are mostly interesting for 32bit mode.
64bit mode has fewer performance issues thanks to the ability to use vsyscalls
for gettimeofday().

I added all three interested parties (Adam, Evan, Craig) as the reviewers. I
need at least one of you guys to give me a LGTM. But I am OK if some of you bow
out. I know it is hard to find reviewers for this type of code.

[Craig, 2010-03-03 05:36:47]

> Craig, can you let me know whether this fixes the problems that you were
seeing?

Unfortunately I'm still seeing socketpair fail, both in the standalone sandbox
and in chrome. I've been using this patch in the standalone sandbox:

--- test.cc	(revision 60)
+++ test.cc	(working copy)
@@ -98,7 +98,8 @@
   #endif
 
   int pair[2];
-  socketpair(AF_UNIX, SOCK_STREAM, 0, pair);
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) == -1)
+    perror("Socketpair fails: ");
 
   printf("uid: %d\n", getuid());
   dlopen("libncurses.so.5", RTLD_LAZY)

and the failure is:

"Socketpair fails: : Address family not supported by protocol"
(in chrome that is fatal inside libevent)

I have been slowly sifting through the sandbox code for the last few days
but it's going slowly as there is lots of interesting magic to wrap my head
around so I haven't conclusively figured out where the problem is yet ...

> I added all three interested parties (Adam, Evan, Craig) as the reviewers. I
> need at least one of you guys to give me a LGTM. But I am OK if some of you
bow
> out. I know it is hard to find reviewers for this type of code.

I'm probably going to opt out :( ... I have looked through the changes and at
first
glance they seem ok, but my assembly skills and knowledge of the sandbox are
still weak enough that I don't feel that comfortable reviewing this.

The only thing I did spot btw. is that devnull in sandbox.cc doesn't seem to be
closed
but that doesn't seem to be too problematic.

Thank you.

[Craig Schlenter, 2010-03-03 10:52:04]

On Wed, Mar 3, 2010 at 7:36 AM,  <craig.schlenter@chromium.org> wrote:
> The only thing I did spot btw. is that devnull in sandbox.cc doesn't seem to
> be closed but that doesn't seem to be too problematic.

I see this is in the child process that terminates shortly thereafter
anyway so this is totally ignorable.

--Craig

> http://codereview.chromium.org/661438
>

[agl, 2010-03-03 15:31:42]

LGTM

http://codereview.chromium.org/661438/diff/1019/19
File sandbox/linux/seccomp/allocator.cc (right):

http://codereview.chromium.org/661438/diff/1019/19#newcode1
sandbox/linux/seccomp/allocator.cc:1: // The allocator is very simplistic. It
requests memory pages directly from
Copyright block?

http://codereview.chromium.org/661438/diff/1019/19#newcode13
sandbox/linux/seccomp/allocator.cc:13: #include <stdint.h>
This code all rather assumes a 4k page size. I don't know how generally true
that is on other platforms like ARM. Is there a runtime check that the pagesize
is what we think it is anywhere?

http://codereview.chromium.org/661438/diff/1019/19#newcode46
sandbox/linux/seccomp/allocator.cc:46: size_t used;
maybe comment something like:

"used: the number of bytes currently allocated in this page. Note that, as
elements are freed from this page this is updated allowing us to track when the
page is free. However, these holes in the page are never used so |tail| is the
only way to find out how much free space remains."

http://codereview.chromium.org/661438/diff/1019/19#newcode53
sandbox/linux/seccomp/allocator.cc:53: size_t len = (size + 3) & ~3;
if (size + 3 < size)
  return NULL

(maybe)

http://codereview.chromium.org/661438/diff/1019/19#newcode57
sandbox/linux/seccomp/allocator.cc:57: size_t remainder = last_alloc->total_len
-
There's a |used| member in struct Header, but it's not used in this calculation?

Later: ah, I see.

http://codereview.chromium.org/661438/diff/1019/19#newcode70
sandbox/linux/seccomp/allocator.cc:70: size_t total_len = (sizeof(Header) + len
+ 4095) & ~4095;
if (len + 4095 + sizeof(Header) < len)
  return NULL

(maybe)

http://codereview.chromium.org/661438/diff/1019/19#newcode78
sandbox/linux/seccomp/allocator.cc:78: // If we were only asked to allocated a
single page, then we will use any
s/allocated/allocate/

http://codereview.chromium.org/661438/diff/1019/19#newcode93
sandbox/linux/seccomp/allocator.cc:93: size_t len = (size + 3) & ~3;
same overflow check, maybe?

http://codereview.chromium.org/661438/diff/1019/26
File sandbox/linux/seccomp/allocator.h (right):

http://codereview.chromium.org/661438/diff/1019/26#newcode1
sandbox/linux/seccomp/allocator.h:1: // Implement a very basic memory allocator
that make direct system calls
Copyright block?

http://codereview.chromium.org/661438/diff/1019/26#newcode18
sandbox/linux/seccomp/allocator.h:18: template <class T> class SystemAllocator :
SystemAllocatorHelper {
style nit. Typically this would be written as

template<class T>
class SystemAllocator : ....

http://codereview.chromium.org/661438/diff/1019/26#newcode28
sandbox/linux/seccomp/allocator.h:28: template <class U> struct rebind {
(it matters less on nested classes)

http://codereview.chromium.org/661438/diff/1019/26#newcode35
sandbox/linux/seccomp/allocator.h:35: const_pointer address(const_reference
value) const {
nit: new line before this

http://codereview.chromium.org/661438/diff/1019/26#newcode49
sandbox/linux/seccomp/allocator.h:49: return (pointer)sys_allocate(num *
sizeof(T));
are we really sure that greater than max_size() will never be asked for? If not,
then maybe:

if (num > max_size())
  return NULL;

http://codereview.chromium.org/661438/diff/1019/26#newcode69
sandbox/linux/seccomp/allocator.h:69: }
nit: new line after this

http://codereview.chromium.org/661438/diff/1019/24
File sandbox/linux/seccomp/syscall.cc (right):

http://codereview.chromium.org/661438/diff/1019/24#newcode159
sandbox/linux/seccomp/syscall.cc:159: "cmp  %eax, %fs:0x102C-0x58\n"
Please forgive my lack of AT&T syntax knowledge, but what does the %fs:... mean?
Just a far memory read?

http://codereview.chromium.org/661438/diff/1019/25
File sandbox/linux/seccomp/trusted_thread.cc (right):

http://codereview.chromium.org/661438/diff/1019/25#newcode159
sandbox/linux/seccomp/trusted_thread.cc:159: "mov  $102f-101f, %%rdi\n"   // len
= strlen(msg)
rdi -> rdx?

[Markus (顧孟勤), 2010-03-06 02:56:09]

Thanks to Craig's excellent detective work, we now know why the code was
crashing on his machine. We made a mistake instrumenting code that referenced
%esp. The best option to fix this problem was to fix our code so that we never
run any relocated instructions after having changed the stack layout.

Essentially, this means replacing all CALLs with a JMPs. The exact details were
a little more difficult, as x86-32 doesn't actually have any 32bit JMP
instruction. But this should give you a sense of what the code change does.

I tested the change by injecting a small library that deliberate was coded so
that it would included the assembly instructions that Craig had noticed in his
version of glibc. After making the changes, my test application ran
successfully.

Moreover, by using a tiny (less than 4kB) library, I uncovered a few more subtle
corner cases in how we deal with memory mapped libraries. They are now fixed.

http://codereview.chromium.org/661438/diff/1019/19
File sandbox/linux/seccomp/allocator.cc (right):

http://codereview.chromium.org/661438/diff/1019/19#newcode13
sandbox/linux/seccomp/allocator.cc:13: #include <stdint.h>
For x86-32/64, Linus put his foot down at one point and categorically stated
that the page size is never going to change. That actually makes a lot of sense,
as this assumption is part of the ELF specification.

For ARM, I think some embedded platforms decide to use a 2kB pagesize. No idea
how common that is. We'll have to make a bunch of changes like this, when we
port the code to ARM. But I'll deal with that, when we get there.

The allocator is easy to adjust, if needed. But some of the assembly code is
more difficult. We probably will need to do a compile-time rather than a
run-time check.

http://codereview.chromium.org/661438/diff/1019/24
File sandbox/linux/seccomp/syscall.cc (right):

http://codereview.chromium.org/661438/diff/1019/24#newcode159
sandbox/linux/seccomp/syscall.cc:159: "cmp  %eax, %fs:0x102C-0x58\n"
This is memory access through the %fs segment register. That's how we get access
to the TLS area that trust_thread.cc set up for us. The exact memory layout is
described in a comment in trusted_thread.cc and matches the definition in
securemem.h. Unfortunately, gcc inline assembly isn't sufficiently powerful to
use symbolic identifiers...

http://codereview.chromium.org/661438/diff/1019/25
File sandbox/linux/seccomp/trusted_thread.cc (right):

http://codereview.chromium.org/661438/diff/1019/25#newcode159
sandbox/linux/seccomp/trusted_thread.cc:159: "mov  $102f-101f, %%rdi\n"   // len
= strlen(msg)
Good catch. Thanks for spotting that.

[Craig, 2010-03-06 15:09:51]

Both the standalone and chromium versions of the sandbox work for me now. Yay!!!
Thank you!

http://codereview.chromium.org/661438/diff/2001/3007
File sandbox/linux/seccomp/library.h (right):

http://codereview.chromium.org/661438/diff/2001/3007#newcode66
sandbox/linux/seccomp/library.h:66: if (prot & PROT_EXEC == 0) {
This needs some brackets please:

if ((prot & PROT_EXEC) == 0) {

gcc 4.4 complained about this but only in the stand-alone sandbox ... it seems
as if sandbox.gyp might want to set  chromium_code = 1 (but that seems to turn
on lots of settings ... not sure what effect those will have on the windows
sandbox code) or add cflags -Wall -Wextra.

[Craig, 2010-03-06 15:11:13]

On 2010/03/06 15:09:51, Craig wrote:
> or add cflags -Wall -Wextra.

whoops and -Werror too of course ...

[agl, 2010-03-08 14:22:58]

LGTM in case you were waiting for it.