Seccomp-BPF: add a new synthetic unittest

Seccomp-BPF: add a new synthetic unittest

This adds a synthetic but slightly more complex unittest for the BPF compiler.

BUG=130662
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145800

[jln (very slow on Chromium), 2012-06-27 22:49:00]

Yet another unittest.

Chris, do you mind taking a look while Markus is on vacation ?

[palmer, 2012-06-28 19:58:13]

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

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:202: // program by iterating
through all syscalls and checking for a errno that
Nit: "for an errno" :)

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:206: int
SysnoToRandomErrno(int sysno) {
I think this only makes sense if you have some context that I don't have. More
documentation? Is there some inherent relationship between contiguous system
call numbers?

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:221: else {
Nit: Two-line (one code line) block gets no curly braces, but one-line block
does? I'd probably go with braces for both, for clarity and consistency.

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:233: for (int syscall_number =
 static_cast<int>(MIN_SYSCALL);
Why are these static_casts necessary? Can we just declare syscall_number/sysno
as the right type, whatever that is?

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:242: errno !=
SysnoToRandomErrno(syscall_number)) {
I don't see why this is correct. Shouldn't we be expecting a specific errno
value?

[jln (very slow on Chromium), 2012-06-28 20:57:45]

Thanks Chris, PTAL!

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

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:202: // program by iterating
through all syscalls and checking for a errno that
On 2012/06/28 19:58:14, Chris P. wrote:
> Nit: "for an errno" :)

Done.

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:206: int
SysnoToRandomErrno(int sysno) {
On 2012/06/28 19:58:14, Chris P. wrote:
> I think this only makes sense if you have some context that I don't have. More
> documentation? Is there some inherent relationship between contiguous system
> call numbers?

Let me know if this is more clear.

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:221: else {
On 2012/06/28 19:58:14, Chris P. wrote:
> Nit: Two-line (one code line) block gets no curly braces, but one-line block
> does? I'd probably go with braces for both, for clarity and consistency.

Done.

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:233: for (int syscall_number =
 static_cast<int>(MIN_SYSCALL);
On 2012/06/28 19:58:14, Chris P. wrote:
> Why are these static_casts necessary? Can we just declare syscall_number/sysno
> as the right type, whatever that is?

Perhaps surprisingly, syscalls are actually signed, however MIN_SYSCALL /
MAX_SYSCALL are unsigned integer, for a reason that I don't remember.

https://chromiumcodereview.appspot.com/10693019/diff/3001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:242: errno !=
SysnoToRandomErrno(syscall_number)) {
On 2012/06/28 19:58:14, Chris P. wrote:
> I don't see why this is correct. Shouldn't we be expecting a specific errno
> value?

This is what this is doing, isn't it ? I added a comment, hopefully it's more
clear.

[jln (very slow on Chromium), 2012-06-29 22:31:18]

Ping ?

If anyone has a suggestion for a reviewer I could use while Markus is on
vacation, that would be appreciated.

[Chris Evans, 2012-06-29 22:34:05]

On 2012/06/29 22:31:18, Julien Tinnes wrote:
> Ping ?
> 
> If anyone has a suggestion for a reviewer I could use while Markus is on
> vacation, that would be appreciated.

Look at sandbox/OWNERS

[jln (very slow on Chromium), 2012-06-29 22:36:21]

On 2012/06/29 22:34:05, Chris Evans wrote:
> On 2012/06/29 22:31:18, Julien Tinnes wrote:
> > Ping ?
> > 
> > If anyone has a suggestion for a reviewer I could use while Markus is on
> > vacation, that would be appreciated.
> 
> Look at sandbox/OWNERS

It's Windows-only outside of Markus.

I'll just wait for Markus to come  back for the rest, it's almost the WE anyway
:)

[Chris Evans, 2012-06-29 22:38:42]

On 2012/06/29 22:36:21, Julien Tinnes wrote:
> On 2012/06/29 22:34:05, Chris Evans wrote:
> > On 2012/06/29 22:31:18, Julien Tinnes wrote:
> > > Ping ?
> > > 
> > > If anyone has a suggestion for a reviewer I could use while Markus is on
> > > vacation, that would be appreciated.
> > 
> > Look at sandbox/OWNERS
> 
> It's Windows-only outside of Markus.
> 
> I'll just wait for Markus to come  back for the rest, it's almost the WE
anyway
> :)

You'll be a full committer soon, at which time you can add yourself to OWNERS.
We should probably add sandbox/linux/OWNERS and add noparent. I'm happy to be
one of the OWNERS

[jln (very slow on Chromium), 2012-06-29 22:44:06]

On 2012/06/29 22:38:42, Chris Evans wrote:
> On 2012/06/29 22:36:21, Julien Tinnes wrote:
> > On 2012/06/29 22:34:05, Chris Evans wrote:
> > > On 2012/06/29 22:31:18, Julien Tinnes wrote:
> > > > Ping ?
> > > > 
> > > > If anyone has a suggestion for a reviewer I could use while Markus is on
> > > > vacation, that would be appreciated.
> > > 
> > > Look at sandbox/OWNERS
> > 
> > It's Windows-only outside of Markus.
> > 
> > I'll just wait for Markus to come  back for the rest, it's almost the WE
> anyway
> > :)
> 
> You'll be a full committer soon, at which time you can add yourself to OWNERS.
> We should probably add sandbox/linux/OWNERS and add noparent. I'm happy to be
> one of the OWNERS

The problem is not owership, it's code review. OWNER stamping is never an issue.

[Chris Evans, 2012-06-29 22:44:50]

On 2012/06/29 22:44:06, Julien Tinnes wrote:
> On 2012/06/29 22:38:42, Chris Evans wrote:
> > On 2012/06/29 22:36:21, Julien Tinnes wrote:
> > > On 2012/06/29 22:34:05, Chris Evans wrote:
> > > > On 2012/06/29 22:31:18, Julien Tinnes wrote:
> > > > > Ping ?
> > > > > 
> > > > > If anyone has a suggestion for a reviewer I could use while Markus is
on
> > > > > vacation, that would be appreciated.
> > > > 
> > > > Look at sandbox/OWNERS
> > > 
> > > It's Windows-only outside of Markus.
> > > 
> > > I'll just wait for Markus to come  back for the rest, it's almost the WE
> > anyway
> > > :)
> > 
> > You'll be a full committer soon, at which time you can add yourself to
OWNERS.
> > We should probably add sandbox/linux/OWNERS and add noparent. I'm happy to
be
> > one of the OWNERS
> 
> The problem is not owership, it's code review. OWNER stamping is never an
issue.

Looks like Chris P already jumped in to help with review?

[jln (very slow on Chromium), 2012-06-29 22:47:42]

On 2012/06/29 22:44:50, Chris Evans wrote:
> On 2012/06/29 22:44:06, Julien Tinnes wrote:
> > On 2012/06/29 22:38:42, Chris Evans wrote:
> > > On 2012/06/29 22:36:21, Julien Tinnes wrote:
> > > > On 2012/06/29 22:34:05, Chris Evans wrote:
> > > > > On 2012/06/29 22:31:18, Julien Tinnes wrote:
> > > > > > Ping ?
> > > > > > 
> > > > > > If anyone has a suggestion for a reviewer I could use while Markus
is
> on
> > > > > > vacation, that would be appreciated.
> > > > > 
> > > > > Look at sandbox/OWNERS
> > > > 
> > > > It's Windows-only outside of Markus.
> > > > 
> > > > I'll just wait for Markus to come  back for the rest, it's almost the WE
> > > anyway
> > > > :)
> > > 
> > > You'll be a full committer soon, at which time you can add yourself to
> OWNERS.
> > > We should probably add sandbox/linux/OWNERS and add noparent. I'm happy to
> be
> > > one of the OWNERS
> > 
> > The problem is not owership, it's code review. OWNER stamping is never an
> issue.
> 
> Looks like Chris P already jumped in to help with review?

Yep, but delays indicate that he's busy, so I don't think I should send him more
:)

Anyway, not a biggie, Markus will be back soon enough.

[jln (very slow on Chromium), 2012-07-09 17:51:55]

Monday ping?

[palmer, 2012-07-09 18:52:18]

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

https://chromiumcodereview.appspot.com/10693019/diff/7001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:209: // same errno.
To be honest, I still don't understand this. Maybe it's because I don't know
anything about how BPF will optimize policy expressions. (For example, what does
it mean to coalesce system calls, and why would it be done only for calls with
contiguous numbers?)

In my tiny mind, this unittest should look like this:

for n in range(MIN_SYSCALL, MAX_SYSCALL + 1):
    #
    # Make sure exit_group always works, and that nothing
    # else ever works.
    #
    if __NR_exit_group == n:
        assert(0 == syscall(n))
        assert(Sandbox::SB_ALLOWED == errno)
    else:
        assert(-1 == syscall(n))
        assert(Sandbox::SB_DENIED == errno)

This seems like essentially what you already are doing, except with some
additional stuff to handle BPF optimizations that I don't understand.

Maybe it is OK that I don't understand the optimization stuff, as long as it is
documented in some well-known place that future maintainers of this code will
know about.

[jln (very slow on Chromium), 2012-07-09 19:46:00]

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

https://chromiumcodereview.appspot.com/10693019/diff/7001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:209: // same errno.
On 2012/07/09 18:52:18, Chris P. wrote:
> To be honest, I still don't understand this. Maybe it's because I don't know
> anything about how BPF will optimize policy expressions. (For example, what
does
> it mean to coalesce system calls, and why would it be done only for calls with
> contiguous numbers?)

It's not the BPF interpreter, it's the BPF compiler itself (the code is in the
same directory). Markus should be back from vacation now, so feel free to skip.

If you have a range, from n to m, you can optimize with 'if (x < n or x > m)'
for instance.

> In my tiny mind, this unittest should look like this:

>     if __NR_exit_group == n:
>         assert(0 == syscall(n))

I wouldn't want to actually call exit_group() here.

>         assert(Sandbox::SB_ALLOWED == errno)
>     else:
>         assert(-1 == syscall(n))
>         assert(Sandbox::SB_DENIED == errno)
> 
> This seems like essentially what you already are doing, except with some
> additional stuff to handle BPF optimizations that I don't understand.

BPF apart, isn't this what SyntheticProcess() is doing?

> Maybe it is OK that I don't understand the optimization stuff, as long as it
is
> documented in some well-known place that future maintainers of this code will
> know about.

Well it's really a unit test for code in that directory. I understand how you
position as a reviewer is difficult given that you didn't get the opportunity to
write / review that code first.

[palmer, 2012-07-09 22:23:11]

lgtm

[commit-bot: I haz the power, 2012-07-09 22:37:00]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jln (very slow on Chromium), 2012-07-09 22:37:43]

On 2012/07/09 22:37:00, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Chris (Evans), would you mind approving this based on Chris P.'s review?

[Chris Evans, 2012-07-09 22:49:41]

On 2012/07/09 22:37:43, Julien Tinnes wrote:
> On 2012/07/09 22:37:00, I haz the power (commit-bot) wrote:
> > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > Even if an LGTM may have been provided, it was from a non-committer or
> > a lowly provisional committer, _not_ a full super star committer.
> > See http://www.chromium.org/getting-involved/become-a-committer
> > Note that this has nothing to do with OWNERS files.
> 
> Chris (Evans), would you mind approving this based on Chris P.'s review?

Rubber stamp LGTM

[commit-bot: I haz the power, 2012-07-09 22:51:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10693019/7001

[commit-bot: I haz the power, 2012-07-09 23:56:50]

Change committed as 145800