Linux sandbox: workaround colliding system call.

Linux sandbox: workaround colliding system call.

Some Android kernels have system calls that collide with sys_seccomp,
try to detect this situation.

BUG=478891
Committed: https://crrev.com/c08969c1b8b723494569c893b15c9b2e634ef0fc
Cr-Commit-Position: refs/heads/master@{#326667}

[jln (very slow on Chromium), 2015-04-20 20:37:35]

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

[Robert Sesek, 2015-04-20 20:44:49]

A potential issue even with this probing is that in sys_set_media_ext(), we'll
still end clearing savedfileExtList since the memset to zero is hit
unconditionally. Not sure there's a way to probe non-destructive.

[leecam, 2015-04-20 20:55:44]

On 2015/04/20 20:44:49, Robert Sesek wrote:
> A potential issue even with this probing is that in sys_set_media_ext(), we'll
> still end clearing savedfileExtList since the memset to zero is hit
> unconditionally. Not sure there's a way to probe non-destructive.

Yeah frustrating!

There is another syscall it clobbers too but thats unconditionally destructive
too.

[palmer, 2015-04-21 03:28:43]

palmer@chromium.org changed reviewers:
+ palmer@chromium.org

[palmer, 2015-04-21 03:28:44]

I think it's important to be as clear as is possible in these comments, since
what is happening is pretty out of the ordinary.

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall,
sys_set_media_ext  with the same number as
Typo: s/  /, /

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:70: // This has to be seen as a NULL
pointer by sys_set_media_ext to not crash,
Nit: I think this comment could be stated more clearly. One idea:

sys_set_media_ext will see this as NULL, which should be a safe (non-crashing)
way to invoke it. A genuine seccomp syscall will see it as
|SECCOMP_SET_MODE_STRICT|, which is also safe.

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:76: // A genuine kernel would EINVAL,
or at the very least return some kind of
"...would return -1 and set errno to EINVAL. Any other behavior indicates that
whatever code received our syscall was not the real seccomp."

[Robert Sesek, 2015-04-22 15:24:17]

Based on the internal discussion, I think I'm OK moving forward with this patch
once Chris' nits are addressed.

[jln (very slow on Chromium), 2015-04-23 00:02:51]

PTAL!

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall,
sys_set_media_ext  with the same number as
On 2015/04/21 03:28:43, palmer wrote:
> Typo: s/  /, /

Done.

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:70: // This has to be seen as a NULL
pointer by sys_set_media_ext to not crash,
On 2015/04/21 03:28:43, palmer wrote:
> Nit: I think this comment could be stated more clearly. One idea:
> 
> sys_set_media_ext will see this as NULL, which should be a safe (non-crashing)
> way to invoke it. A genuine seccomp syscall will see it as
> |SECCOMP_SET_MODE_STRICT|, which is also safe.

Done.

https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:76: // A genuine kernel would EINVAL,
or at the very least return some kind of
On 2015/04/21 03:28:44, palmer wrote:
> "...would return -1 and set errno to EINVAL. Any other behavior indicates that
> whatever code received our syscall was not the real seccomp."

Done.

[Robert Sesek, 2015-04-23 17:54:03]

Do we want to restrict this even further, like limiting it to kernel 3.10 (which
is where we know this collides)? It'd be even better if we could limit it to
some kind of vendor tag, though I don't know if that's available to us without
JNI on Android.

[jln (very slow on Chromium), 2015-04-23 17:59:59]

On 2015/04/23 17:54:03, Robert Sesek wrote:
> Do we want to restrict this even further, like limiting it to kernel 3.10
(which
> is where we know this collides)? It'd be even better if we could limit it to
> some kind of vendor tag, though I don't know if that's available to us without
> JNI on Android.

I'm not too worried about it given that we're doing SECCOMP_SET_MODE_STRICT,
which is unlikely to ever evolve. Moreover, the seccomp system call is well
designed insofar as it rejects unknown flags (which is a much more future way to
do things). So in a way this is a pretty reasonable way to detect seccomp
squatting in a fairly generic way.

Matthew also suggested that we could try to see if uname(2) gives us some
lg-specific info. Or we could try and pin the version indeed.

[jln (very slow on Chromium), 2015-04-23 18:08:20]

On 2015/04/23 17:59:59, jln wrote:
> Moreover, the seccomp system call is well
> designed insofar as it rejects unknown flags (which is a much more future way
to
> do things). So in a way this is a pretty reasonable way to detect seccomp
> squatting in a fairly generic way.

s/future/future-proof/

[palmer, 2015-04-23 18:20:23]

LGTM with absurdly tiny nit :)

https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall,
sys_set_media_ext with the same number as
Nit: "...syscall, sys_set_media_ext, with..."

[jln (very slow on Chromium), 2015-04-23 18:23:01]

https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall,
sys_set_media_ext with the same number as
On 2015/04/23 18:20:23, palmer wrote:
> Nit: "...syscall, sys_set_media_ext, with..."

Done.

[jln (very slow on Chromium), 2015-04-23 21:55:43]

Robert: Matthew seems fine with this. Let's go?

[Robert Sesek, 2015-04-23 22:01:02]

LGTM

[jln (very slow on Chromium), 2015-04-23 22:02:32]

The CQ bit was checked by jln@chromium.org

[jln (very slow on Chromium), 2015-04-23 22:02:33]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1095133003/#ps40001
(title: "Add comma.")

[commit-bot: I haz the power, 2015-04-23 22:03:48]

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

[commit-bot: I haz the power, 2015-04-23 22:07:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-23 22:09:27]

Patchset 3 (id:??) landed as
https://crrev.com/c08969c1b8b723494569c893b15c9b2e634ef0fc
Cr-Commit-Position: refs/heads/master@{#326667}