Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1295513003:
Non-SFI mode: Sandbox support for NaCl async-signals. (Closed)
|
Created:
5 years, 4 months ago by Luis Héctor Chávez Modified:
5 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNon-SFI mode: Sandbox support for NaCl async-signals.
In preparation for the async-signal support change in
https://codereview.chromium.org/1212613002, this CL allows two new
syscalls:
* clone() with CLONE_PARENT_SETTID.
* tgkill() with the current pid as tgid and SIGUSR1 as signal number.
TEST=nacl_loader_unittests
TEST=Ran bots
TEST=ARC async-signals work
BUG=nativeclient:4065
R=mseaborn@chromium.org
Committed: https://crrev.com/1a9a9ce3b6bec20af9c0bcc9427f654d53697abe
Cr-Commit-Position: refs/heads/master@{#343807}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Added a TODO to remove a flag after NaCl roll #Patch Set 3 : Added positive restriction for tid + unit tests #
Total comments: 1
Patch Set 4 : Fixed unit tests #Patch Set 5 : Used the correct constant for SIGUSR1 #
Total comments: 6
Patch Set 6 : Addressed feedback #
Total comments: 2
Patch Set 7 : Added a const #
Messages
Total messages: 22 (6 generated)
mseaborn@chromium.org changed reviewers: + jln@chromium.org
LGTM. I'd like Julien to sign off on this too. https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:87: return If(flags == clone_flags || How about adding a TODO to remove the variant without CLONE_PARENT_SETTID after the NaCl-side change is rolled in? https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:155: return If(tgid == getpid() && signum == LINUX_SIGUSR1, Should we check that the thread ID is positive too, just to be on the safe side? The Linux man page for tgkill() doesn't document any special behaviour for 0 or negative values, but the Linux man pages are notoriously incomplete.
https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:87: return If(flags == clone_flags || On 2015/08/13 23:38:10, Mark Seaborn wrote: > How about adding a TODO to remove the variant without CLONE_PARENT_SETTID after > the NaCl-side change is rolled in? Done. https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:155: return If(tgid == getpid() && signum == LINUX_SIGUSR1, On 2015/08/13 23:38:10, Mark Seaborn wrote: > Should we check that the thread ID is positive too, just to be on the safe side? > > The Linux man page for tgkill() doesn't document any special behaviour for 0 or > negative values, but the Linux man pages are notoriously incomplete. Arg<int> does not support greater-than comparison :( (although a mask operation with an Arg<uint32_t> could work in a pinch). Looks like Linux does the lookup for the thread ID using equality (kernel/pid.c, find_pid_ns()) and does not seem to give any special treatment to negative values. Furthermore, negative thread IDs are invalid since alloc_pidmap() returns -1 on failure. We should be safe with the code as-is.
https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:155: return If(tgid == getpid() && signum == LINUX_SIGUSR1, On 2015/08/14 00:29:32, Luis Héctor Chávez wrote: > On 2015/08/13 23:38:10, Mark Seaborn wrote: > > Should we check that the thread ID is positive too, just to be on the safe > side? > > > > The Linux man page for tgkill() doesn't document any special behaviour for 0 > or > > negative values, but the Linux man pages are notoriously incomplete. > > Arg<int> does not support greater-than comparison :( (although a mask operation > with an Arg<uint32_t> could work in a pinch). I suppose you can check that: * arg != 0 * (arg & (1 << 31)) == 0 // arg is non-negative And comment about the lack of a greater-than op. Actually, can you add a test for this in components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc, please? I forgot about that before. > Looks like Linux does the lookup for the thread ID using equality (kernel/pid.c, > find_pid_ns()) and does not seem to give any special treatment to negative > values. Furthermore, negative thread IDs are invalid since alloc_pidmap() > returns -1 on failure. We should be safe with the code as-is. That may be true for current versions, but we're slightly paranoid that future Linux versions might introduce weird corner cases. :-)
On 2015/08/14 20:12:41, Mark Seaborn wrote: > https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... > File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): > > https://codereview.chromium.org/1295513003/diff/1/components/nacl/loader/nons... > components/nacl/loader/nonsfi/nonsfi_sandbox.cc:155: return If(tgid == getpid() > && signum == LINUX_SIGUSR1, > On 2015/08/14 00:29:32, Luis Héctor Chávez wrote: > > On 2015/08/13 23:38:10, Mark Seaborn wrote: > > > Should we check that the thread ID is positive too, just to be on the safe > > side? > > > > > > The Linux man page for tgkill() doesn't document any special behaviour for 0 > > or > > > negative values, but the Linux man pages are notoriously incomplete. > > > > Arg<int> does not support greater-than comparison :( (although a mask > operation > > with an Arg<uint32_t> could work in a pinch). > > I suppose you can check that: > > * arg != 0 > * (arg & (1 << 31)) == 0 // arg is non-negative > > And comment about the lack of a greater-than op. > > Actually, can you add a test for this in > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc, please? I forgot > about that before. Done > > > > Looks like Linux does the lookup for the thread ID using equality > (kernel/pid.c, > > find_pid_ns()) and does not seem to give any special treatment to negative > > values. Furthermore, negative thread IDs are invalid since alloc_pidmap() > > returns -1 on failure. We should be safe with the code as-is. > > That may be true for current versions, but we're slightly paranoid that future > Linux versions might introduce weird corner cases. :-) Fair enough, done.
https://codereview.chromium.org/1295513003/diff/40001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1295513003/diff/40001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:648: int tid = syscall(__NR_gettid); Won't this test get killed at this point, since gettid is not allowed? You could test with tid=1 instead, like the other test cases.
On 2015/08/14 21:25:20, Mark Seaborn wrote: > https://codereview.chromium.org/1295513003/diff/40001/components/nacl/loader/... > File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): > > https://codereview.chromium.org/1295513003/diff/40001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:648: int tid = > syscall(__NR_gettid); > Won't this test get killed at this point, since gettid is not allowed? > > You could test with tid=1 instead, like the other test cases. gettid seems to be allowed, but getpid (signal number 20) is not: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... Removing both the calls to getpid and gettid (for consistency).
jln@chromium.org changed reviewers: + mdempsky@chromium.org
+cc mdempsky FYI https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:157: return If(tgid == getpid() && Don't use getpid() in policies. This should be a property of the policy itself, recorded when the object is created. You can look at sandbox/linux/seccomp-bpf-helpers/baseline_policy.h:policy_pid_ There are several reason for this, but in general we don't want policies to depend on "environment" (we should be able to construct them out of process for instance). https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:160: tid != 0 && FYI, this check isn't strictly necessary, Linux doesn't allow negative tids anyways. But I like the paranoia, fine to keep. https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:691: BPF_TGKILL_DEATH_TEST(tgkill_with_invalid_signum) { The complexity of BPF test is already considerable, and this is pushing it even farther. You should be able to do this with just a normal BPF_DEATH_TEST_D / BPF_TEST_D taking a templated class at the |bpf_tester_delegate_class| argument, no? Slightly more verbosity for improved readability and simplicity would be the right choice here.
https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:157: return If(tgid == getpid() && On 2015/08/17 21:21:11, jln (slow on Chromium) wrote: > Don't use getpid() in policies. This should be a property of the policy itself, > recorded when the object is created. > > You can look at sandbox/linux/seccomp-bpf-helpers/baseline_policy.h:policy_pid_ > > There are several reason for this, but in general we don't want policies to > depend on "environment" (we should be able to construct them out of process for > instance). Done. https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:160: tid != 0 && On 2015/08/17 21:21:11, jln (slow on Chromium) wrote: > FYI, this check isn't strictly necessary, Linux doesn't allow negative tids > anyways. But I like the paranoia, fine to keep. Acknowledged. https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/1295513003/diff/80001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:691: BPF_TGKILL_DEATH_TEST(tgkill_with_invalid_signum) { On 2015/08/17 21:21:11, jln (slow on Chromium) wrote: > The complexity of BPF test is already considerable, and this is pushing it even > farther. > > You should be able to do this with just a normal BPF_DEATH_TEST_D / BPF_TEST_D > taking a templated class at the |bpf_tester_delegate_class| argument, no? > > Slightly more verbosity for improved readability and simplicity would be the > right choice here. Done.
lgtm with nit https://chromiumcodereview.appspot.com/1295513003/diff/100001/components/nacl... File components/nacl/loader/nonsfi/nonsfi_sandbox.h (right): https://chromiumcodereview.appspot.com/1295513003/diff/100001/components/nacl... components/nacl/loader/nonsfi/nonsfi_sandbox.h:29: pid_t policy_pid_; const
https://chromiumcodereview.appspot.com/1295513003/diff/100001/components/nacl... File components/nacl/loader/nonsfi/nonsfi_sandbox.h (right): https://chromiumcodereview.appspot.com/1295513003/diff/100001/components/nacl... components/nacl/loader/nonsfi/nonsfi_sandbox.h:29: pid_t policy_pid_; On 2015/08/17 23:08:35, jln (slow on Chromium) wrote: > const Done.
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org, jln@chromium.org Link to the patchset: https://codereview.chromium.org/1295513003/#ps120001 (title: "Added a const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295513003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295513003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lhchavez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295513003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295513003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1a9a9ce3b6bec20af9c0bcc9427f654d53697abe Cr-Commit-Position: refs/heads/master@{#343807} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
