SECCOMP-BPF: Added supported for inspection system call arguments from BPF filters.

SECCOMP-BPF: Added supported for inspection system call arguments from BPF filters.


BUG=130662
TEST=sandbox_linux_unittests
NOTRY=true


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173243

[Markus (顧孟勤), 2012-12-05 10:42:19]

Julien, this CL is not 100% done, but it is close to finished. There are a few
remaining TODOs that I still need to work on. And my head is hurting too much to
do that tonight.

I am mailing this out early, in case you want to get a head start on the code
review. But if you don't have time, feel free to wait for the final version.

I have been fixing up style-guide issue as I ran into them -- unfortunately,
that then required fixing up other files that no longer compiled. This makes the
CL harder to read than should be necessary. I'll probably work on breaking these
changes out into a separate CL. So, for now, please ignore.

[jln (very slow on Chromium), 2012-12-06 00:35:00]

I glanced at it (except the unit test) and this look pretty good.

I've very happy you're correcting some of the style issues. It'd be great to
make it a separate CL indeed.

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/sandb...
File sandbox/linux/sandbox_linux.gypi (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/sandb...
sandbox/linux/sandbox_linux.gypi:70: # NOTE: Order matters, if building w/ ninja
That is puzzling. It would be worth checking with Evan and check it's not due to
a bug somewhere.

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:715: // CPU registers.
It's even worse than that and is architecture dependent.

On IA32, the sixth argument is on the stack.

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:73: #define SECCOMP_RET_INVALID
0x00010000U  // Illegal return value
Maybe make it clear that's it's not an upstream value ?

Also why does this work? linux/seccomp.h is commented above, is that why ?

Shouldn't you uncomment it and put this one out of the #ifdef ? (Which should be
a #if !defined() btw).

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:83: #if defined(__arm__) &&
(defined(__thumb__) || defined(__ARM_EABI__))
I'm curious: what broke here ?

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/verifier.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/verifier.cc:75: data->args[code.argno_] = code.value_
^ 0x55AA55AA;
I most definitely need a comment here :)

[jln (very slow on Chromium), 2012-12-06 01:48:12]

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:488: class EqualityStressTest
{
I didn't read this yet. But please consider at least adding a couple of small
tests that can be used as an example, as well as a quick check of the public
API.

[Markus (顧孟勤), 2012-12-12 02:43:18]

I still need to address your initial comments, and I need to break out the part
of the CL that just fixes up style violations.

But other than that, I think this is now feature-complete.

It was surprisingly painful to wrap my head around all the intricacies of what
happens when we are dealing with 32bit arguments on a 64bit system. I believe,
the unittests now cover all these corner cases, and so does the Verifier. But I
can now make a very strong argument why nobody should ever try to write BPF
filters by hand. The fact that BPF programs are limited to 32bit operations is
really annoying.

[jln (very slow on Chromium), 2012-12-12 19:25:26]

Didn't have time to look yet. But could you look into
EqualityArgumentUnallowed64bit failing on linux_rel ? I guess BPF_DEATH don't
work if there is no BPF support for some reason ?

[Markus (顧孟勤), 2012-12-12 20:54:35]

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:715: // CPU registers.
I am almost certain this is a red herring. Yes, for SYSENTER, the argument is
placed on the stack. But the kernel then quickly retrieves it from there and the
rest of the code path is identical to what we are doing for "int $0x80". This
all happens before BPF filters come into play. So, no risk of any race
conditions.

On 2012/12/06 00:35:00, Julien Tinnes wrote:
> It's even worse than that and is architecture dependent.
> 
> On IA32, the sixth argument is on the stack.

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:73: #define SECCOMP_RET_INVALID
0x00010000U  // Illegal return value
We cannot uncomment the include statement, as the code will then not compile on
our older build bots. Unfortunately, there is no way in C to say "try to include
this file, iff it exists".

So, for now, we just document that we would like to include it. But we don't do
so ... yet. If our build bots are updated at some point, we can revisit that
decision -- but overall, it is harmless to leave it commented out.

What I expect to happen is that in the future, when we refactor this whole file,
we will have a "ports.h" file that makes all of this a lot clearer. That's what
we have traditionally been doing both in Google3 and in other open source
projects released by Google.

On 2012/12/06 00:35:00, Julien Tinnes wrote:
> Maybe make it clear that's it's not an upstream value ?
> 
> Also why does this work? linux/seccomp.h is commented above, is that why ?
> 
> Shouldn't you uncomment it and put this one out of the #ifdef ? (Which should
be
> a #if !defined() btw).

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:488: class EqualityStressTest
{
I believe that is done now. The tests at the end of this file should fit the
requirements of being small. Or do you want something different?

On 2012/12/06 01:48:12, Julien Tinnes wrote:
> I didn't read this yet. But please consider at least adding a couple of small
> tests that can be used as an example, as well as a quick check of the public
> API.

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:83: #if defined(__arm__) &&
(defined(__thumb__) || defined(__ARM_EABI__))
I believe one of our compilers (clang?) didn't like that I am not using the
"num" argument. This was easiest to fix by adjusting the function signature and
leaving out the parameter name.

And at that point, moving the "#if" statements around made the code a lot more
readable than what it was before.

On 2012/12/06 00:35:00, Julien Tinnes wrote:
> I'm curious: what broke here ?

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/verifier.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/verifier.cc:75: data->args[code.argno_] = code.value_
^ 0x55AA55AA;
On 2012/12/06 00:35:00, Julien Tinnes wrote:
> I most definitely need a comment here :)

Done.

[Jorge Lucangeli Obes, 2012-12-14 02:11:04]

On 2012/12/12 20:54:35, Markus (顧孟勤) wrote:
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/sandbox_bpf.cc:715: // CPU registers.
> I am almost certain this is a red herring. Yes, for SYSENTER, the argument is
> placed on the stack. But the kernel then quickly retrieves it from there and
the
> rest of the code path is identical to what we are doing for "int $0x80". This
> all happens before BPF filters come into play. So, no risk of any race
> conditions.
> 
> On 2012/12/06 00:35:00, Julien Tinnes wrote:
> > It's even worse than that and is architecture dependent.
> > 
> > On IA32, the sixth argument is on the stack.
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/sandbox_bpf.h:73: #define SECCOMP_RET_INVALID
> 0x00010000U  // Illegal return value
> We cannot uncomment the include statement, as the code will then not compile
on
> our older build bots. Unfortunately, there is no way in C to say "try to
include
> this file, iff it exists".
> 
> So, for now, we just document that we would like to include it. But we don't
do
> so ... yet. If our build bots are updated at some point, we can revisit that
> decision -- but overall, it is harmless to leave it commented out.
> 
> What I expect to happen is that in the future, when we refactor this whole
file,
> we will have a "ports.h" file that makes all of this a lot clearer. That's
what
> we have traditionally been doing both in Google3 and in other open source
> projects released by Google.
> 
> On 2012/12/06 00:35:00, Julien Tinnes wrote:
> > Maybe make it clear that's it's not an upstream value ?
> > 
> > Also why does this work? linux/seccomp.h is commented above, is that why ?
> > 
> > Shouldn't you uncomment it and put this one out of the #ifdef ? (Which
should
> be
> > a #if !defined() btw).
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:488: class
EqualityStressTest
> {
> I believe that is done now. The tests at the end of this file should fit the
> requirements of being small. Or do you want something different?
> 
> On 2012/12/06 01:48:12, Julien Tinnes wrote:
> > I didn't read this yet. But please consider at least adding a couple of
small
> > tests that can be used as an example, as well as a quick check of the public
> > API.
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/syscall_iterator.cc:83: #if defined(__arm__) &&
> (defined(__thumb__) || defined(__ARM_EABI__))
> I believe one of our compilers (clang?) didn't like that I am not using the
> "num" argument. This was easiest to fix by adjusting the function signature
and
> leaving out the parameter name.
> 
> And at that point, moving the "#if" statements around made the code a lot more
> readable than what it was before.
> 
> On 2012/12/06 00:35:00, Julien Tinnes wrote:
> > I'm curious: what broke here ?
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/verifier.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11411254/diff/2001/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/verifier.cc:75: data->args[code.argno_] =
code.value_
> ^ 0x55AA55AA;
> On 2012/12/06 00:35:00, Julien Tinnes wrote:
> > I most definitely need a comment here :)
> 
> Done.

Unit tests pass on ARM.

[jln (very slow on Chromium), 2012-12-14 02:28:02]

Looks good in general, with a few nits.

I didn't look deep into everything yet, but I'm very happy that there is ample
testing :)

The most important remark is that you might want to make use of the system calls
iterator that already exists.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/sand...
File sandbox/linux/sandbox_linux.gypi (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/sand...
sandbox/linux/sandbox_linux.gypi:79: 'seccomp-bpf/sandbox_bpf.h',
.h should be after .cc.
Is this an artifact of putting it back into place?

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/die.h (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/die.h:8: 
This line always wants to come back for some reason ;)

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (left):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:6: #if __BYTE_ORDER == __BIG_ENDIAN
You don't want to keep something around since it was never tested on big endian
?

I don't feel strongly either way.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:289: void Sandbox::SetProcFd(int
proc_fd) {
Should be set_proc_fd() since it's a mutator.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:715: // BPF programs operate on 32bit
entities. Load both halfs of the 64bit
s/halfs/halves.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:763: #if __SIZEOF_POINTER__ > 4
I'll trust the test on this, my brain is not working properly right now.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:392: static int ProcFd() { return
proc_fd_; }
Nit: for accessors, the correct is indeed proc_fd().

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:488: class EqualityStressTest
{
FYI Note: gtest allows "fixture classes". We should make use of this at some
point.

When I started the sandbox test framework I assumed we would have very simple
tests and not need this. It's too bad that now it'll be very difficult to move
away from having tests as functions.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:492: srand(0);
Ideally we would even have our mersene twister generator included. We're a bit
on a schedule right now, maybe a TODO ?

This is really something that gtest should provide.

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:499: for (int sysno = 0, end =
kNumTestCases; sysno < end; ++sysno) {
This should almost certainly make use of the SyscallIterator class, no ?

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:579: sysno ==
__NR_restart_syscall;
On ARM we should include the private system calls as well, as they are part of
the ABI.

Also, don't we want sigreturn here ?

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:21: }
We should perhaps make this a COMPILE_ASSERT ?

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/verifier.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/verifier.cc:150: case SECCOMP_RET_TRAP:
nit: indent the case:

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/test...
File sandbox/linux/tests/unit_tests.h (right):

https://chromiumcodereview.appspot.com/11411254/diff/16001/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:30: // A SANDBOX_DEATH_TEST is just like a
SANDBOX_TEST (see beloc), but it assumes
s/beloc/below

[jln (very slow on Chromium), 2012-12-14 23:33:12]

https://chromiumcodereview.appspot.com/11411254/diff/30001/content/common/san...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/11411254/diff/30001/content/common/san...
content/common/sandbox_seccomp_bpf_linux.cc:1406: Sandbox::startSandbox();
You missed renaming these.

[jln (very slow on Chromium), 2012-12-14 23:42:02]

lgtm

Let's land this and we can iterate on some of the details later.

[commit-bot: I haz the power, 2012-12-15 00:21:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/11411254/18008

[commit-bot: I haz the power, 2012-12-15 00:34:55]

Change committed as 173243