Add basic ARM support to the seccomp-bpf sandbox.

Add basic ARM support to the seccomp-bpf sandbox.

BUG=141157
TEST=unit tests on daisy.


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

[Jorge Lucangeli Obes, 2012-08-08 16:29:26]

Make seccomp-bpf ARM-ready.

PTAL

[Markus (顧孟勤), 2012-08-08 18:35:54]

lgtm

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

https://chromiumcodereview.appspot.com/10827223/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # ifndef EM_ARM
Julien pointed out to me, that our style guide apparently prefers "#if
!defined()" over "#ifndef". I personally don't care much. But since it's an easy
change, we should probably pay attention to it.

[jln (very slow on Chromium), 2012-08-08 18:40:34]

On 2012/08/08 18:35:54, Markus (顧孟勤) wrote:

> Julien pointed out to me, that our style guide apparently prefers "#if
> !defined()" over "#ifndef". I personally don't care much. But since it's an
easy
> change, we should probably pay attention to it.

Indeed!

Overall looks ok, but did you make sure the unittests pass ? If you can think of
anything arm specific you want to test, please add more unittests. It's ok if
some of those unittests are behind ifdefs.

Julien

[Jorge Lucangeli Obes, 2012-08-09 01:16:18]

I added support for compiling the unit test binary on ARM. A single test is
failing weird, and I suspect it's because ARM has a hole in its syscall range.
All other tests are passing =)

PTAL

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

https://chromiumcodereview.appspot.com/10827223/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # ifndef EM_ARM
On 2012/08/08 18:35:55, Markus (顧孟勤) wrote:
> Julien pointed out to me, that our style guide apparently prefers "#if
> !defined()" over "#ifndef". I personally don't care much. But since it's an
easy
> change, we should probably pay attention to it.

Done.

[Jorge Lucangeli Obes, 2012-08-09 21:27:42]

PTAL

[jln (very slow on Chromium), 2012-08-09 22:41:23]

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:86: 
It looks as if most of this really shouldn't be exposed in the interface at all
and should be moved to the .cc or a _impl.h file.

Markus, do you mind taking care of that in another CL ? This interface file is
getting very confusing and all the important stuff is hidden.

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:118: #define MAX_SYSCALL  ((unsigned
int)__ARM_NR_BASE + 16u)
There is the magic cmpxchg system call as well.

We don't want to forbid it, since the kernel uses it internally and it would go
through the filter.

It looks like the "implicit" maximum syscall number for arm is in reality
__NR_SYSCALL_BASE+0x0fffff.

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # if !defined(EM_ARM)
Why do we need this at all ?

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:265: int sysno_ceiling =
kArmPublicSysnoCeiling;
These should be const.

[Jorge Lucangeli Obes, 2012-08-09 22:58:25]

PTAL, thanks

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:118: #define MAX_SYSCALL  ((unsigned
int)__ARM_NR_BASE + 16u)
On 2012/08/09 22:41:23, Julien Tinnes wrote:
> There is the magic cmpxchg system call as well.
> 
> We don't want to forbid it, since the kernel uses it internally and it would
go
> through the filter.
> 
> It looks like the "implicit" maximum syscall number for arm is in reality
> __NR_SYSCALL_BASE+0x0fffff.
> 

Yeah, that syscall is weird. I wasn't sure if it would go through the filter.

Done.

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # if !defined(EM_ARM)
On 2012/08/09 22:41:23, Julien Tinnes wrote:
> Why do we need this at all ?

If not the AUDIT_ARCH_ARM value is not defined.

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:265: int sysno_ceiling =
kArmPublicSysnoCeiling;
On 2012/08/09 22:41:23, Julien Tinnes wrote:
> These should be const.

Done.

[jln (very slow on Chromium), 2012-08-09 23:16:28]

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # if !defined(EM_ARM)
On 2012/08/09 22:58:26, Jorge Lucangeli Obes wrote:
> On 2012/08/09 22:41:23, Julien Tinnes wrote:
> > Why do we need this at all ?
> 
> If not the AUDIT_ARCH_ARM value is not defined.

Did you intend to add an #include here ? Something in wrong, EM_ARM here can't
possibly influence AUDIT_ARCH_ARM just below.

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

On 2012/08/09 23:16:28, Julien Tinnes wrote:
>
https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
> File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):
> 
>
https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
> sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # if !defined(EM_ARM)
> On 2012/08/09 22:58:26, Jorge Lucangeli Obes wrote:
> > On 2012/08/09 22:41:23, Julien Tinnes wrote:
> > > Why do we need this at all ?
> > 
> > If not the AUDIT_ARCH_ARM value is not defined.
> 
> Did you intend to add an #include here ? Something in wrong, EM_ARM here can't
> possibly influence AUDIT_ARCH_ARM just below.

Hmm, crazy macro expansion. LGTM.

[Jorge Lucangeli Obes, 2012-08-10 06:21:15]

On 2012/08/09 23:22:07, Julien Tinnes wrote:
> On 2012/08/09 23:16:28, Julien Tinnes wrote:
> >
>
https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
> > File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10827223/diff/9/sandbox/linux/seccomp-...
> > sandbox/linux/seccomp-bpf/sandbox_bpf.h:121: # if !defined(EM_ARM)
> > On 2012/08/09 22:58:26, Jorge Lucangeli Obes wrote:
> > > On 2012/08/09 22:41:23, Julien Tinnes wrote:
> > > > Why do we need this at all ?
> > > 
> > > If not the AUDIT_ARCH_ARM value is not defined.
> > 
> > Did you intend to add an #include here ? Something in wrong, EM_ARM here
can't
> > possibly influence AUDIT_ARCH_ARM just below.
> 
> Hmm, crazy macro expansion. LGTM.

android trybot failure looks unrelated. Let's see what the CQ thinks.

[commit-bot: I haz the power, 2012-08-10 06:22:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10827223/5002

[commit-bot: I haz the power, 2012-08-10 08:11:49]

Change committed as 151007