Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196793023:
Add seccomp sandbox for non-SFI NaCl (Closed)
|
Created:
6 years, 9 months ago by hamaji Modified:
6 years, 8 months ago CC:
chromium-reviews, hidehiko, kmixter1, dvyukov, Alexander Potapenko Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd seccomp sandbox for non-SFI NaCl
All syscalls except whitelisted ones will cause SIGSYS.
We test the sandbox with BPF_TEST and BPF_TEST_DEATH, which appropriately fork the process so the main process of the test will never enable the sandbox.
TEST=Our app works with this sandbox on i686 and ARM
TEST=Build chrome and nacl_helper on i686, x86-64, and ARM
TEST=./out/Release/components_unittests --gtest_filter='NaClNonSfi*'
# on i686, x86-64, and ARM
TEST=SFI NaCl apps still work
TEST=trybots
BUG=359285
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264383
Patch Set 1 #
Total comments: 25
Patch Set 2 : #Patch Set 3 : #
Total comments: 17
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 9
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 8
Patch Set 14 : #Patch Set 15 : #
Total comments: 23
Patch Set 16 : #Patch Set 17 : #
Total comments: 16
Patch Set 18 : allow shutdown and pwrite64 #Patch Set 19 : #
Total comments: 2
Patch Set 20 : #Patch Set 21 : remove one more unittest for shutdown #
Total comments: 57
Patch Set 22 : #Patch Set 23 : #
Total comments: 12
Patch Set 24 : #Messages
Total messages: 38 (0 generated)
Julien and Will: PTAL components/nacl/loader/nacl_sandbox_linux.cc Mark: PTAL others In this change, I'd like to focus on ARM seccomp sandbox. Though I confirmed that this patch works for our app on non-SFI i686 and we can compile this on x86-64, this is only for internal development. We will not enable non-SFI mode for i686 and x86-64 anytime soon. Note that this mode is currently guarded by --enable-nacl-nonsfi-mode flag. We want to enable this mode only for ARM in M35. We want to suspend newlib switch for now, to make non-SFI mode available in M35. We promise we prioritize newlib switch in M36 so that we should not care about maintenance burden too much. Luckily, it seems brk is the only syscall we need to allow for glibc. I think we can remove brk by newlib switch. We can also change the response for get_robust_list from EPERM to SIGSYS by newlib switch. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:353: bool sandbox_is_initialized = content::InitializeSandbox( I thought we should add something like #if !defined(__arm__) return false; #endif because i686 and x86-64 support could be incomplete. However, Hidehiko says it will break our unit tests and having no guard for this would be OK, as non-SFI mode is guarded by --enable-nacl-nonsfi-mode flag anyway. https://codereview.chromium.org/196793023/diff/1/components/nacl/zygote/nacl_... File components/nacl/zygote/nacl_fork_delegate_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/zygote/nacl_... components/nacl/zygote/nacl_fork_delegate_linux.cc:266: uses_nonsfi_ = process_type == switches::kNaClNonSfiLoaderProcess; This would be probably the most doubtful change in this patch except for seccomp changes. I think Zygote process sequentially calls Fork after CanHelp so this should be fine. Another solution would be add process_type argument to NaClForkDelegate::Fork. I didn't do this as we will probably add one more NaClForkDelegate instance to Zygote class and NaClForkDelegate::Fork won't need process_type.
Julien and Will: PTAL components/nacl/loader/nacl_sandbox_linux.cc Mark: PTAL others In this change, I'd like to focus on ARM seccomp sandbox. Though I confirmed that this patch works for our app on non-SFI i686 and we can compile this on x86-64, this is only for internal development. We will not enable non-SFI mode for i686 and x86-64 anytime soon. Note that this mode is currently guarded by --enable-nacl-nonsfi-mode flag. We want to enable this mode only for ARM in M35. We want to suspend newlib switch for now, to make non-SFI mode available in M35. We promise we prioritize newlib switch in M36 so that we should not care about maintenance burden too much. Luckily, it seems brk is the only syscall we need to allow for glibc. I think we can remove brk by newlib switch. We can also change the response for get_robust_list from EPERM to SIGSYS by newlib switch. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:353: bool sandbox_is_initialized = content::InitializeSandbox( I thought we should add something like #if !defined(__arm__) return false; #endif because i686 and x86-64 support could be incomplete. However, Hidehiko says it will break our unit tests and having no guard for this would be OK, as non-SFI mode is guarded by --enable-nacl-nonsfi-mode flag anyway. https://codereview.chromium.org/196793023/diff/1/components/nacl/zygote/nacl_... File components/nacl/zygote/nacl_fork_delegate_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/zygote/nacl_... components/nacl/zygote/nacl_fork_delegate_linux.cc:266: uses_nonsfi_ = process_type == switches::kNaClNonSfiLoaderProcess; This would be probably the most doubtful change in this patch except for seccomp changes. I think Zygote process sequentially calls Fork after CanHelp so this should be fine. Another solution would be add process_type argument to NaClForkDelegate::Fork. I didn't do this as we will probably add one more NaClForkDelegate instance to Zygote class and NaClForkDelegate::Fork won't need process_type.
I'm not an owner of these seccomp-bpf filters, so Julien might disagree with me about how to factor this. Adding more seccomp sandboxing for Bare Metal Mode might be premature at this point. We should probably have more tests running on the bots first, and have a design discussion about how the syscall filtering should be done. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:172: ErrorCode ret = baseline_policy_->EvaluateSyscall(sb, sysno); You're falling through to the baseline policy. I don't think that's good for Bare Metal Mode because the baseline policy is quite complex. I think we'll want to define a minimal standalone policy. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:193: // We only allow PR_SET_NAME, PR_SET_DUMPABLE, and PR_GET_DUMPABLE. I don't think these are operations that should be needed or allowed. Also this comment duplicates what RestrictPrctl() checks, which could get out of date. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:196: #if defined(__i686__) __i386__ would be preferred. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:234: #if !defined(__arm__) Should be "defined(__i386__) || defined(__x86_64__)" for x86 things such as modify_ldt. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:239: // nacl_helper calls this from service_runtime/linux/x86/nacl_ldt.c But this doesn't apply to Non-SFI Mode. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:246: // glibc internally calls brk in its memory allocation. glibc does call brk(), but it doesn't require it to work. It's OK for brk() to fail: glibc will fall back to using mmap(). https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:249: case __NR_capget: This is a blacklist of disallowed syscalls, but I don't think having a blacklist is a good idea. Disallowing via SIGSYS should really be the default. Maybe disallowed syscalls should just be listed in a test that checks that kill() etc. are disabled by the BPF filter. It looks like this blacklist aims to override syscalls that are allowed by the baseline policy defined by sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc.
We'll also need some testing. This is critical enough that we want a test that checks that any syscall not in the whitelist (the whitelist should be duplicated in the test for redundancy) is EPERM-ed or crashes the process. We can't afford a BPF compiler bug or a typo (forgetting "break" in a switch statement) to affect us. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_helper_linux.cc:53: // don't need zygote FD any more We need to refactor a bunch of this logic, but in the meantime, let's make absolutely sure that we're always under the setuid sandbox in nonsfi mode. const bool setuid_sandbox_enabled = IsSandboxed(); if (uses_nonsfi) CHECK(setuid_sandbox_enabled); https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_helper_linux.cc:56: bool sandbox_initialized = false; rename to bpf_sandbox_initialized. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:172: ErrorCode ret = baseline_policy_->EvaluateSyscall(sb, sysno); On 2014/03/18 23:53:58, Mark Seaborn wrote: > You're falling through to the baseline policy. I don't think that's good for > Bare Metal Mode because the baseline policy is quite complex. > > I think we'll want to define a minimal standalone policy. I absolutely agree with Mark here. We need a short whitelist of what's allowed. The rest must be explicitly denied here. This complex combination of a blacklist on top of an external policy is not safe. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:246: // glibc internally calls brk in its memory allocation. On 2014/03/18 23:53:58, Mark Seaborn wrote: > glibc does call brk(), but it doesn't require it to work. It's OK for brk() to > fail: glibc will fall back to using mmap(). Yes, in other words, just EPERM it. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:353: bool sandbox_is_initialized = content::InitializeSandbox( On 2014/03/14 12:46:23, hamaji wrote: > I thought we should add something like > > #if !defined(__arm__) > return false; > #endif > > because i686 and x86-64 support could be incomplete. However, Hidehiko says it > will break our unit tests and having no guard for this would be OK, as non-SFI > mode is guarded by --enable-nacl-nonsfi-mode flag anyway. We should not return false in non SFI mode. We should LOG(FATAL) if this fails.
As Mark has suggested, it would be better to add tests after Hidehiko finished his work for non-SFI NaCl testing. Please feel free to suspend the review of this change if you want. But I'll be really appreciated your early feedbacks for this change. I'd like to know possible road blocks as fast as possible. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_helper_linux.cc:53: // don't need zygote FD any more On 2014/03/20 00:54:53, jln wrote: > We need to refactor a bunch of this logic, but in the meantime, let's make > absolutely sure that we're always under the setuid sandbox in nonsfi mode. > > const bool setuid_sandbox_enabled = IsSandboxed(); > if (uses_nonsfi) CHECK(setuid_sandbox_enabled); Done. However, this essentially disables --no-sandbox option. We want to use BMM mode + no sandbox mode for our internal development and deprecate our trusted plugin mode. So, we'd like to have a way to disable sandbox even on non-SFI NaCl. Does this make sense to add an environment variable like NACL_DANGEROUS_NO_SANDBOX_ON_NONSFI_MODE and disable sandbox only when both --no-sandbox and NACL_DANGEROUS_NO_SANDBOX_ON_NONSFI_MODE are specified, for example? https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_helper_linux.cc:56: bool sandbox_initialized = false; On 2014/03/20 00:54:53, jln wrote: > rename to bpf_sandbox_initialized. Done. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:172: ErrorCode ret = baseline_policy_->EvaluateSyscall(sb, sysno); On 2014/03/20 00:54:53, jln wrote: > On 2014/03/18 23:53:58, Mark Seaborn wrote: > > You're falling through to the baseline policy. I don't think that's good for > > Bare Metal Mode because the baseline policy is quite complex. > > > > I think we'll want to define a minimal standalone policy. > > I absolutely agree with Mark here. We need a short whitelist of what's allowed. > The rest must be explicitly denied here. > > This complex combination of a blacklist on top of an external policy is not > safe. Done. I also stopped using sandbox::Restrict* as they might be changed without thinking about BMM. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:193: // We only allow PR_SET_NAME, PR_SET_DUMPABLE, and PR_GET_DUMPABLE. On 2014/03/18 23:53:58, Mark Seaborn wrote: > I don't think these are operations that should be needed or allowed. Also this > comment duplicates what RestrictPrctl() checks, which could get out of date. I stopped using RestrictPrctl. Now it returns EPERM for PR_SET_NAME and otherwise it crashes. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:196: #if defined(__i686__) On 2014/03/18 23:53:58, Mark Seaborn wrote: > __i386__ would be preferred. Done. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:234: #if !defined(__arm__) On 2014/03/18 23:53:58, Mark Seaborn wrote: > Should be "defined(__i386__) || defined(__x86_64__)" for x86 things such as > modify_ldt. Done. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:239: // nacl_helper calls this from service_runtime/linux/x86/nacl_ldt.c On 2014/03/18 23:53:58, Mark Seaborn wrote: > But this doesn't apply to Non-SFI Mode. It seems current nacl_helper always calls NaClTlsInit. I created another CL so that we can avoid calling NaClTlsInit in non-SFI mode. https://codereview.chromium.org/209423004/ https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:246: // glibc internally calls brk in its memory allocation. On 2014/03/20 00:54:53, jln wrote: > On 2014/03/18 23:53:58, Mark Seaborn wrote: > > glibc does call brk(), but it doesn't require it to work. It's OK for brk() > to > > fail: glibc will fall back to using mmap(). > > Yes, in other words, just EPERM it. Oops, my comment was wrong. brk was called by tcmalloc. We can get rid of brk call by removing #define HAVE_SBRK 1 from third_party/tcmalloc/chromium/src/config_linux.h. I think there are some options: 1. Remove HAVE_SBRK from config_linux.h. I feel this is the right choice, but I'd like to avoid this, because this might affect a lot of things (memory profiling tools, etc.). 2. Remove HAVE_SBRK only from nacl_helper. Currently, nacl_helper uses the same liballocator.a as chrome's, so some gyp tweaks would be needed. 3. Stop using tcmalloc from nacl_helper. I'm not sure if nacl_helper really wants tcmalloc. 4. Wait until newlib switch. Once we have switched to newlib and split nacl_helper into SFI and non-SFI helpers, we don't need to link tcmalloc in non-SFI nacl_helper. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:249: case __NR_capget: On 2014/03/18 23:53:58, Mark Seaborn wrote: > This is a blacklist of disallowed syscalls, but I don't think having a blacklist > is a good idea. Disallowing via SIGSYS should really be the default. > > Maybe disallowed syscalls should just be listed in a test that checks that > kill() etc. are disabled by the BPF filter. > > It looks like this blacklist aims to override syscalls that are allowed by the > baseline policy defined by sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc. Done. https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:353: bool sandbox_is_initialized = content::InitializeSandbox( On 2014/03/20 00:54:53, jln wrote: > On 2014/03/14 12:46:23, hamaji wrote: > > I thought we should add something like > > > > #if !defined(__arm__) > > return false; > > #endif > > > > because i686 and x86-64 support could be incomplete. However, Hidehiko says it > > will break our unit tests and having no guard for this would be OK, as non-SFI > > mode is guarded by --enable-nacl-nonsfi-mode flag anyway. > > We should not return false in non SFI mode. We should LOG(FATAL) if this fails. Done. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:215: // Allowed syscalls. I have a question about sigaction and sigaltstack. I guess breakpad uses them. I didn't check when they can be initialized. If we cannot initialize breakpad's signal handlers before the initialization of the sandbox, we might need to allow sigaction/sigaltstack. I don't know if it's are OK with allowing them. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:222: case __NR_epoll_ctl: Would it be better to check the operation type is ADD or MOD or DEL?
A few more questions/comments. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:227: case __NR_futex: Maybe better to restrict the operation of futex? https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:248: case __NR_clock_getres: Do we need to restrict clk_id for gettime and getres? https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_main.cc (left): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_main.cc:181: NaClSrpcModuleInit(); This was called from here for historical reason, I think. Hidehiko introduced this file first, but now this module is initialized in NaClChromeMainInit, so we don't need this anymore.
Julien and I went through this change together today. Can you split out some parts of this for committing separately (see below), so that reviewing the BPF policy part will be simpler? In particular, the "uses_nonsfi_mode" plumbing can be split out. https://codereview.chromium.org/196793023/diff/40001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:585: uses_nonsfi_mode_ ? Can you split out the parts that plumb through "uses_nonsfi_mode" to nacl_helper into a separate change? There's already a place where we're passing "uses_nonsfi_mode" to the nacl_helper process. nacl_listener.cc receives NaClProcessMsg_Start, which contains this flag, but this isn't early enough for the seccomp-bpf sandboxing. Rather than having two places where this is plumbed through, can you change nacl_listener.cc to receive the flag via the new route you're adding? (Otherwise I'd worry that the two could get out of sync.) https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:93: NaClListener listener; This could become "listener(uses_nonsfi_mode)" (see other comment). https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:272: NaClChromeMainInitForNonSfi(); AFAICT, this function isn't defined, so this change doesn't compile? I don't see any trybot results for this change. However, it would be nice to clean up this function. It would make sense to refactor OnStart() so that it checks params.uses_nonsfi_mode right at the start and only does the minimum required when running in Non-SFI Mode. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:198: class NonSfiNaClBPFSandboxPolicy : public SandboxBPFPolicy { Can we put the Non-SFI Mode sandbox policy in a separate file, for clarity, since it's so much more security critical than the SFI Mode sandbox policy? (Julien and I talked about this today.) The new file can go under components/nacl/loader/nonsfi. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:273: ssize_t read_ret = (*NACL_VTBL(NaClDesc, descriptor)->Read)( Julien and I are both OK with allowing the pread() syscall, so you can leave this as it was and change the BPF policy. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_main.cc (left): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_main.cc:181: NaClSrpcModuleInit(); On 2014/03/24 16:25:43, hamaji wrote: > This was called from here for historical reason, I think. Hidehiko introduced > this file first, but now this module is initialized in NaClChromeMainInit, so we > don't need this anymore. Calling NaClChromeMainInit() for the non-SFI code path is somewhat accidental, though. Was there a problem with calling NaClSrpcModuleInit() twice? There's nothing wrong with removing this call, but AFAICT it's not related to tightening up the sandbox. Could you send this for review as a separate change, please?
https://codereview.chromium.org/196793023/diff/40001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:585: uses_nonsfi_mode_ ? On 2014/03/28 01:38:25, Mark Seaborn wrote: > Can you split out the parts that plumb through "uses_nonsfi_mode" to nacl_helper > into a separate change? > > There's already a place where we're passing "uses_nonsfi_mode" to the > nacl_helper process. nacl_listener.cc receives NaClProcessMsg_Start, which > contains this flag, but this isn't early enough for the seccomp-bpf sandboxing. > > Rather than having two places where this is plumbed through, can you change > nacl_listener.cc to receive the flag via the new route you're adding? > (Otherwise I'd worry that the two could get out of sync.) Thanks for the review! I've uploaded another CL for review. https://codereview.chromium.org/216603002/ I'll update this CL based on your comments once it turned out the preparation CLs seem to be OK. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:93: NaClListener listener; On 2014/03/28 01:38:25, Mark Seaborn wrote: > This could become "listener(uses_nonsfi_mode)" (see other comment). Done, but I added set_uses_nonsfi_mode for consistency with the following two fields. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:272: NaClChromeMainInitForNonSfi(); On 2014/03/28 01:38:25, Mark Seaborn wrote: > AFAICT, this function isn't defined, so this change doesn't compile? I don't > see any trybot results for this change. > > However, it would be nice to clean up this function. It would make sense to > refactor OnStart() so that it checks params.uses_nonsfi_mode right at the start > and only does the minimum required when running in Non-SFI Mode. Sorry, I forgot to publish this change (https://codereview.chromium.org/209423004/) for review. As this changes the native_client repository, I need to wait for the next NaCl roll until try bot works, I think? https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_sandbox_linux.cc:198: class NonSfiNaClBPFSandboxPolicy : public SandboxBPFPolicy { On 2014/03/28 01:38:25, Mark Seaborn wrote: > Can we put the Non-SFI Mode sandbox policy in a separate file, for clarity, > since it's so much more security critical than the SFI Mode sandbox policy? > (Julien and I talked about this today.) > > The new file can go under components/nacl/loader/nonsfi. Thanks, will do! https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:273: ssize_t read_ret = (*NACL_VTBL(NaClDesc, descriptor)->Read)( On 2014/03/28 01:38:25, Mark Seaborn wrote: > Julien and I are both OK with allowing the pread() syscall, so you can leave > this as it was and change the BPF policy. OK, I'd remove this change. https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_main.cc (left): https://codereview.chromium.org/196793023/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_main.cc:181: NaClSrpcModuleInit(); On 2014/03/28 01:38:25, Mark Seaborn wrote: > On 2014/03/24 16:25:43, hamaji wrote: > > This was called from here for historical reason, I think. Hidehiko introduced > > this file first, but now this module is initialized in NaClChromeMainInit, so > we > > don't need this anymore. > > Calling NaClChromeMainInit() for the non-SFI code path is somewhat accidental, > though. Was there a problem with calling NaClSrpcModuleInit() twice? There's > nothing wrong with removing this call, but AFAICT it's not related to tightening > up the sandbox. > > Could you send this for review as a separate change, please? I'll send a patch after https://codereview.chromium.org/209423004/ is landed, and then return to this change.
https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/196793023/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_sandbox_linux.cc:246: // glibc internally calls brk in its memory allocation. On 2014/03/24 15:56:37, hamaji wrote: > On 2014/03/20 00:54:53, jln wrote: > > On 2014/03/18 23:53:58, Mark Seaborn wrote: > > > glibc does call brk(), but it doesn't require it to work. > > > It's OK for brk() to > > > fail: glibc will fall back to using mmap(). > > > > Yes, in other words, just EPERM it. > > Oops, my comment was wrong. brk was called by tcmalloc. We can get rid of brk > call by removing #define HAVE_SBRK 1 from > third_party/tcmalloc/chromium/src/config_linux.h. I think there are some > options: > > 1. Remove HAVE_SBRK from config_linux.h. I feel this is the right choice, but > I'd like to avoid this, because this might affect a lot of things (memory > profiling tools, etc.). > 2. Remove HAVE_SBRK only from nacl_helper. Currently, nacl_helper uses the same > liballocator.a as chrome's, so some gyp tweaks would be needed. > 3. Stop using tcmalloc from nacl_helper. I'm not sure if nacl_helper really > wants tcmalloc. > 4. Wait until newlib switch. Once we have switched to newlib and split > nacl_helper into SFI and non-SFI helpers, we don't need to link tcmalloc in > non-SFI nacl_helper. Does tcmalloc currently require brk(), or does it fall back to mmap() if brk() fails? Either way, I think it would make sense to stop linking tcmalloc into nacl_helper. Statically linking tcmalloc is mostly just wasting space. We don't need tcmalloc for SFI mode. Unlike a renderer, NaCl's trusted runtime doesn't do a lot of allocations, so there's no need to tweak the allocator by overriding glibc's one.
As a preparation (https://codereview.chromium.org/226033002/) is not landed yet, I didn't modify nacl_helper_linux.cc in this change. In the unittest, I used syscall() instead of libc's syscall wrappers because I'm guessing we might want to test this with the lowest interface. I used libc wrappers for a few syscalls whose interface in an architecture is different from the one in other architectures. I'm not sure about the right direction for this. As always, suggestions are appreciated! https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:179: ASSERT_SIGSYS_MSG(DoClone(), ""); Julien: We should be able to use "clone\\(\\) failure" if sigsys_handlers.cc does not have !defined(NDEBUG) around RAW_LOG. Why isn't it consistent consistent with CrashSIGSYS_Handler, which always output a message to stderr? Is it OK to remove !defined(NDEBUG) from sigsys_handlers.cc in a separate change? https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:184: ASSERT_SIGSYS_MSG(syscall(__NR_prctl, PR_SET_DUMPABLE, 1UL), ""); ditto.
https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:214: return ErrorCode(EPERM); Do you want to group all the EPERM into a separate function, such as "IsGracefullyDenied(int sysno)" ? Also let's do the if (IsGracefullyDenied(sysno)) { return EPERM; } at the end, before the "default case". I would like to see this order, as it does facilitate a security review: 1. Stuff that is unconditionally allowed. 2. Stuff that is allowed under restriction. 3. Stuff that is EPERM-ed 4. Stuff that will trap. (1) and (2) are the classes we care about to check the kernel's attack surface. https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:265: case __NR_recvmsg: These guys are pretty problematic :( https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:31: #define ASSERT_SIGSYS_MSG(expr, msg) \ This doesn't look too bad, but in case you need it: - Look at //sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc to see some unit tests and our helpers. I think it's fine for components/ unittests to use the unittests helpers in sandbox/. - //sandbox/linux/services/scoped_process.h can be very handy. https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:179: ASSERT_SIGSYS_MSG(DoClone(), ""); On 2014/04/07 21:17:59, hamaji wrote: > Julien: We should be able to use "clone\\(\\) failure" if sigsys_handlers.cc > does not have !defined(NDEBUG) around RAW_LOG. Why isn't it consistent > consistent with CrashSIGSYS_Handler, which always output a message to stderr? Is > it OK to remove !defined(NDEBUG) from sigsys_handlers.cc in a separate change? Yes!
https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:214: return ErrorCode(EPERM); On 2014/04/08 00:51:10, jln wrote: > Do you want to group all the EPERM into a separate function, such as > "IsGracefullyDenied(int sysno)" ? > > Also let's do the if (IsGracefullyDenied(sysno)) { return EPERM; } at the end, > before the "default case". > > I would like to see this order, as it does facilitate a security review: > > 1. Stuff that is unconditionally allowed. > 2. Stuff that is allowed under restriction. > 3. Stuff that is EPERM-ed > 4. Stuff that will trap. > > (1) and (2) are the classes we care about to check the kernel's attack surface. Done. https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:265: case __NR_recvmsg: On 2014/04/08 00:51:10, jln wrote: > These guys are pretty problematic :( One of my naive question: isn't it possible to add dereference operation to seccomp-bpf so that we can check msghdr::msg_name or arguments of socketcall? I can imagine BPF does not have such concept though. https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:31: #define ASSERT_SIGSYS_MSG(expr, msg) \ On 2014/04/08 00:51:10, jln wrote: > This doesn't look too bad, but in case you need it: > > - Look at //sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc to see some unit > tests and our helpers. > > I think it's fine for components/ unittests to use the unittests helpers in > sandbox/. > > - //sandbox/linux/services/scoped_process.h can be very handy. Ah, the test utilities are nice. I should have guessed there should be this kind of test infra for seccomp. Done.
This looks pretty much ready to me. Sorry about the little annoyance in the unit tests (having to fork() makes everything hard). If this work with Mark, I think I would like to see this landed and then iterate from there. https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:55: unsigned long denied_mask = ~(O_ACCMODE | O_NONBLOCK); Nit: const https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:71: sb->Trap(sandbox::CrashSIGSYS_Handler, NULL)))); Nit: shouldn't this be aligned with sb->Cond()? https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:111: uint32_t denied_mask = ~(PROT_READ | PROT_WRITE | PROT_EXEC); nit const: https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/240001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:59: ASSERT_EQ(0, pthread_create(&th, NULL, &SetZeroInThread, &test_val)); Unfortunately, BPF_TEST requires using BPF_ASSERT() instead of the gtest macros (I think ASSERT "sort-of" works but is very fragile in this situation). It's a bit annoying because we don't have BPF_ASSERT_EQ (but you could easily add that!).
https://codereview.chromium.org/196793023/diff/240001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/240001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:55: unsigned long denied_mask = ~(O_ACCMODE | O_NONBLOCK); On 2014/04/10 21:00:03, jln wrote: > Nit: const Done. https://codereview.chromium.org/196793023/diff/240001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:71: sb->Trap(sandbox::CrashSIGSYS_Handler, NULL)))); On 2014/04/10 21:00:03, jln wrote: > Nit: shouldn't this be aligned with sb->Cond()? Done. I'm not sure if I understand the preferred style in syscall_parameters_restrictions.cc. This file looks a bit inconsistent to me, but I tried to make this similar to syscall_parameters_restrictions.cc. https://codereview.chromium.org/196793023/diff/240001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:111: uint32_t denied_mask = ~(PROT_READ | PROT_WRITE | PROT_EXEC); On 2014/04/10 21:00:03, jln wrote: > nit const: Done. https://codereview.chromium.org/196793023/diff/240001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:120: uint32_t denied_flag_mask = ~(MAP_SHARED | MAP_PRIVATE | MAP_ANONYMOUS | I also added const here. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.h (right): https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.h:35: bool InitializeBPFSandbox(); I changed the return value from void to bool, as this is more compatible with my another change: https://codereview.chromium.org/226033002/ https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:98: BPF_ASSERT_EQ(static_cast<int>(payload.size()), sendmsg(fds[1], &msg, 0)); I found this assertion was wrong but this test didn't catch this issue before I changed ASSERT_EQ to BPF_ASSERT_EQ. Interesting...
I'm only at the beginning of looking at the tests and in a bit of a rush, but I thought I would send you an early review: It looks like you're constructing some tests like this: ASSERT(Do1()); ASSERT.... ASSERT... DoSomeThatBlowsUp. And you're testing whether things are blowing up. The problem is that if Do1() blows up, the test will pass. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:30: static const char kSeccompFailureMsg[] = "seccomp-bpf failure"; This is a hidden dependency on the messages in sigsys_handlers.cc Could we do better? Maybe add a GetErrorMessageContentForTests() in sigsys_handlers.h Then do something along the lines of #define SECCOMP_MESSAGE_COMMON_CONTENT "seccomp-bpf failures" in sigsys_handlers.cc and replace that string with SECCOMP_MESSAGE_COMMON_CONTENT inside the existing strings. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:51: _Exit(0); Nit: _exit() is more common. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:67: // The sanity check for DoClone without the sandbox. Why do you say this is not running in the sandbox? https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:71: BPF_ASSERT_EQ(pid, waitpid(pid, &status, 0)); HANDLE_EINTR() https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:72: BPF_ASSERT_EQ(1, WIFEXITED(status)); I don't think you have a guarantee that this will be 1. I would just BPF_ASSERT(WIFEXITED()) https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:75: DoClone(); I don't understand what you're doing here. But the test is dangerously constructed: how could you know if the first DoClone() causes SIGSYS or the second? https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:98: BPF_ASSERT_EQ(static_cast<int>(payload.size()), sendmsg(fds[1], &msg, 0)); On 2014/04/11 01:25:58, hamaji wrote: > I found this assertion was wrong but this test didn't catch this issue before I > changed ASSERT_EQ to BPF_ASSERT_EQ. Interesting... Yeah, GTEST blows up silently when one uses fork(). That's why we has to create our own BPF_ASSERT etc..
https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:198: DEATH_MESSAGE(kSeccompFailureMsg), Same remark as above: you need to split this into two tests: 1. a normal test for the case that should be allowed. 2. a second test for the case that will SIGSYS. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:208: DEATH_MESSAGE(kSeccompFailureMsg), Same remark: split https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:413: // The rest of syscalls should just raise SIGSYS regardless of arguments. I wonder if we shouldn't fix 5 arguments (to 0) regardless, just to prevent flakiness (I think the syscall() wrapper might leave the unused arguments uninitialized). https://codereview.chromium.org/196793023/diff/280001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/bpf_tests.h (right): https://codereview.chromium.org/196793023/diff/280001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/bpf_tests.h:47: #define BPF_ASSERT_EQ(x, y) BPF_ASSERT(x == y) Don't forget parentheses around (x) and (y)
https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:30: static const char kSeccompFailureMsg[] = "seccomp-bpf failure"; On 2014/04/11 19:29:54, jln wrote: > This is a hidden dependency on the messages in sigsys_handlers.cc > > Could we do better? > > Maybe add a GetErrorMessageContentForTests() in sigsys_handlers.h > > Then do something along the lines of > > #define SECCOMP_MESSAGE_COMMON_CONTENT "seccomp-bpf failures" in > sigsys_handlers.cc and replace that string with SECCOMP_MESSAGE_COMMON_CONTENT > inside the existing strings. Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:51: _Exit(0); On 2014/04/11 19:29:54, jln wrote: > Nit: _exit() is more common. Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:67: // The sanity check for DoClone without the sandbox. On 2014/04/11 19:29:54, jln wrote: > Why do you say this is not running in the sandbox? Sorry, this was not wrong when I didn't use BPF_DEATH_TEST. I didn't think a lot when I changed all TEST to BPF_TEST or BPF_DEATH_TEST, so some tests were invalid, as you mentioned. Thanks for the catch. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:71: BPF_ASSERT_EQ(pid, waitpid(pid, &status, 0)); On 2014/04/11 19:29:54, jln wrote: > HANDLE_EINTR() Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:72: BPF_ASSERT_EQ(1, WIFEXITED(status)); On 2014/04/11 19:29:54, jln wrote: > I don't think you have a guarantee that this will be 1. I would just > BPF_ASSERT(WIFEXITED()) > Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:75: DoClone(); On 2014/04/11 19:29:54, jln wrote: > I don't understand what you're doing here. But the test is dangerously > constructed: how could you know if the first DoClone() causes SIGSYS or the > second? Now I splitted this BPF_DEATH_TEST into two tests. I did the same for some other tests. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:198: DEATH_MESSAGE(kSeccompFailureMsg), On 2014/04/11 20:19:47, jln wrote: > Same remark as above: you need to split this into two tests: > > 1. a normal test for the case that should be allowed. > 2. a second test for the case that will SIGSYS. Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:208: DEATH_MESSAGE(kSeccompFailureMsg), On 2014/04/11 20:19:47, jln wrote: > Same remark: split Done. https://codereview.chromium.org/196793023/diff/280001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:413: // The rest of syscalls should just raise SIGSYS regardless of arguments. On 2014/04/11 20:19:47, jln wrote: > I wonder if we shouldn't fix 5 arguments (to 0) regardless, just to prevent > flakiness (I think the syscall() wrapper might leave the unused arguments > uninitialized). Done. Isn't there a syscall which takes 6 arguments? I'm asking this because the table in this man page has from arg1 to arg6. http://man7.org/linux/man-pages/man2/syscall.2.html https://codereview.chromium.org/196793023/diff/280001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/bpf_tests.h (right): https://codereview.chromium.org/196793023/diff/280001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/bpf_tests.h:47: #define BPF_ASSERT_EQ(x, y) BPF_ASSERT(x == y) On 2014/04/11 20:19:47, jln wrote: > Don't forget parentheses around (x) and (y) Done.
This looks pretty much ready to me, with a few nits. Mark, what do you think? It would be nice to land this ASAP, even if we want to iterate from it (perhaps add a few more tests). https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:35: All the BPF_TEST will silently pass on a machine without seccomp-bpf, on bots running TSAN, etc, by design. It's unfortunately the only thing to do. But perhaps you should add something such as: TEST(NaClNonSfiSandboxTest, BPFIsSupported) { bool seccomp_bpf_supported = false; #if !defined(THREAD_SANITIZER) seccomp_bpf_supported = SandboxBPF::SupportsSeccompSandbox(-1) == SandboxBPF::STATUS_AVAILABLE; #endif if (!seccomp_bpf_supported) { LOG(ERROR) << " Seccomp BPF is not supported, these tests will pass without running"; } https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79: DoClone(); At the very least, you need to BPF_ASSERT_LT(0, pid); It's unfortunate that you can't wait on your child here. It's not the end of the world, as it'll be reaped by init as soon as this test finishes. https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:110: BPF_ASSERT_EQ(0, close(fds[0])); You could add IGNORE_EINTR() here. Or simply make them ScopedFD above. https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:209: BPF_ASSERT_EQ(0, fcntl(2, F_SETFD, FD_CLOEXEC)); Maybe create a socketpair and do this on one of the fds instead? I'm slightly worried about touching stderr. https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:215: fcntl(2, F_SETFD, 99); Same remark as above. https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:226: BPF_ASSERT_EQ(0, close(fds[0])); IGNORE_EINTR, or better: use a ScopedFD https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:233: fcntl(2, F_SETFL, O_APPEND); Same remark as above. (You could also make a small class such as ScopedDummyFdForTests since you seem to be needing this a lot).
PPAPI tests hidehiko have added recently found we need two more syscalls,
shutdown and pwrite64. It seems they are called inside ppapi proxy. My updated
patch now allows them. Are they OK? Or even if we don't want to allow them, we
can do it in a follow-up patch?
Anyway, thanks Julien for the review. I'll handle your comment shortly.
For the record, here are the stacktraces for the cases where shutdown and
pwrite64 are called.
(gdb) bt
#0 0xf761c614 in sandbox::CrashSIGSYS_Handler(sandbox::arch_seccomp_data const&
, void*) () from /ssd2/chrome2/src/out/Release/lib/libseccomp_bpf_helpers.so
#1 0xf762d4cb in sandbox::Trap::SigSys(int, siginfo*, void*) ()
from /ssd2/chrome2/src/out/Release/lib/libseccomp_bpf.so
#2 0xf762d59b in sandbox::Trap::SigSysAction(int, siginfo*, void*) ()
from /ssd2/chrome2/src/out/Release/lib/libseccomp_bpf.so
#3 <signal handler called>
#4 0xf7636430 in __kernel_vsyscall ()
#5 0xf5ce0817 in shutdown () at ../sysdeps/unix/sysv/linux/i386/socket.S:61
#6 0xf745c027 in base::CancelableSyncSocket::Shutdown() ()
from /ssd2/chrome2/src/out/Release/lib/libbase.so
#7 0xf758ec45 in ppapi::PPB_Audio_Shared::~PPB_Audio_Shared() ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_shared.so
#8 0xf73522ce in ppapi::proxy::Audio::~Audio() ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_proxy.so
#9 0xf7352332 in ppapi::proxy::Audio::~Audio() ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_proxy.so
#10 0xf759ee40 in ppapi::ResourceTracker::ReleaseResource(int) ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_shared.so
#11 0xf7355f61 in ppapi::proxy::(anonymous namespace)::ReleaseResource(int) ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_proxy.so
#0 0xf75f5614 in sandbox::CrashSIGSYS_Handler(sandbox::arch_seccomp_data
const&, void*) () from
/ssd2/chrome2/src/out/Release/lib/libseccomp_bpf_helpers.so
#1 0xf76064cb in sandbox::Trap::SigSys(int, siginfo*, void*) ()
from /ssd2/chrome2/src/out/Release/lib/libseccomp_bpf.so
#2 0xf760659b in sandbox::Trap::SigSysAction(int, siginfo*, void*) ()
from /ssd2/chrome2/src/out/Release/lib/libseccomp_bpf.so
#3 <signal handler called>
#4 0xf760f430 in __kernel_vsyscall ()
#5 0xf5eb0777 in do_pwrite64 (offset=0, count=12, buf=0xf03066bd, fd=35)
at ../sysdeps/unix/sysv/linux/pwrite64.c:54
#6 __libc_pwrite64 (fd=35, buf=0xf03066bd, count=12, offset=0)
at ../sysdeps/unix/sysv/linux/pwrite64.c:77
#7 0xf7494b79 in base::WritePlatformFile(int, long long, char const*, int) ()
from /ssd2/chrome2/src/out/Release/lib/libbase.so
#8 0xf72f967e in ppapi::proxy::FileIOResource::WriteOp::DoWork() ()
from /ssd2/chrome2/src/out/Release/lib/libppapi_proxy.so
#9 0xf72f9031 in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(ppapi::proxy::FileIOResource::WriteOp::*)()>, int
(ppapi::proxy::FileIOResource::WriteOp*), void
(scoped_refptr<ppapi::proxy::FileIOResource::WriteOp>)>, int
(ppapi::proxy::FileIOResource::WriteOp*)>::Run(base::internal::BindStateBase*)
()
from /ssd2/chrome2/src/out/Release/lib/libppapi_proxy.so
On 2014/04/15 18:56:05, hamaji wrote: > PPAPI tests hidehiko have added recently found we need two more syscalls, > shutdown and pwrite64. It seems they are called inside ppapi proxy. My updated > patch now allows them. Are they OK? Or even if we don't want to allow them, we > can do it in a follow-up patch? Yeah, let's allow them for now and discuss it later. I would rather land this soon and handle things incrementally.
https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:35: On 2014/04/15 17:52:32, jln wrote: > All the BPF_TEST will silently pass on a machine without seccomp-bpf, on bots > running TSAN, etc, by design. It's unfortunately the only thing to do. > > But perhaps you should add something such as: > > TEST(NaClNonSfiSandboxTest, BPFIsSupported) { > bool seccomp_bpf_supported = false; > #if !defined(THREAD_SANITIZER) > seccomp_bpf_supported = SandboxBPF::SupportsSeccompSandbox(-1) == > SandboxBPF::STATUS_AVAILABLE; > #endif > > if (!seccomp_bpf_supported) { > LOG(ERROR) << " Seccomp BPF is not supported, these tests will pass without > running"; > } Done. I'll run tryjobs with tsan bots. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79: DoClone(); On 2014/04/15 17:52:32, jln wrote: > At the very least, you need to BPF_ASSERT_LT(0, pid); > > It's unfortunate that you can't wait on your child here. It's not the end of the > world, as it'll be reaped by init as soon as this test finishes. Done, but I'm not sure if I understand the motivation of this correctly. As this is a death test, the assertion won't be checked. We will want to know if clone has succeeded or not when the clone call happens to succeed, I assume? https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:110: BPF_ASSERT_EQ(0, close(fds[0])); On 2014/04/15 17:52:32, jln wrote: > You could add IGNORE_EINTR() here. Or simply make them ScopedFD above. Done. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:209: BPF_ASSERT_EQ(0, fcntl(2, F_SETFD, FD_CLOEXEC)); On 2014/04/15 17:52:32, jln wrote: > Maybe create a socketpair and do this on one of the fds instead? > > I'm slightly worried about touching stderr. Done. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:215: fcntl(2, F_SETFD, 99); On 2014/04/15 17:52:32, jln wrote: > Same remark as above. Done. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:226: BPF_ASSERT_EQ(0, close(fds[0])); On 2014/04/15 17:52:32, jln wrote: > IGNORE_EINTR, or better: use a ScopedFD Done. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:233: fcntl(2, F_SETFL, O_APPEND); On 2014/04/15 17:52:32, jln wrote: > Same remark as above. (You could also make a small class such as > ScopedDummyFdForTests since you seem to be needing this a lot). Done.
lgtm (but let's wait for Mark's review) https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/320001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79: DoClone(); > Done, but I'm not sure if I understand the motivation of this correctly. As this > is a death test, the assertion won't be checked. We will want to know if clone > has succeeded or not when the clone call happens to succeed, I assume? Ohh sorry, this was a mistake on my part (not noticing this was a death test). Feel free to remove indeed. The real test for clone working is of course pthread_create() above. https://chromiumcodereview.appspot.com/196793023/diff/350001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/350001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:33: static int GetPageSize() { Nit: you can use an anonymous namespace for the whole file (and remove the "static" qualifier), the test functions don't need to be visible.
+jochen for components/components_tests.gyp as it seems this change is almost ready. The background: we are adding a test for a new seccomp-bpf filter and we'd like to run it as a part of component_unittests. https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/320001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79: DoClone(); On 2014/04/15 20:19:52, jln wrote: > > Done, but I'm not sure if I understand the motivation of this correctly. As > this > > is a death test, the assertion won't be checked. We will want to know if clone > > has succeeded or not when the clone call happens to succeed, I assume? > > Ohh sorry, this was a mistake on my part (not noticing this was a death test). > Feel free to remove indeed. OK, let's revert this change. > > The real test for clone working is of course pthread_create() above. https://codereview.chromium.org/196793023/diff/350001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/350001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:33: static int GetPageSize() { On 2014/04/15 20:19:52, jln wrote: > Nit: you can use an anonymous namespace for the whole file (and remove the > "static" qualifier), the test functions don't need to be visible. Done.
My only non-trivial comments are: * Some comments are labelled "[For future]": I'd like to revisit these in later changes, but not in this change. * The amount of copy-and-paste in nonsfi_sandbox_unittest.cc concerns me a little. See below... https://codereview.chromium.org/196793023/diff/390001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/196793023/diff/390001/components/components_t... components/components_tests.gyp:348: ['disable_nacl==0 and OS == "linux"', { Nit: removes spaces around "==" to follow usual Gyp style https://codereview.chromium.org/196793023/diff/390001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl.gyp#new... components/nacl.gyp:277: 'ldflags': ['-pie'], This doesn't make sense for a library, so you can remove it. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:27: #define MAP_STACK 0x20000 // Daisy build environment has old headers. "Daisy" -> "cros_daisy (ARM)", perhaps? The term "Daisy" is really obscure so I'm not sure exactly what is means. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:37: ErrorCode RestrictFcntlCommandsForNaClNonSfi(SandboxBPF* sb) { Now that the Non-SFI sandbox policy is in its own file, you can remove all the "ForNaClNonSfi" suffixes from the function names, for readability. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:39: if (sizeof(long) == 8) Nit: when if/elses are non-trivial (e.g. multi-line), I think the style is to use {}s around the inner statements. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:206: #if defined(__x86_64__) Nit: maybe use #elif for brevity/readability in all cases like this that are obviously mutually-exclusive alternatives? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:221: #if defined(__x86_64__) Similarly, #elif? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:224: case __NR_futex: [For future] We need to restrict this to only allow operations with FUTEX_PRIVATE_FLAG. Otherwise, there is a big, not-so-covert communications channel between processes: two processes that map a common file (e.g. nacl_helper) can wake each other by signalling a futex on the common memory pages. This used to work even on read-only pages, and maybe it still does -- I've not checked. (Yes, read-only pages can still be used for sending messages.) https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:225: case __NR_gettid: [For future] Do we actually need this? Ideally we would not reveal PIDs and TIDs. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:229: case __NR_pipe: [For future] Do we need pipe()? We should aim to disallow it. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:234: case __NR_sigaltstack: We shouldn't be using sigaltstack at the moment. If the tests pass on x86-32 with this removed, I'd like to remove it, just to make it clear if/when we're adding it back. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:241: case __NR_clock_getres: Nit: put next to clock_gettime? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:288: // failure. glibc thinks brk failed the return value of brk Missing "if": "brk failed if" https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:289: // is lesser than the requested address (i.e., brk(addr) < addr) "less than". Also add "." at end of line. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.h (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.h:19: explicit NaClNonSfiBPFSandboxPolicy() { Nit: write "{}" for consistency with next line? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:35: int GetPageSize() { Just use the standard getpagesize() from unistd.h? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:55: #if !defined(THREAD_SANITIZER) Can you add a comment saying why this is disabled under TSan? Is this because SupportsSeccompSandbox() forks a subprocess to check, and TSan doesn't like that? https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:74: *reinterpret_cast<int*>(test_val_ptr) = 0; Nit: use a non-zero value to make it more obvious where you check for this, e.g. 123 https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:85: BPF_ASSERT_EQ(0, test_val); ...then 123 here. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:88: int DoClone() { How about calling this DoFork()? You're using clone() as fork() below, but that's not obvious, so can you comment that? (I started writing a comment saying this isn't safe to use clone() this way in C. But of course fork() is fine.) https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:2405: syscall(__NR_swapoff, 0, 0, 0, 0, 0); Crikey, this file is huge. Does it really make sense to be copy-and-pasting all of this stuff? Is there any reason to believe our whitelist doesn't actually function as a whitelist? Could we reduce the duplication here, e.g. by iterating through syscall numbers, to check __NR_swapoff in 1 line instead of 6 lines? We could do that programmatically (with a loop that fork()s), or with a macro for defining tests. One worry I'd have is that it's too hard to see which are the interesting tests among all the copy-and-pasted tests. At a minimum, we could put the copy-and-pasted tests in a different file from the real tests. https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/sandbox_l... File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/sandbox_l... sandbox/linux/sandbox_linux.gypi:73: 'type': 'static_library', Nit: put after target_name https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h (right): https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h:42: // Following four functions return substrings of error messages used Nit: "The following..." (otherwise I start reading it as a verb instead of an adjective)
I agree about the copy-paste issue. Would it be ok with you to handle as a follow-up bug? It would be nice to get this landed soon. https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:234: case __NR_sigaltstack: On 2014/04/15 23:03:14, Mark Seaborn wrote: > We shouldn't be using sigaltstack at the moment. If the tests pass on x86-32 > with this removed, I'd like to remove it, just to make it clear if/when we're > adding it back. Seconded, I would love to not have it. https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:236: case __NR_pwrite64: I missed this: let's just keep them sorted. https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:241: case __NR_clock_getres: On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: put next to clock_gettime? Could you actually re-sort? There are a few which are no longer sorted. https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:55: #if !defined(THREAD_SANITIZER) On 2014/04/15 23:03:14, Mark Seaborn wrote: > Can you add a comment saying why this is disabled under TSan? Is this because > SupportsSeccompSandbox() forks a subprocess to check, and TSan doesn't like > that? It's because TSAN got smarter at detecting fork() in a recent roll. So, on fork(), TSAN creates a thread in the child. Supports() does fork and then tries to engaged seccomp-bpf in the child, which it can't do because there is a thread in there. You can point to crbug.com/356588 as a comment. https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:74: *reinterpret_cast<int*>(test_val_ptr) = 0; On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: use a non-zero value to make it more obvious where you check for this, e.g. > 123 You might be the first person to not suggest 42 :) https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:2405: syscall(__NR_swapoff, 0, 0, 0, 0, 0); On 2014/04/15 23:03:14, Mark Seaborn wrote: > Crikey, this file is huge. > > Does it really make sense to be copy-and-pasting all of this stuff? > > Is there any reason to believe our whitelist doesn't actually function as a > whitelist? I think it's useful to really test for all of this, in case there is a compiler bug, or a bug in the policy. > Could we reduce the duplication here, e.g. by iterating through syscall numbers, > to check __NR_swapoff in 1 line instead of 6 lines? We could do that > programmatically (with a loop that fork()s), or with a macro for defining tests. > > One worry I'd have is that it's too hard to see which are the interesting tests > among all the copy-and-pasted tests. At a minimum, we could put the > copy-and-pasted tests in a different file from the real tests. I agree, it's unfortunate. I think we should list these system calls in a vector and then iterate over this. It's non trivial though, since we'll need to duplicate the BPF_DEATH_TEST logic. How about doing this in a follow-up bug?
https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/196793023/diff/390001/components/nacl/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:56: seccomp_bpf_supported = ( Do you mind also adding ASSERT_TRUE(ThreadHelpers::IsSingleThreaded(-1)); ?
Julien, do you have an idea about the failure in this tsan builder? http://build.chromium.org/p/tryserver.chromium/builders/linux_tsan/builds/880 I think this is valgrind-based tsan, and the compiler switch does not work for this. It seems most tests are failing because BPF_DEATH_TEST detects four threads: http://build.chromium.org/p/tryserver.chromium/builders/linux_tsan/builds/880... This one is failing due to a different reason. It seems fork does not work on tsan: http://build.chromium.org/p/tryserver.chromium/builders/linux_tsan/builds/880... https://codereview.chromium.org/196793023/diff/390001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/196793023/diff/390001/components/components_t... components/components_tests.gyp:348: ['disable_nacl==0 and OS == "linux"', { On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: removes spaces around "==" to follow usual Gyp style Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl.gyp#new... components/nacl.gyp:277: 'ldflags': ['-pie'], On 2014/04/15 23:03:14, Mark Seaborn wrote: > This doesn't make sense for a library, so you can remove it. Oops, done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:27: #define MAP_STACK 0x20000 // Daisy build environment has old headers. On 2014/04/15 23:03:14, Mark Seaborn wrote: > "Daisy" -> "cros_daisy (ARM)", perhaps? The term "Daisy" is really obscure so > I'm not sure exactly what is means. I changed this to "Chrome OS Daisy (ARM)" to let this be more explicit. I copied this comment from syscall_parameters_restrictions.cc, so I also modified the original comment. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:37: ErrorCode RestrictFcntlCommandsForNaClNonSfi(SandboxBPF* sb) { On 2014/04/15 23:03:14, Mark Seaborn wrote: > Now that the Non-SFI sandbox policy is in its own file, you can remove all the > "ForNaClNonSfi" suffixes from the function names, for readability. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:39: if (sizeof(long) == 8) On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: when if/elses are non-trivial (e.g. multi-line), I think the style is to > use {}s around the inner statements. Done, also modified syscall_parameters_restrictions.cc. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:206: #if defined(__x86_64__) On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: maybe use #elif for brevity/readability in all cases like this that are > obviously mutually-exclusive alternatives? Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:221: #if defined(__x86_64__) On 2014/04/15 23:03:14, Mark Seaborn wrote: > Similarly, #elif? Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:224: case __NR_futex: On 2014/04/15 23:03:14, Mark Seaborn wrote: > [For future] We need to restrict this to only allow operations with > FUTEX_PRIVATE_FLAG. > > Otherwise, there is a big, not-so-covert communications channel between > processes: two processes that map a common file (e.g. nacl_helper) can wake > each other by signalling a futex on the common memory pages. > > This used to work even on read-only pages, and maybe it still does -- I've not > checked. (Yes, read-only pages can still be used for sending messages.) Got it. I assume we want TODOs for them? Added some information. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:225: case __NR_gettid: On 2014/04/15 23:03:14, Mark Seaborn wrote: > [For future] Do we actually need this? Ideally we would not reveal PIDs and > TIDs. FYI: I tried to change this to EPERM and a test was failing. One caller of this is PlatformThread::CurrentId(). https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:229: case __NR_pipe: On 2014/04/15 23:03:14, Mark Seaborn wrote: > [For future] Do we need pipe()? We should aim to disallow it. FYI: base::MessagePumpLibevent::Init crashes without this. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:234: case __NR_sigaltstack: On 2014/04/15 23:03:14, Mark Seaborn wrote: > We shouldn't be using sigaltstack at the moment. If the tests pass on x86-32 > with this removed, I'd like to remove it, just to make it clear if/when we're > adding it back. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:236: case __NR_pwrite64: On 2014/04/16 01:36:11, jln wrote: > I missed this: let's just keep them sorted. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:241: case __NR_clock_getres: On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: put next to clock_gettime? Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:241: case __NR_clock_getres: On 2014/04/16 01:36:11, jln wrote: > On 2014/04/15 23:03:14, Mark Seaborn wrote: > > Nit: put next to clock_gettime? > > Could you actually re-sort? There are a few which are no longer sorted. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:288: // failure. glibc thinks brk failed the return value of brk On 2014/04/15 23:03:14, Mark Seaborn wrote: > Missing "if": "brk failed if" Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:289: // is lesser than the requested address (i.e., brk(addr) < addr) On 2014/04/15 23:03:14, Mark Seaborn wrote: > "less than". Also add "." at end of line. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.h (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.h:19: explicit NaClNonSfiBPFSandboxPolicy() { On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: write "{}" for consistency with next line? Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:35: int GetPageSize() { On 2014/04/15 23:03:14, Mark Seaborn wrote: > Just use the standard getpagesize() from unistd.h? I created this because the man page of getpagesize said POSIX deprecated getpagesize(), but done. This file is only for linux anyway. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:74: *reinterpret_cast<int*>(test_val_ptr) = 0; On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: use a non-zero value to make it more obvious where you check for this, e.g. > 123 Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:85: BPF_ASSERT_EQ(0, test_val); On 2014/04/15 23:03:14, Mark Seaborn wrote: > ...then 123 here. Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:88: int DoClone() { On 2014/04/15 23:03:14, Mark Seaborn wrote: > How about calling this DoFork()? > > You're using clone() as fork() below, but that's not obvious, so can you comment > that? > > (I started writing a comment saying this isn't safe to use clone() this way in > C. But of course fork() is fine.) Done. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:2405: syscall(__NR_swapoff, 0, 0, 0, 0, 0); On 2014/04/15 23:03:14, Mark Seaborn wrote: > Crikey, this file is huge. > > Does it really make sense to be copy-and-pasting all of this stuff? > > Is there any reason to believe our whitelist doesn't actually function as a > whitelist? > > Could we reduce the duplication here, e.g. by iterating through syscall numbers, > to check __NR_swapoff in 1 line instead of 6 lines? We could do that > programmatically (with a loop that fork()s), or with a macro for defining tests. > > One worry I'd have is that it's too hard to see which are the interesting tests > among all the copy-and-pasted tests. At a minimum, we could put the > copy-and-pasted tests in a different file from the real tests. I'm not sure if they are actually useful. I imagined they could be useful to prevent a very weird bug in SandboxBPF or something. I agree this file is too big and they are not interesting. I defined a few macros to compress them and moved them to another file. Only whitelisted syscalls exist in this file now. https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/sandbox_l... File sandbox/linux/sandbox_linux.gypi (right): https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/sandbox_l... sandbox/linux/sandbox_linux.gypi:73: 'type': 'static_library', On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: put after target_name Done. https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h (right): https://codereview.chromium.org/196793023/diff/390001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h:42: // Following four functions return substrings of error messages used On 2014/04/15 23:03:14, Mark Seaborn wrote: > Nit: "The following..." (otherwise I start reading it as a verb instead of an > adjective) Done.
https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:55: #if !defined(THREAD_SANITIZER) On 2014/04/16 01:36:11, jln wrote: > On 2014/04/15 23:03:14, Mark Seaborn wrote: > > Can you add a comment saying why this is disabled under TSan? Is this because > > SupportsSeccompSandbox() forks a subprocess to check, and TSan doesn't like > > that? > > It's because TSAN got smarter at detecting fork() in a recent roll. > So, on fork(), TSAN creates a thread in the child. Supports() does fork and then > tries to engaged seccomp-bpf in the child, which it can't do because there is a > thread in there. > > You can point to crbug.com/356588 as a comment. Thanks. Added a comment. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:56: seccomp_bpf_supported = ( On 2014/04/16 01:39:38, jln wrote: > Do you mind also adding ASSERT_TRUE(ThreadHelpers::IsSingleThreaded(-1)); ? I'm happy to add this, but I guess this will fail on linux_tsan as well? Please see the cover comment of my previous reply. https://codereview.chromium.org/196793023/diff/390001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:74: *reinterpret_cast<int*>(test_val_ptr) = 0; On 2014/04/16 01:36:11, jln wrote: > On 2014/04/15 23:03:14, Mark Seaborn wrote: > > Nit: use a non-zero value to make it more obvious where you check for this, > e.g. > > 123 > > You might be the first person to not suggest 42 :) Hehe, 42 has been already used as its initial value :)
I'm not sure what to do about TSANv1, I only ever ran sandbox_linux_unittests on TSANv2. Adding Alexander and Dmitry as cc: Do you guys know how we could detect TSANv1 at runtime? (Detecting Valgrind won't work as we don't want to loose Valgrind coverage). In the worse case, maybe we should just remove component_unittests from TSANv1 coverage on Linux (and rely on TSANv2 there). https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No (c) in new files, just "Copyright 2014" https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:70: *reinterpret_cast<int*>(test_val_ptr) = 123; Let's make it a global "const int kExpectedValue = 123;" since it's shared with the test.
On Wed, Apr 16, 2014 at 6:27 AM, <jln@chromium.org> wrote: > I'm not sure what to do about TSANv1, I only ever ran > sandbox_linux_unittests on > TSANv2. > > Adding Alexander and Dmitry as cc: Do you guys know how we could detect > TSANv1 > at runtime? (Detecting Valgrind won't work as we don't want to loose > Valgrind > coverage). > > In the worse case, maybe we should just remove component_unittests from > TSANv1 > coverage on Linux (and rely on TSANv2 there). TSANv1 is deprecated now. So do whatever consumes less of your time. Excluding tests sounds good. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... > File components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc > (right): > > https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc:1: // > Copyright (c) 2014 The Chromium Authors. All rights reserved. > No (c) in new files, just "Copyright 2014" > > https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... > File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): > > https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:70: > *reinterpret_cast<int*>(test_val_ptr) = 123; > Let's make it a global "const int kExpectedValue = 123;" since it's > shared with the test. > > https://codereview.chromium.org/196793023/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
components_tests.gypi lgtm
Please change the commit message (the "Description" field in Rietveld) to start with "Add seccomp sandbox for non-SFI NaCl". Note that the "Subject" field gets ignored by the tools (CQ, git cl dcommit, etc.) when committing -- it does not get prepended to the "Description" field. Then LGTM. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:210: // NaCl runtime exposes clock_gettime and clock_getres to untrusted code. Nit: put above clock_getres? https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc:12: // Test cases in this file just makes sure not-whitelisted syscalls "make sure" https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:85: const int pid = syscall(__NR_clone, Can you comment this as: Call clone() to do a fork(). https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:86: CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, CLONE_CHILD_CLEARTID and CLONE_CHILD_SETTID both expect a ctid pointer, which you're not passing, and the docs don't say NULL is OK. (This came up recently in the clone wrapper in https://codereview.chromium.org/220933002.) You should just use SIGCHLD for flags to fix this.
https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:210: // NaCl runtime exposes clock_gettime and clock_getres to untrusted code. On 2014/04/16 16:19:25, Mark Seaborn wrote: > Nit: put above clock_getres? Done. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/16 02:27:34, jln wrote: > No (c) in new files, just "Copyright 2014" Done. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_sigsys_unittest.cc:12: // Test cases in this file just makes sure not-whitelisted syscalls On 2014/04/16 16:19:25, Mark Seaborn wrote: > "make sure" Done. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:70: *reinterpret_cast<int*>(test_val_ptr) = 123; On 2014/04/16 02:27:34, jln wrote: > Let's make it a global "const int kExpectedValue = 123;" since it's shared with > the test. Done. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:85: const int pid = syscall(__NR_clone, On 2014/04/16 16:19:25, Mark Seaborn wrote: > Can you comment this as: Call clone() to do a fork(). Done. https://codereview.chromium.org/196793023/diff/430001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:86: CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, On 2014/04/16 16:19:25, Mark Seaborn wrote: > CLONE_CHILD_CLEARTID and CLONE_CHILD_SETTID both expect a ctid pointer, which > you're not passing, and the docs don't say NULL is OK. (This came up recently > in the clone wrapper in https://codereview.chromium.org/220933002.) > > You should just use SIGCHLD for flags to fix this. Done.
The CQ bit was checked by hamaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/196793023/450001
Message was sent while issue was closed.
Change committed as 264383
Message was sent while issue was closed.
On 2014/04/17 01:55:11, I haz the power (commit-bot) wrote: > Change committed as 264383 Ohh, bummer, this broke ASAN on the main WF. I'm reverting. https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20B... [47/52] LINK unit_tests FAILED: clang++ -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -pthread -Wl,-z,noexecstack -fPIC -B/b/build/slave/Linux_ASan_LSan_Builder/build/src/third_party/binutils/Linux_x64/Release/bin -Wl,--disable-new-dtags -L. -Wl,-u_sanitizer_options_link_helper -pie -m64 -fsanitize=address -fsanitize=leak -Wl,--icf=none -Wl,-O1 -Wl,--gc-sections -o nacl_helper -Wl,--start-group obj/components/nacl/loader/nacl_helper.nacl_helper_linux.o obj/base/libsanitizer_options.a obj/components/libnacl_loader.a obj/components/libnacl.a obj/base/libbase.a obj/base/libbase_static.a obj/base/allocator/liballocator_extension_thunks.a obj/third_party/modp_b64/libmodp_b64.a obj/base/third_party/dynamic_annotations/libdynamic_annotations.a obj/base/libsymbolize.a obj/base/libxdg_mime.a obj/third_party/libevent/libevent.a obj/ipc/libipc.a obj/ppapi/libppapi_shared.a obj/base/libbase_i18n.a obj/third_party/icu/libicui18n.a obj/third_party/icu/libicuuc.a obj/third_party/icu/libicudata.a obj/gpu/command_buffer/libgles2_utils.a obj/gpu/libcommand_buffer_client.a obj/gpu/libcommand_buffer_common.a obj/gpu/libgles2_implementation.a obj/ui/gl/libgl_wrapper.a obj/skia/libskia_library.a obj/skia/libskia_opts.a obj/skia/libskia_opts_ssse3.a obj/third_party/zlib/libchrome_zlib.a obj/third_party/sfntly/libsfntly.a obj/skia/libskia_chrome.a obj/skia/libskia_chrome_opts.a obj/ui/gfx/libgfx.a obj/third_party/libpng/libpng.a obj/ui/gfx/libgfx_geometry.a obj/third_party/libjpeg_turbo/libjpeg_turbo.a obj/ui/gfx/libgfx_x11.a obj/gpu/libgles2_cmd_helper.a obj/media/libshared_memory_support.a obj/media/libshared_memory_support_sse.a obj/ui/surface/libsurface.a obj/media/libmedia.a obj/crypto/libcrcrypto.a obj/net/third_party/nss/libcrssl.a obj/third_party/opus/libopus.a obj/ui/events/libevents_base.a obj/ui/events/libdom4_keycode_converter.a obj/url/liburl_lib.a obj/third_party/ffmpeg/libffmpeg.a obj/third_party/libvpx/libvpx.a obj/third_party/libvpx/libvpx_asm_offsets_vp8.a obj/third_party/libvpx/libvpx_intrinsics_mmx.a obj/third_party/libvpx/libvpx_intrinsics_sse2.a obj/third_party/libvpx/libvpx_intrinsics_ssse3.a obj/media/libmedia_asm.a obj/media/libmedia_mmx.a obj/media/libmedia_sse.a obj/media/libmedia_sse2.a obj/ui/base/libui_base.a obj/net/libnet.a obj/sdch/libsdch.a obj/build/linux/libgio.a obj/ui/events/libevents.a obj/third_party/WebKit/Source/platform/libblink_platform.a obj/third_party/WebKit/Source/wtf/libwtf.a obj/third_party/WebKit/Source/platform/libblink_common.a obj/third_party/WebKit/Source/platform/libblink_heap_asm_stubs.a obj/gpu/libgles2_c_lib.a obj/third_party/libwebp/libwebp_dec.a obj/third_party/libwebp/libwebp_dsp.a obj/third_party/libwebp/libwebp_utils.a obj/third_party/libwebp/libwebp_demux.a obj/third_party/libwebp/libwebp_enc.a obj/third_party/ots/libots.a obj/third_party/brotli/libbrotli.a obj/third_party/qcms/libqcms.a obj/v8/tools/gyp/libv8_base.x64.a obj/v8/tools/gyp/libv8_snapshot.a obj/third_party/iccjpeg/libiccjpeg.a obj/third_party/harfbuzz-ng/libharfbuzz-ng.a obj/third_party/WebKit/Source/web/libblink_web.a obj/third_party/WebKit/Source/core/libwebcore_dom.a obj/third_party/angle/src/libtranslator.a obj/third_party/angle/src/libpreprocessor.a obj/third_party/libxml/libxml2.a obj/third_party/libxslt/libxslt.a obj/third_party/sqlite/libsqlite3.a obj/third_party/WebKit/Source/core/libwebcore_html.a obj/third_party/WebKit/Source/core/libwebcore_remaining.a obj/third_party/WebKit/Source/core/libwebcore_rendering.a obj/third_party/WebKit/Source/core/libwebcore_svg.a obj/third_party/WebKit/Source/core/libwebcore_generated.a obj/gin/libgin.a obj/third_party/WebKit/Source/modules/libmodules.a obj/ppapi/libppapi_ipc.a obj/gpu/libgpu_ipc.a obj/native_client/src/trusted/service_runtime/libsel.a obj/native_client/src/trusted/service_runtime/libenv_cleanser.a obj/native_client/src/trusted/service_runtime/libnacl_error_code.a obj/native_client/src/shared/gio/libgio.a obj/native_client/src/shared/platform/libplatform.a obj/native_client/src/shared/srpc/libnonnacl_srpc.a obj/native_client/src/trusted/debug_stub/libdebug_stub.a obj/native_client/src/trusted/desc/libnrd_xfer.a obj/native_client/src/trusted/desc/libdesc_wrapper.a obj/native_client/src/shared/imc/libimc.a obj/native_client/src/trusted/nacl_base/libnacl_base.a obj/native_client/src/trusted/desc_cacheability/libdesc_cacheability.a obj/native_client/src/trusted/fault_injection/libnacl_fault_inject.a obj/native_client/src/trusted/gio/libgio_wrapped_desc.a obj/native_client/src/trusted/interval_multiset/libnacl_interval.a obj/native_client/src/trusted/perf_counter/libnacl_perf_counter.a obj/native_client/src/trusted/platform_qualify/libplatform_qual_lib.a obj/native_client/src/trusted/cpu_features/libcpu_features.a obj/native_client/src/trusted/manifest_name_service_proxy/libmanifest_proxy.a obj/native_client/src/trusted/threading/libthread_interface.a obj/native_client/src/trusted/simple_service/libsimple_service.a obj/native_client/src/trusted/validator/libvalidation_cache.a obj/native_client/src/trusted/validator/libvalidators.a obj/native_client/src/trusted/service_runtime/arch/x86/libservice_runtime_x86_common.a obj/native_client/src/trusted/validator_ragel/libdfa_validate_x86_64.a obj/native_client/src/trusted/service_runtime/arch/x86_64/libservice_runtime_x86_64.a obj/native_client/src/trusted/validator_x86/libnccopy_x86_64.a obj/native_client/src/trusted/service_runtime/libnacl_signal.a obj/components/libnacl_common.a obj/content/libcontent_common.a obj/components/libtracing.a obj/third_party/libjingle/libjingle.a obj/third_party/libjingle/libjingle_p2p_constants.a obj/ui/accessibility/libaccessibility.a obj/ui/accessibility/libax_gen.a obj/tools/json_schema_compiler/libapi_gen_util.a obj/ui/shell_dialogs/libshell_dialogs.a obj/ui/aura/libaura.a obj/gpu/libcommand_buffer_service.a obj/gpu/libdisk_cache_proto.a obj/third_party/protobuf/libprotobuf_lite.a obj/third_party/re2/libre2.a obj/third_party/smhasher/libcityhash.a obj/gpu/libgpu_config.a obj/build/linux/libpci.a obj/third_party/libXNVCtrl/libXNVCtrl.a obj/ui/compositor/libcompositor.a obj/cc/libcc.a obj/gpu/skia_bindings/libgpu_skia_bindings.a obj/mojo/libmojo_environment_chromium.a obj/mojo/libmojo_common_lib.a obj/mojo/libmojo_system_impl.a obj/mojo/libmojo_environment_chromium_impl.a obj/webkit/common/gpu/libwebkit_gpu.a obj/webkit/common/libwebkit_common.a obj/webkit/libwebkit_storage_browser.a obj/sql/libsql.a obj/third_party/leveldatabase/libleveldatabase.a obj/third_party/snappy/libsnappy.a obj/webkit/libwebkit_storage_common.a obj/components/libnacl_switches.a obj/sandbox/libc_urandom_override.a obj/sandbox/libsandbox_services.a obj/sandbox/libsuid_sandbox_client.a obj/sandbox/libseccomp_bpf.a obj/sandbox/libseccomp_bpf_helpers.a obj/ppapi/libppapi_proxy.a -Wl,--end-group -lrt -ldl -lgmodule-2.0 -lgobject-2.0 -lgthread-2.0 -lglib-2.0 -lfontconfig -lfreetype -lpangocairo-1.0 -lcairo -lpangoft2-1.0 -lpango-1.0 -lX11 -lXi -lXcomposite -lXext -lasound -lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4 -lXdamage -lXfixes -lXtst -lgconf-2 -lgio-2.0 -lresolv -lXcursor -lXrender -lpthread -lexpat -lXrandr -lcap /b/build/slave/Linux_ASan_LSan_Builder/build/src/third_party/binutils/Linux_x64/Release/bin/ld: error: obj/base/libsanitizer_options.a(obj/base/debug/sanitizer_options.sanitizer_options.o): multiple definition of '__asan_default_options' /b/build/slave/Linux_ASan_LSan_Builder/build/src/third_party/binutils/Linux_x64/Release/bin/ld: obj/components/nacl/loader/nacl_helper.nacl_helper_linux.o: previous definition here clang: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/240613003/ by jln@chromium.org. The reason for reverting is: Broke ASAN on main WF. /b/build/slave/Linux_ASan_LSan_Builder/build/src/third_party/binutils/Linux_x64/Release/bin/ld: error: obj/base/libsanitizer_options.a(obj/base/debug/sanitizer_options.sanitizer_options.o): multiple definition of '__asan_default_options' /b/build/slave/Linux_ASan_LSan_Builder/build/src/third_party/binutils/Linux_x64/Release/bin/ld: obj/components/nacl/loader/nacl_helper.nacl_helper_linux.o: previous definition here clang: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed.. |
