Linux sandbox: Allow restricting sched_* on other processes.

Linux sandbox: Allow restricting sched_* on other processes.

Adds a RestrictSchedTarget parameter restriction which only allows
sched_* syscalls if the pid argument is the sandboxed process's pid or
if the pid is 0, which means the current thread.  glibc's pthread
implementation sometimes calls these syscalls with pid equal to the
current tid.  On these calls, the policy triggers a SIGSYS, and the
SIGSYS handler reruns the syscall with a pid argument of 0.

R=jln@chromium.org
BUG=413855
Committed: https://crrev.com/282ba301cf990ce291c45a05b5226df6804ae271
Cr-Commit-Position: refs/heads/master@{#297059}

[jln (very slow on Chromium), 2014-09-22 21:56:45]

Thanks for working on this Ricky!

- I think there is a bug that will result in us not having the correct errno for
forwarded sched_ syscalls that fail.

- nedeljko.babic@imgtec.com / Kees: I can't remember the status of seccomp-bpf
on MIPS for 8 parameters system call.
Do the last 2 parameter (6 and 7) get folded into the upper 32 bits of
parameters 4 and 5 for the SIGSYS handler? Or do they completely disappear
(making system call non-forwardable on MIPS).

- Currently you're not modifying any existing policy to make use of this new
functionality. I suppose that it is on purpose? (and will be handled in another
CL)

https://codereview.chromium.org/590213003/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h (right):

https://codereview.chromium.org/590213003/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h:40: pid_t
GetCurrentPid() const;
I don't think you need this here. We can add it when it starts becoming required
perhaps?

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File sandbox/linux/seccomp-bpf-helpers/baseline_policy.h (right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/baseline_policy.h:36: pid_t GetCurrentPid()
const;
As per style-guide, this can just be current_pid() instead.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:212: pid_t tid =
syscall(__NR_gettid);
const

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:213: if (args.args[0] ==
(uint64_t) tid) {
style: C++ static_cast

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:213: if (args.args[0] ==
(uint64_t) tid) {
A nit (feel free to ignore), but I find it a tad awkward to check the first
argument before checking for the system call.

Unless we have a bug in our policies we know that the syscall will be one of the
syscalls below, but it feels more natural to me to: 1. check if the syscall is
sched_something, then inspect its first argument. (And in case either fail,
crash).

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:225: return
syscall(args.nr,
glibc's syscall sets errno (which will be discarded by our trap handler anyways)
and returns -1.

The convention for trap handlers is to do like native system calls on Intel and
return -errno.

You could use syscall.h to perform the syscall instead of glibc's.

We also have ForwardSyscall, explicitly for that.

However, I can't remember what the deal is with ForwardSyscall on MIPS (and
perhaps ARM):

nedeljko: is it correct that on MIPS, if an 8 parameters system call traps on
SIGSYS, userland will only see the 6 first parameters and therefore a real
ForwardSyscall is impossible? Or was there some magic where argument 6 and 7
would end-up in the 32 upper bits of argument 4 and 5? We should absolutely
document this behavior better in the ForwardSyscall interface as well as in
trap.h.

Ricky: Given the system calls that you're handling, this may be ok, but you
should quickly check that none of them would trigger parameter 6 and 7 on MIPS.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:236: NOTREACHED();
Don't use NOTREACHED() here, as it's not async signal safe. You can do a
RAW_CHECK() instead (This will exist in release builds as well, which IMO is
fine.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:237: return -1;
return -ENOSYS maybe?

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc
(right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:175:
BPF_ASSERT(CPU_EQUAL(&zero_mask, &tid_mask));
Could you add some testing of errno if you make some wrong call to
sched_getaffinity?

I would like a test that catches your current bug (of using Glibc's syscall())

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:187:
getaffinity_thread.Stop();
You need synchronization here to make sure that your thread has executed the
task posted.

You can take a look at the SendRecvMsgAbortOnReplyFDClose test in base/ for an
example of how to do that.

[jln (very slow on Chromium), 2014-09-23 01:11:38]

> - mailto:nedeljko.babic@imgtec.com / Kees: I can't remember the status of
seccomp-bpf
> on MIPS for 8 parameters system call.
> Do the last 2 parameter (6 and 7) get folded into the upper 32 bits of
> parameters 4 and 5 for the SIGSYS handler? Or do they completely disappear
> (making system call non-forwardable on MIPS).

I now remember the need for a 7th parameter: indirect syscalls (i.e. the syscall
number is the first argument) on MIPS.

However, this is all hidden from seccomp-bpf
(http://www.linux-mips.org/archives/linux-mips/2014-01/msg00231.html), so
ForwardSyscall would work fine as-is.

*but* in the end of the day we had to add support for an 8th argument. And that
I have no idea why anymore and ForwardSyscall() may be broken.

There may be information buried in
https://chromiumcodereview.appspot.com/260793003/. In there we requested a
specific test for forwarding pread64(). But it looks like it's not tested as an
indirect system call.

[rickyz (no longer on Chrome), 2014-09-23 06:03:21]

Thanks for the info on MIPS syscalls.  None of the ones this touches takes
64-bit arguments or has >3 arguments, so we shouldn't have any issues with
forwarding.

https://codereview.chromium.org/590213003/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h (right):

https://codereview.chromium.org/590213003/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h:40: pid_t
GetCurrentPid() const;
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> I don't think you need this here. We can add it when it starts becoming
required
> perhaps?

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File sandbox/linux/seccomp-bpf-helpers/baseline_policy.h (right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/baseline_policy.h:36: pid_t GetCurrentPid()
const;
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> As per style-guide, this can just be current_pid() instead.

Good to know, removed this for now and will reintroduce in a future CL.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:212: pid_t tid =
syscall(__NR_gettid);
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> const

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:213: if (args.args[0] ==
(uint64_t) tid) {
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> A nit (feel free to ignore), but I find it a tad awkward to check the first
> argument before checking for the system call.
> 
> Unless we have a bug in our policies we know that the syscall will be one of
the
> syscalls below, but it feels more natural to me to: 1. check if the syscall is
> sched_something, then inspect its first argument. (And in case either fail,
> crash).

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:225: return
syscall(args.nr,
Ah, good catch - fixed this and added a test that catches it.  Ended up using
Syscall directly to avoid making a copy of arch_seccomp_data.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:236: NOTREACHED();
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> Don't use NOTREACHED() here, as it's not async signal safe. You can do a
> RAW_CHECK() instead (This will exist in release builds as well, which IMO is
> fine.

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:237: return -1;
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> return -ENOSYS maybe?

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
File
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc
(right):

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:175:
BPF_ASSERT(CPU_EQUAL(&zero_mask, &tid_mask));
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> Could you add some testing of errno if you make some wrong call to
> sched_getaffinity?
> 
> I would like a test that catches your current bug (of using Glibc's syscall())

Done.

https://codereview.chromium.org/590213003/diff/1/sandbox/linux/seccomp-bpf-he...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:187:
getaffinity_thread.Stop();
On 2014/09/22 21:56:44, jln (slow on IPC reviews) wrote:
> You need synchronization here to make sure that your thread has executed the
> task posted.
> 
> You can take a look at the SendRecvMsgAbortOnReplyFDClose test in base/ for an
> example of how to do that.

Done.

[nedeljko, 2014-09-25 10:57:13]

On 2014/09/23 01:11:38, jln (slow on IPC reviews) wrote:
> > - mailto:nedeljko.babic@imgtec.com / Kees: I can't remember the status of
> seccomp-bpf
> > on MIPS for 8 parameters system call.
> > Do the last 2 parameter (6 and 7) get folded into the upper 32 bits of
> > parameters 4 and 5 for the SIGSYS handler? Or do they completely disappear
> > (making system call non-forwardable on MIPS).
> 
> I now remember the need for a 7th parameter: indirect syscalls (i.e. the
syscall
> number is the first argument) on MIPS.
> 
> However, this is all hidden from seccomp-bpf
> (http://www.linux-mips.org/archives/linux-mips/2014-01/msg00231.html), so
> ForwardSyscall would work fine as-is.
> 
> *but* in the end of the day we had to add support for an 8th argument. And
that
> I have no idea why anymore and ForwardSyscall() may be broken.
> 
> There may be information buried in
> https://chromiumcodereview.appspot.com/260793003/. In there we requested a
> specific test for forwarding pread64(). But it looks like it's not tested as
an
> indirect system call.

There is support for syscall with up to 8 arguments in Linux MIPS and that was
the 
reason for adding support for 8th argument (although I am not sure that there is
a
syscall with 8 arguments...)

The support for 8 native-size parameters was added in Syscall::Call() in
https://codereview.chromium.org/357323003.

However, I think that this is useful only when UnsafeTrap() is used to proxy
system calls.
As far as I know, seccomp bpf has a limit on number of arguments that can be
filtered
and that limit is 6 (and this is not architecture dependent).

I saw the comment that no more than 3 arguments are used here, but I am replying
anyway
because I think that this can be useful to someone ...

[jln (very slow on Chromium), 2014-09-25 15:32:05]

Thank you Nedeljko!

On Thu, Sep 25, 2014 at 3:57 AM, <nedeljko.babic@imgtec.com> wrote:

> On 2014/09/23 01:11:38, jln (slow on IPC reviews) wrote:
>
>> > - mailto:nedeljko.babic@imgtec.com / Kees: I can't remember the status
>> of
>> seccomp-bpf
>> > on MIPS for 8 parameters system call.
>> > Do the last 2 parameter (6 and 7) get folded into the upper 32 bits of
>> > parameters 4 and 5 for the SIGSYS handler? Or do they completely
>> disappear
>> > (making system call non-forwardable on MIPS).
>>
>
>  I now remember the need for a 7th parameter: indirect syscalls (i.e. the
>>
> syscall
>
>> number is the first argument) on MIPS.
>>
>
>  However, this is all hidden from seccomp-bpf
>> (http://www.linux-mips.org/archives/linux-mips/2014-01/msg00231.html), so
>> ForwardSyscall would work fine as-is.
>>
>
>  *but* in the end of the day we had to add support for an 8th argument. And
>>
> that
>
>> I have no idea why anymore and ForwardSyscall() may be broken.
>>
>
>  There may be information buried in
>> https://chromiumcodereview.appspot.com/260793003/. In there we requested
>> a
>> specific test for forwarding pread64(). But it looks like it's not tested
>> as
>>
> an
>
>> indirect system call.
>>
>
> There is support for syscall with up to 8 arguments in Linux MIPS and that
> was
> the
> reason for adding support for 8th argument (although I am not sure that
> there is
> a
> syscall with 8 arguments...)
>
> The support for 8 native-size parameters was added in Syscall::Call() in
> https://codereview.chromium.org/357323003.
>
> However, I think that this is useful only when UnsafeTrap() is used to
> proxy
> system calls.
> As far as I know, seccomp bpf has a limit on number of arguments that can
> be
> filtered
> and that limit is 6 (and this is not architecture dependent).
>
> I saw the comment that no more than 3 arguments are used here, but I am
> replying
> anyway
> because I think that this can be useful to someone ...
>
> https://codereview.chromium.org/590213003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rickyz (no longer on Chrome), 2014-09-26 18:59:23]

Friendly ping - once this is checked in, I think we can start restricting these
calls on the gpu and ppapi policies without breaking browser_tests or
content_browser_tests.  It should also be possible to restrict these on the
renderer once https://webrtc-codereview.appspot.com/28539004/ is checked in.

[jln (very slow on Chromium), 2014-09-26 20:21:51]

Excellent, lgtm!

Sorry for the long delay.

https://chromiumcodereview.appspot.com/590213003/diff/130001/sandbox/linux/se...
File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h (right):

https://chromiumcodereview.appspot.com/590213003/diff/130001/sandbox/linux/se...
sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h:53: // current thread).
Could you list the syscalls supported?

[rickyz (Google), 2014-09-26 20:44:26]

rickyz@google.com changed reviewers:
+ rickyz@google.com

[rickyz (Google), 2014-09-26 20:44:26]

Thanks!  Will check CQ once try bots pass

[rickyz (no longer on Chrome), 2014-09-26 21:40:38]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2014-09-26 21:42:00]

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

[commit-bot: I haz the power, 2014-09-26 22:33:18]

Committed patchset #9 (id:150001) as 853ff832d8c3412b0c952417500b34b07cb44ded

[commit-bot: I haz the power, 2014-09-26 22:34:01]

Patchset 9 (id:??) landed as
https://crrev.com/282ba301cf990ce291c45a05b5226df6804ae271
Cr-Commit-Position: refs/heads/master@{#297059}