Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10458040:
Initial snapshot of the new BPF-enabled seccomp sandbox. This code is (Closed)
|
Created:
8 years, 6 months ago by Markus (顧孟勤) Modified:
8 years, 6 months ago CC:
Chris Evans, drewry_chromium.org, chromium-reviews, agl, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionInitial snapshot of the new BPF-enabled seccomp sandbox. This code is
still quite incomplete. In fact, it barely even compiles. You can use the
Makefile to experiment with it, but we deliberately have not integrated it
with the Chrome build system at this time.
The main intention for checking in the code at this point is to give others
a chance to take a look at the API. We made a few changes already, and I
want to make sure I give everybody an opportunity to speak up, if they still
want further revisions of the publicly exposed API.
BUG=130662
TEST=build with Makefile, then run demo32 and demo64
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140407
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 38
Patch Set 5 : #
Total comments: 20
Patch Set 6 : #
Total comments: 15
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 35 (0 generated)
Feel free to critique, but at this stage I am more interested in comments on the API than in comments on the actual code. The code is still changing as I am implementing the API and back-porting the code that I wrote earlier.
I took a very quick high level look. I would recommend simplifying still a bit more (sorry) for the first iteration, we can put more stuff back later. Ideally, the first iteration of this patch, could land nicely and we could replace ApplyGPUPolicy() and ApplyFlashPolicy() in sandbox_init_linux.cc to something using your API. For the first iteration it's important and easier to land the exact same policy than the one already there. https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:10: int Sandbox::supportsSeccompSandbox(int proc_fd) { It looks like content/common/sandbox_init_linux.cc is the right place for all sandbox initialization code to go. I think we need to get seccomp initialization out of the Zygote, which means that giving proc_fd will be difficult (we don't want to have a /proc fd that far down the stack). We can solve this later, but I really would prefer to have the signature be int supportsSeccompSandbox(void) for now. It's perhaps ok if isSingleThreaded only works in DEBUG mode without when the setuid sandbox is not here (which is what Chris did). Otherwise, I think you can do a proper check with the ChildProcess singleton: ChildProcess::current() == NULL. Need to finish understanding all the code to be sure. Of course it only works for Chrome code, if glibc or some other decides to create new threads on us that won't work. It's nice to detect it, but it's not worth the trouble for the first iteration of the patch. https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:104: bool Sandbox::disableFilesystem() { Looks good, but this should be kept independent of seccomp BPF IMO. Could be dropped for now to reduce the initial patch complexity ? https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:365: void Sandbox::sigSys(int nr, siginfo_t *info, void *void_context) { For the purpose of merging with Chris code, would you mind doing the same thing as he does for now (access memory with an address that leaks the syscall number)? We can review it later, and I agree that we need a better way to log eventually (I think we should just have a helper process take care of that and write to it via a pipe in the handler).
On Wed, May 30, 2012 at 1:31 PM, <jln@chromium.org> wrote: > I took a very quick high level look. I would recommend simplifying still a > bit > more (sorry) for the first iteration, we can put more stuff back later. > > Ideally, the first iteration of this patch, could land nicely and we could > replace ApplyGPUPolicy() and ApplyFlashPolicy() in sandbox_init_linux.cc to > something using your API. > In terms of landing, merging, IMHO: - Policy (i.e. anything resembling a syscall list) should stay in sandbox_init_linux.cc. - Mechanism should go in src/sandbox. > > For the first iteration it's important and easier to land the exact same > policy > than the one already there. > > > https://chromiumcodereview.**appspot.com/10458040/diff/1/** > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc<https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_bpf/sandbox_bpf.cc> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.cc (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/1/** > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc#newcode10<https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode10> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:10: int > Sandbox::**supportsSeccompSandbox(int proc_fd) { > It looks like content/common/sandbox_init_**linux.cc is the right place > for all sandbox initialization code to go. > > I think we need to get seccomp initialization out of the Zygote, which > means that giving proc_fd will be difficult (we don't want to have a > /proc fd that far down the stack). > > We can solve this later, but I really would prefer to have the signature > be int supportsSeccompSandbox(void) for now. > > It's perhaps ok if isSingleThreaded only works in DEBUG mode without > when the setuid sandbox is not here (which is what Chris did). > > Otherwise, I think you can do a proper check with the ChildProcess > singleton: ChildProcess::current() == NULL. Need to finish understanding > all the code to be sure. > Of course it only works for Chrome code, if glibc or some other decides > to create new threads on us that won't work. It's nice to detect it, but > it's not worth the trouble for the first iteration of the patch. > > https://chromiumcodereview.**appspot.com/10458040/diff/1/** > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc#newcode104<https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode104> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:104: bool > Sandbox::disableFilesystem() { > Looks good, but this should be kept independent of seccomp BPF IMO. > > Could be dropped for now to reduce the initial patch complexity ? > > https://chromiumcodereview.**appspot.com/10458040/diff/1/** > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc#newcode365<https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode365> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:365: void Sandbox::sigSys(int > nr, siginfo_t *info, void *void_context) { > For the purpose of merging with Chris code, would you mind doing the > same thing as he does for now (access memory with an address that leaks > the syscall number)? > > We can review it later, and I agree that we need a better way to log > eventually (I think we should just have a helper process take care of > that and write to it via a pipe in the handler). > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... >
On Wed, May 30, 2012 at 1:37 PM, Chris Evans <cevans@google.com> wrote: > On Wed, May 30, 2012 at 1:31 PM, <jln@chromium.org> wrote: >> >> I took a very quick high level look. I would recommend simplifying still a >> bit >> more (sorry) for the first iteration, we can put more stuff back later. >> >> Ideally, the first iteration of this patch, could land nicely and we could >> replace ApplyGPUPolicy() and ApplyFlashPolicy() in sandbox_init_linux.cc >> to >> something using your API. > > > In terms of landing, merging, IMHO: > - Policy (i.e. anything resembling a syscall list) should stay in > sandbox_init_linux.cc. > - Mechanism should go in src/sandbox. > >> >> >> For the first iteration it's important and easier to land the exact same >> policy >> than the one already there. This is crucial. The current policies have been tested for quite some time now. I want to enable this on Chrome OS but I don't want to enable an untested policy. >> https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... >> File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): >> >> >> https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... >> sandbox/linux/seccomp_bpf/sandbox_bpf.cc:10: int >> Sandbox::supportsSeccompSandbox(int proc_fd) { >> It looks like content/common/sandbox_init_linux.cc is the right place >> for all sandbox initialization code to go. >> >> I think we need to get seccomp initialization out of the Zygote, which >> means that giving proc_fd will be difficult (we don't want to have a >> /proc fd that far down the stack). >> >> We can solve this later, but I really would prefer to have the signature >> be int supportsSeccompSandbox(void) for now. >> >> It's perhaps ok if isSingleThreaded only works in DEBUG mode without >> when the setuid sandbox is not here (which is what Chris did). >> >> Otherwise, I think you can do a proper check with the ChildProcess >> singleton: ChildProcess::current() == NULL. Need to finish understanding >> all the code to be sure. >> Of course it only works for Chrome code, if glibc or some other decides >> to create new threads on us that won't work. It's nice to detect it, but >> it's not worth the trouble for the first iteration of the patch. >> >> >> https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... >> sandbox/linux/seccomp_bpf/sandbox_bpf.cc:104: bool >> Sandbox::disableFilesystem() { >> Looks good, but this should be kept independent of seccomp BPF IMO. >> >> Could be dropped for now to reduce the initial patch complexity ? >> >> >> https://chromiumcodereview.appspot.com/10458040/diff/1/sandbox/linux/seccomp_... >> sandbox/linux/seccomp_bpf/sandbox_bpf.cc:365: void Sandbox::sigSys(int >> nr, siginfo_t *info, void *void_context) { >> For the purpose of merging with Chris code, would you mind doing the >> same thing as he does for now (access memory with an address that leaks >> the syscall number)? >> >> We can review it later, and I agree that we need a better way to log >> eventually (I think we should just have a helper process take care of >> that and write to it via a pipe in the handler). >> >> https://chromiumcodereview.appspot.com/10458040/ > >
Please take another look. I have been stripping out all things that are conceivably complicated or controversial. The upshot is that this code doesn't really do much useful sandboxing at all. But it does enough to get us going (hopefully) and I will then start backfilling the missing pieces.
By Julien's request, I removed the C-style API for now. We might not ever need it again. But if we do, it's easy to add back in.
Looks good in general. I didn't review some details yet and I didn't look at demo / util at all. Here are a few remarks. - We need a way to hook up to a user-specified SIGSYS handler. For now I think that's all we should do. Then we can hook up to Chris' in the first iteration and work on a nice default handler later. - I think you tend to die() too much, but you had some good arguments. My preference is that the caller would make the decision of what to do, but if we're know that the programmer made a mistake and that die() will hit every codepath I can agree. However, please use CHECK() or some such, just killing the process can go unnoticed. - util.* is for demo. only at this point, let's keep it separate from sandbox_* - Please take a look at all the "lint" issues that the review tool raises. - If you can already make demo.cc a test in this iteration, that would be fantastic. If that's hard, let's skip. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:8: namespace playground2 { Don't forget to change it https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:17: die("Failed to check for sandbox support"); I really don't think we should die here. supports should not make you die, that's the point, isn't it ? Now, agreed if we can't fork we're going to have some issues, but I would rather let core Chrome code fail and handle that. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:32: BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, __NR_exit_group, 0, 2), Should we rather use your normal API to install a filter? Should we block all signals? Merely getting a signal for which there is a handler would get us killed since we don't have *sigreturn. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:51: TEMP_FAILURE_RETRY(waitpid(pid, &status, 0)); Very minor: Chrome seems to use HANDLE_EINTR https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:119: filter_failed: Minor: Do you mind putting this label at the end? I find it much easier to review when stuff that exits without return: is at the end. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:170: case SB_INSPECT_ARG_1...SB_INSPECT_ARG_6: We'll need SB_INSPECT_ARG1 for Chris' policy because of kill(). If that's the only show stopper, we can probably live without it and allow kill() unrestricted for now. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:191: BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_DENY)); It's not strictly correct to ENOPERM for many system calls, but I think it's a good default. ENOMEM is more generally supported but it may trigger a panic shut down in those rare callers that look for the actual errno. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:204: if (info->si_code != SYS_SECCOMP || !void_context) { I think for now it's best to let the user specify the SIGSYS handler and do nothing in it. For the first iteration, we'll just keep Chris' handler and change it in the next one. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.h (right): https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:9: #include <endian.h> Please, have the C++ header below the C headers. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:37: #include "util.h" Not needed as far as I can tell. https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:106: namespace playground2 { Don't forget to rename https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:160: // SB_INSPECT_ARG_x bits. As much as I think that "kill" should be avoided, it needs to be an option. Especially because I would like us to move to your API, without any change in semantic in respect to the current filters.
skimming things https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/demo.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/8003/sandbox/linux/secco... sandbox/linux/seccomp_bpf/demo.cc:35: static playground2::Sandbox::ErrorCode evaluator(int sysno) { is it possible to export a syscall bounding set too? Iterating from 0 to MAX_SYSCALLS isn't super portable.
I think, I addressed the majority of issues raised so far. PTAL
https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:172: // eventually. Definitely. In particular, we cannot keep "hot" syscalls at the beginning of the jump table without a non-trivial change to this section of the code. I.e. looks like we'll need syscall number iterator or something.
I know, and I have a plan for how to do this (and I have code that needs back porting to do so). But jln convinced me that this amount of complexity should not be part of the initial change lists. So, I ripped it out for the time being. If I can get the current code merged, expect to see API extensions (e.g. code to inspect arguments, better logging and error handling, custom SIGSYS handling, ...) in the next couple of changelists; and then you should start seeing significant internal changes that improve performance without any API changes. Markus On Thu, May 31, 2012 at 8:06 PM, <jorgelo@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.cc (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode172<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode172> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:172: // eventually. > Definitely. In particular, we cannot keep "hot" syscalls at the > beginning of the jump table without a non-trivial change to this section > of the code. I.e. looks like we'll need syscall number iterator or > something. > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... >
On 2012/06/01 06:33:31, Markus (顧孟勤) wrote: > I know, and I have a plan for how to do this (and I have code that needs > back porting to do so). But jln convinced me that this amount of complexity > should not be part of the initial change lists. So, I ripped it out for the > time being. > > If I can get the current code merged, expect to see API extensions (e.g. > code to inspect arguments, better logging and error handling, custom SIGSYS > handling, ...) in the next couple of changelists; and then you should start > seeing significant internal changes that improve performance without any > API changes. Great! Sounds like the correct way to do it. > > On Thu, May 31, 2012 at 8:06 PM, <mailto:jorgelo@chromium.org> wrote: > > > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > > > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc> > > File sandbox/linux/seccomp_bpf/**sandbox_bpf.cc (right): > > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > > > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode172<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode172> > > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:172: // eventually. > > Definitely. In particular, we cannot keep "hot" syscalls at the > > beginning of the jump table without a non-trivial change to this section > > of the code. I.e. looks like we'll need syscall number iterator or > > something. > > > > > https://chromiumcodereview.**appspot.com/10458040/%3Chttps://chromiumcoderevi...> > >
LGTM I am in favor in landing this so that we can all continue working incrementally towards the API change.
On Fri, Jun 1, 2012 at 12:30 PM, <jln@chromium.org> wrote: > LGTM > > I am in favor in landing this so that we can all continue working > incrementally > towards the API change. > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... > I'll take a quick look now. I like the small size of the CL now.
Here are some quick comments. This looks like a good base we can build on. In terms of follow-up CLs, I'd like APIs such as: - permitBase() (or something; permits things like sigreturn etc. that the Linux ABI might otherwise surprise you with. maybe also exit() and similar always-used things) - permitMalloc() or permitCommon(), possibly rolled into permitBase()? - permitSafe(): allows all the genuinely trivial things that no-one is worried about. getpid, getuid, etc. - denyFileSystemAccess(): ENOENT for any syscall dealing with filenames. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:30: status_ = STATUS_UNSUPPORTED; Should we have a specific status for that? As a user of the API, I'd probably read "unsupported" to mean the kernel support isn't there, not that my program (or point of sandbox initialization) can be tweaked to get it working. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:33: sigfillset(&newMask); Although this realistically isn't going to fail, sigfillset() does have a return value, so it should be checked. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:35: pid_t pid = fork(); I wonder what the overhead is of this check? We'll be doing it for every new renderer process creation, probably. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:44: syscall(__NR_exit_group, (intptr_t)100); Why use syscall(exit_group) here but _exit() below? Seems inconsistent. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:46: for (;;) { Why is the infinite loop needed? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:50: sigprocmask(SIG_SETMASK, &oldMask, NULL); Check return values where possible. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:52: HANDLE_EINTR(waitpid(pid, &status, 0)); Does this look at status without checking that waitpid returned a non-error code return? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:55: } else { Is this branch the case that fork() fails? Is it your intention to leave status_ at STATUS_UNSUPPORTED in this case? Should the fork() failure be logged? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:69: if (status_ == STATUS_UNSUPPORTED) { What should we do for STATUS_UNKNOWN? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:172: // eventually. On 2012/06/01 03:06:18, Jorge Lucangeli Obes wrote: > Definitely. In particular, we cannot keep "hot" syscalls at the beginning of the > jump table without a non-trivial change to this section of the code. I.e. looks > like we'll need syscall number iterator or something. Agree with Jorge, checking e.g. syscall 200 will be very expensive! But we can fix it in a follow-up CL. BTW, I was very excited about the tree-like comparison thing, to get O(log N) check time with number of permitted syscalls? Will that make a return? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:187: ret = SECCOMP_RET_ERRNO + err; For paranoid, probably check that "err" is within bounds of what can be handled. (Check the mask?) https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:213: die("Unexpected SIGSYS received"); I'd also check that nr==SIGSYS https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:218: (void *)(intptr_t)-(int)(SECCOMP_RET_DENY & SECCOMP_RET_DATA); I don't understand that; may be worth a comment? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.h (right): https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:13: // #include <linux/seccomp.h> Remove if it's dead code. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:98: #define Unsupported target platform Did you mean #error ? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:110: (static_cast<int>(sizeof(x)/sizeof(*(x)))) A size macro might want to return size_t ? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:148: Constraint *constraint); Style: I think Chromium style is to position the pointer next to the type, not the type name. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:148: Constraint *constraint); Style: I think Chromium style is to position the pointer next to the type, not the type name. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:160: // The sandbox becomes the newer owner of this file descriptor and will Typo: newer -> new ? https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:191: for (;;) { Why loop infinitely? At least a comment why we're expecting exit_group to fail, perhaps?
On 2012/06/01 20:07:55, Chris Evans wrote: > Here are some quick comments. > > This looks like a good base we can build on. > > In terms of follow-up CLs, I'd like APIs such as: > - permitBase() (or something; permits things like sigreturn etc. that the Linux > ABI might otherwise surprise you with. maybe also exit() and similar always-used > things) > - permitMalloc() or permitCommon(), possibly rolled into permitBase()? > - permitSafe(): allows all the genuinely trivial things that no-one is worried > about. getpid, getuid, etc. > - denyFileSystemAccess(): ENOENT for any syscall dealing with filenames. Yes, those are all things I want to land as soon as this base is in. Let's do the API switch first and then I'll land those (I have some of those things ready already). Julien
I think we are starting to reach the point of diminishing returns. But I do appreciate your thorough review and there were in fact a handful of issues that I am very glad we fixed. PTAL and see inlined comments below. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:30: status_ = STATUS_UNSUPPORTED; On 2012/06/01 20:07:55, Chris Evans wrote: > Should we have a specific status for that? As a user of the API, I'd probably > read "unsupported" to mean the kernel support isn't there, not that my program > (or point of sandbox initialization) can be tweaked to get it working. I see your point. While this has never come up in practice before, now that we are putting sandboxes in more places, this could make sense. I changed tracking of "status_" to be more complete. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:33: sigfillset(&newMask); On 2012/06/01 20:07:55, Chris Evans wrote: > Although this realistically isn't going to fail, sigfillset() does have a return > value, so it should be checked. Done. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:35: pid_t pid = fork(); On 2012/06/01 20:07:55, Chris Evans wrote: > I wonder what the overhead is of this check? We'll be doing it for every new > renderer process creation, probably. That's why the result is cached in status_. You should only ever do it once. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:44: syscall(__NR_exit_group, (intptr_t)100); On 2012/06/01 20:07:55, Chris Evans wrote: > Why use syscall(exit_group) here but _exit() below? Seems inconsistent. Good call. This was a mistake when I changed the code from what I had before (using direct calls to prctl()) to using the same infrastructure as when starting the "real" sandbox. It probably would still have worked just fine; but the inconsistency makes it harder to reason about correctness. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:46: for (;;) { On 2012/06/01 20:07:55, Chris Evans wrote: > Why is the infinite loop needed? We really absolutely do not want to fall through. Crazy bad things could happen. Our attempts at exiting the program should work. But since we just installed a system call filter, there is the possibility that the system call actually returned. If that's the case, our next best option is to loop. Hopefully, somebody is going to notice... https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:52: HANDLE_EINTR(waitpid(pid, &status, 0)); On 2012/06/01 20:07:55, Chris Evans wrote: > Does this look at status without checking that waitpid returned a non-error code > return? We really should never see an error here. But you are correct that we should be checking. For now, I return STATUS_UNAVAILABLE (signalling a temporary failure). But I could easily be convinced to turn this into a fatal error, as a failure of this system call is completely unexpected. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:55: } else { On 2012/06/01 20:07:55, Chris Evans wrote: > Is this branch the case that fork() fails? Is it your intention to leave status_ > at STATUS_UNSUPPORTED in this case? > Should the fork() failure be logged? Take a look at the new logic and let me know if you like this better. I think it addresses your concerns, but maybe I am misunderstanding what you would like here. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:69: if (status_ == STATUS_UNSUPPORTED) { On 2012/06/01 20:07:55, Chris Evans wrote: > What should we do for STATUS_UNKNOWN? STATUS_UNKNOWN is perfectly OK. This would happen if the user decided to call startSandbox() without first checking whether it is supported. This is explicitly allowed by the API, even if most users wouldn't want to use it this way. I could be persuaded to revisit this design decision, but I am generally comfortable with leaving it this way. So, you'd have to bring a strong counter-argument. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:172: // eventually. On 2012/06/01 20:07:55, Chris Evans wrote: > On 2012/06/01 03:06:18, Jorge Lucangeli Obes wrote: > > Definitely. In particular, we cannot keep "hot" syscalls at the beginning of > the > > jump table without a non-trivial change to this section of the code. I.e. > looks > > like we'll need syscall number iterator or something. > > Agree with Jorge, checking e.g. syscall 200 will be very expensive! But we can > fix it in a follow-up CL. > > BTW, I was very excited about the tree-like comparison thing, to get O(log N) > check time with number of permitted syscalls? Will that make a return? That's the plan. In my first attempt at writing a BPF compiler, I only used the binary tree for making yes/no decisions. But any system call that required more elaborate special-casing (e.g. inspection of arguments) still needed to be handled by hand-coded linear search. Having learned from this prototype, I now realize it is actually really easy to always to a binary search for all decisions. So, we'll get a BPF program that is close to optimal. And since we now have a clear way to define what behavior we want the BPF filter to do, we can also perform a much better run-time check to verify that the compiled BPF program is in fact correct. That makes me very happy :-) No more risk of sneaking in subtle bugs because of mistakes in hand-coded recipes. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:187: ret = SECCOMP_RET_ERRNO + err; On 2012/06/01 20:07:55, Chris Evans wrote: > For paranoid, probably check that "err" is within bounds of what can be handled. > (Check the mask?) I like how you are thinking :-) https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:213: die("Unexpected SIGSYS received"); On 2012/06/01 20:07:55, Chris Evans wrote: > I'd also check that nr==SIGSYS Done. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:218: (void *)(intptr_t)-(int)(SECCOMP_RET_DENY & SECCOMP_RET_DATA); On 2012/06/01 20:07:55, Chris Evans wrote: > I don't understand that; may be worth a comment? I added a comment. No idea whether it makes things easier or harder to understand. The complication arises from the fact that we need to return a negative integer value sign extended and cast to a void *. C/C++ makes this difficult and I had subtle bugs in this code before. Each of the casts is actually necessary. Also, note that I use C style instead of C++ style casts. I find C++ style casts hard to read without really contributing much value. But I am usually willing to bow to the style guide. This is an exception. With C++ casts, nobody could figure out what this line does. With C style casts, there is a chance that the intention of the code is decypherable. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... File sandbox/linux/seccomp_bpf/sandbox_bpf.h (right): https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:13: // #include <linux/seccomp.h> On 2012/06/01 20:07:55, Chris Evans wrote: > Remove if it's dead code. It's not dead code. It just doesn't work correctly until all toolchains have been upgraded to include this header file. In a year or two, we want to include the header file from the system. Until then, we are forced to define the symbols ourselves. If we had "autoconfig", we could do this nicer. But I don't think we have support for any such thing in our toolchain. So, this is the best I can do -- I think. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:98: #define Unsupported target platform On 2012/06/01 20:07:55, Chris Evans wrote: > Did you mean #error ? Doh https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:110: (static_cast<int>(sizeof(x)/sizeof(*(x)))) On 2012/06/01 20:07:55, Chris Evans wrote: > A size macro might want to return size_t ? Really. Do I have to? It means a million ugly casts all over the code, instead of in a single header file. But I guess, I lost this fight. Chrome's arraysize() already picked size_t. Sniff. (n.b. right now, we don't actually have any instances of arraysize(), but that going to change soon, I think) https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:148: Constraint *constraint); On 2012/06/01 20:07:55, Chris Evans wrote: > Style: I think Chromium style is to position the pointer next to the type, not > the type name. This is so broken. And it tends to get painful when mixing Chrome code with platform specific code that actually puts the asterisks where the language's syntax description expects it. We already don't use it consistently across Chrome, but if you insist I'll switch it. https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/secco... sandbox/linux/seccomp_bpf/sandbox_bpf.h:160: // The sandbox becomes the newer owner of this file descriptor and will On 2012/06/01 20:07:55, Chris Evans wrote: > Typo: newer -> new ? Done.
Another quick round. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:11: Redundant newline. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:27: Sandbox::SandboxStatus Sandbox::supportsSeccompSandbox(int proc_fd) { Just wanted to note that this API draws its result from two things: one stateless and one stateful. Stateless: does the kernel have the needed support? Stateful: are there too many threads at _this_ exact moment in time? I'll cover the consequence in a separate reply / e-mail. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:28: if (status_ == STATUS_UNKNOWN || status_ == STATUS_UNAVAILABLE) { Style issue: these nested ifs are getting harder to read. I'd recommend early returns? e.g. if (!isSingleThreaded()) return (status_ = STATUS_UNAVAILABLE) perhaps? Then the subsequent indenting doesn't get crazy. Same for the initial status check. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:47: die(NULL); die() calls strlen() on the pointer so seems like this will crash. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:53: status_ = HANDLE_EINTR(waitpid(pid, &status, 0)) != pid Nested ternarys are a real PITA to read / parse, can we make it clearer? https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:58: } else if (sigprocmask(SIG_SETMASK, &oldMask, NULL)) { It looks like this branch is associated with fork() failing? It's hard to tell, perhaps another argument for a "return early" indenting style as made above. If this were indeed the association, is it what you intended? https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:65: status_ = STATUS_UNAVAILABLE; Still would prefer a dedicated error, STATUS_BADTHREADINGSITUATIONOMGOMG? :D https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:232: die("Unexpected SIGSYS received"); Bug: die() in the Chromium context uses LOG(), which I'm pretty sure won't be safe in the signal context. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:235: int old_errno = errno; We don't seem to do anything with old_errno? https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.h (right): https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.h:212: _exit(1); Again, seems strange to alternate between syscall(exit) and exit() in such close proximity.
On Fri, Jun 1, 2012 at 2:38 PM, <markus@chromium.org> wrote: > I think we are starting to reach the point of diminishing returns. But I do > appreciate your thorough review and there were in fact a handful of issues > that > I am very glad we fixed. > > PTAL and see inlined comments below. > > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.cc (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode30<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode30> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:30: status_ = > STATUS_UNSUPPORTED; > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Should we have a specific status for that? As a user of the API, I'd >> > probably > >> read "unsupported" to mean the kernel support isn't there, not that my >> > program > >> (or point of sandbox initialization) can be tweaked to get it working. >> > > I see your point. While this has never come up in practice before, now > that we are putting sandboxes in more places, this could make sense. > > I changed tracking of "status_" to be more complete. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode33<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode33> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:33: sigfillset(&newMask); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Although this realistically isn't going to fail, sigfillset() does >> > have a return > >> value, so it should be checked. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode35<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode35> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:35: pid_t pid = fork(); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> I wonder what the overhead is of this check? We'll be doing it for >> > every new > >> renderer process creation, probably. >> > > That's why the result is cached in status_. You should only ever do it > once. You do it once per process, on account of the use of a "static", right? Unless you do the check in the _zygote_ process, each new fork()ed renderer will have its own copy of the static which will start off "unknown". However you can't use that API in the Zygote process to "warm up" the subsystem because the API mixes stateless (kernel support?) vs. stateful (current threading situation?) -- I added a note about the statefulness in another round of comments I just added. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode44<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode44> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:44: syscall(__NR_exit_group, > (intptr_t)100); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Why use syscall(exit_group) here but _exit() below? Seems >> > inconsistent. > > Good call. This was a mistake when I changed the code from what I had > before (using direct calls to prctl()) to using the same infrastructure > as when starting the "real" sandbox. > > It probably would still have worked just fine; but the inconsistency > makes it harder to reason about correctness. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode46<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode46> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:46: for (;;) { > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Why is the infinite loop needed? >> > > We really absolutely do not want to fall through. Crazy bad things could > happen. Our attempts at exiting the program should work. But since we > just installed a system call filter, there is the possibility that the > system call actually returned. If that's the case, our next best option > is to loop. Hopefully, somebody is going to notice... Fair enough :) > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode52<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode52> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:52: HANDLE_EINTR(waitpid(pid, > &status, 0)); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Does this look at status without checking that waitpid returned a >> > non-error code > >> return? >> > > We really should never see an error here. But you are correct that we > should be checking. For now, I return STATUS_UNAVAILABLE (signalling a > temporary failure). But I could easily be convinced to turn this into a > fatal error, as a failure of this system call is completely unexpected. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode55<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode55> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:55: } else { > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Is this branch the case that fork() fails? Is it your intention to >> > leave status_ > >> at STATUS_UNSUPPORTED in this case? >> Should the fork() failure be logged? >> > > Take a look at the new logic and let me know if you like this better. I > think it addresses your concerns, but maybe I am misunderstanding what > you would like here. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode69<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode69> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:69: if (status_ == > STATUS_UNSUPPORTED) { > On 2012/06/01 20:07:55, Chris Evans wrote: > >> What should we do for STATUS_UNKNOWN? >> > > STATUS_UNKNOWN is perfectly OK. This would happen if the user decided to > call startSandbox() without first checking whether it is supported. > > This is explicitly allowed by the API, even if most users wouldn't want > to use it this way. > > I could be persuaded to revisit this design decision, but I am generally > comfortable with leaving it this way. So, you'd have to bring a strong > counter-argument. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode172<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode172> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:172: // eventually. > On 2012/06/01 20:07:55, Chris Evans wrote: > >> On 2012/06/01 03:06:18, Jorge Lucangeli Obes wrote: >> > Definitely. In particular, we cannot keep "hot" syscalls at the >> > beginning of > >> the >> > jump table without a non-trivial change to this section of the code. >> > I.e. > >> looks >> > like we'll need syscall number iterator or something. >> > > Agree with Jorge, checking e.g. syscall 200 will be very expensive! >> > But we can > >> fix it in a follow-up CL. >> > > BTW, I was very excited about the tree-like comparison thing, to get >> > O(log N) > >> check time with number of permitted syscalls? Will that make a return? >> > > That's the plan. In my first attempt at writing a BPF compiler, I only > used the binary tree for making yes/no decisions. But any system call > that required more elaborate special-casing (e.g. inspection of > arguments) still needed to be handled by hand-coded linear search. > > Having learned from this prototype, I now realize it is actually really > easy to always to a binary search for all decisions. So, we'll get a BPF > program that is close to optimal. > > And since we now have a clear way to define what behavior we want the > BPF filter to do, we can also perform a much better run-time check to > verify that the compiled BPF program is in fact correct. That makes me > very happy :-) No more risk of sneaking in subtle bugs because of > mistakes in hand-coded recipes. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode187<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode187> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:187: ret = SECCOMP_RET_ERRNO + > err; > On 2012/06/01 20:07:55, Chris Evans wrote: > >> For paranoid, probably check that "err" is within bounds of what can >> > be handled. > >> (Check the mask?) >> > > I like how you are thinking :-) > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode213<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode213> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:213: die("Unexpected SIGSYS > received"); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> I'd also check that nr==SIGSYS >> > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode218<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode218> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:218: (void > *)(intptr_t)-(int)(SECCOMP_**RET_DENY & SECCOMP_RET_DATA); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> I don't understand that; may be worth a comment? >> > > I added a comment. No idea whether it makes things easier or harder to > understand. The complication arises from the fact that we need to return > a negative integer value sign extended and cast to a void *. C/C++ makes > this difficult and I had subtle bugs in this code before. Each of the > casts is actually necessary. > > Also, note that I use C style instead of C++ style casts. I find C++ > style casts hard to read without really contributing much value. But I > am usually willing to bow to the style guide. > > This is an exception. With C++ casts, nobody could figure out what this > line does. With C style casts, there is a chance that the intention of > the code is decypherable. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.h (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode13<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode13> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:13: // #include > <linux/seccomp.h> > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Remove if it's dead code. >> > > It's not dead code. It just doesn't work correctly until all toolchains > have been upgraded to include this header file. > > In a year or two, we want to include the header file from the system. > Until then, we are forced to define the symbols ourselves. > > If we had "autoconfig", we could do this nicer. But I don't think we > have support for any such thing in our toolchain. So, this is the best I > can do -- I think. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode98<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode98> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:98: #define Unsupported target > platform > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Did you mean #error ? >> > > Doh > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode110<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode110> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:110: > (static_cast<int>(sizeof(x)/**sizeof(*(x)))) > On 2012/06/01 20:07:55, Chris Evans wrote: > >> A size macro might want to return size_t ? >> > > Really. Do I have to? It means a million ugly casts all over the code, > instead of in a single header file. > > But I guess, I lost this fight. Chrome's arraysize() already picked > size_t. Sniff. > > (n.b. right now, we don't actually have any instances of arraysize(), > but that going to change soon, I think) > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode148<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode148> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:148: Constraint *constraint); > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Style: I think Chromium style is to position the pointer next to the >> > type, not > >> the type name. >> > > This is so broken. And it tends to get painful when mixing Chrome code > with platform specific code that actually puts the asterisks where the > language's syntax description expects it. > > We already don't use it consistently across Chrome, but if you insist > I'll switch it. Your call if you want to leave it as-is. It's in its own separate directory so no insisting. Cheers Chris > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 8004/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode160<https://chromiumcodereview.appspot.com/10458040/diff/8004/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode160> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:160: // The sandbox becomes the > newer owner of this file descriptor and will > On 2012/06/01 20:07:55, Chris Evans wrote: > >> Typo: newer -> new ? >> > > Done. > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... >
On Fri, Jun 1, 2012 at 3:52 PM, Chris Evans <cevans@chromium.org> wrote: > You do it once per process, on account of the use of a "static", right? > > Unless you do the check in the _zygote_ process, each new fork()ed > renderer will have its own copy of the static which will start off > "unknown". > > However you can't use that API in the Zygote process to "warm up" the > subsystem because the API mixes stateless (kernel support?) vs. stateful > (current threading situation?) -- I added a note about the statefulness in > another round of comments I just added. > I think this is actually correct. Can you take another look and let me know, if I am maybe misunderstanding you (or I introduced a bug that I don't see)? The initial state is "unknown". The zygote can "warm up" the subsystem and perform the expensive check (i.e. fork() and installing of a syscall filter). It then sets the state to "unsupported" (kernel support is missing, this can never work), "available" (everything is hunky dory), or "unavailable" (the kernel supports it, but the current run-time environment is incompatible). Any future calls to supportsSeccompSandbox() will skip the expensive checks, but they will still perform the cheap check for whether the process is single or multi threaded. So, you do in fact always get the correct return code, and you can warm up the subsystem. Or at least that's my intention. If I introduced a bug, please speak up. Markus
On Fri, Jun 1, 2012 at 4:00 PM, Markus Gutschke <markus@chromium.org> wrote: > On Fri, Jun 1, 2012 at 3:52 PM, Chris Evans <cevans@chromium.org> wrote: > >> You do it once per process, on account of the use of a "static", right? >> >> Unless you do the check in the _zygote_ process, each new fork()ed >> renderer will have its own copy of the static which will start off >> "unknown". >> >> However you can't use that API in the Zygote process to "warm up" the >> subsystem because the API mixes stateless (kernel support?) vs. stateful >> (current threading situation?) -- I added a note about the statefulness in >> another round of comments I just added. >> > > I think this is actually correct. Can you take another look and let me > know, if I am maybe misunderstanding you (or I introduced a bug that I > don't see)? > > The initial state is "unknown". The zygote can "warm up" the subsystem and > perform the expensive check (i.e. fork() and installing of a syscall > filter). It then sets the state to "unsupported" (kernel support is > missing, this can never work), "available" (everything is hunky dory), or > "unavailable" (the kernel supports it, but the current run-time environment > is incompatible). > Ok. This will work as long as the zygote isn't multi-threaded. I don't know the answer to that. I didn't know you already had zygote pre-warming in mind, hence the disconnect. > > Any future calls to supportsSeccompSandbox() will skip the expensive > checks, but they will still perform the cheap check for whether the process > is single or multi threaded. > > So, you do in fact always get the correct return code, and you can warm up > the subsystem. Or at least that's my intention. If I introduced a bug, > please speak up. > > > Markus >
This can be made to work with a multi-threaded zygote, but it will make the logic more complicated. For now, I don't bother. But I added a lot more comments and restructured the "if" statements. That should make things more readable, addressing your original concern. And it also makes it clear what needs to be changed, if we are already multi-threaded by the time that we pre-warm. If you want me to address this special case right now, I'd be happy to do so; but I think it requires adding one more state to track -- thus making the code a little more complicated to review. Your call. Markus On Fri, Jun 1, 2012 at 4:05 PM, Chris Evans <cevans@google.com> wrote: > On Fri, Jun 1, 2012 at 4:00 PM, Markus Gutschke <markus@chromium.org>wrote: > >> On Fri, Jun 1, 2012 at 3:52 PM, Chris Evans <cevans@chromium.org> wrote: >> >>> You do it once per process, on account of the use of a "static", right? >>> >>> Unless you do the check in the _zygote_ process, each new fork()ed >>> renderer will have its own copy of the static which will start off >>> "unknown". >>> >>> However you can't use that API in the Zygote process to "warm up" the >>> subsystem because the API mixes stateless (kernel support?) vs. stateful >>> (current threading situation?) -- I added a note about the statefulness in >>> another round of comments I just added. >>> >> >> I think this is actually correct. Can you take another look and let me >> know, if I am maybe misunderstanding you (or I introduced a bug that I >> don't see)? >> >> The initial state is "unknown". The zygote can "warm up" the subsystem >> and perform the expensive check (i.e. fork() and installing of a syscall >> filter). It then sets the state to "unsupported" (kernel support is >> missing, this can never work), "available" (everything is hunky dory), or >> "unavailable" (the kernel supports it, but the current run-time environment >> is incompatible). >> > > Ok. This will work as long as the zygote isn't multi-threaded. I don't > know the answer to that. > I didn't know you already had zygote pre-warming in mind, hence the > disconnect. > > >> >> Any future calls to supportsSeccompSandbox() will skip the expensive >> checks, but they will still perform the cheap check for whether the process >> is single or multi threaded. >> >> So, you do in fact always get the correct return code, and you can warm >> up the subsystem. Or at least that's my intention. If I introduced a bug, >> please speak up. >> >> >> Markus >> > >
https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:11: On 2012/06/01 22:48:23, Chris Evans wrote: > Redundant newline. Done. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:27: Sandbox::SandboxStatus Sandbox::supportsSeccompSandbox(int proc_fd) { I think, I explained the rationale in my other e-mail. If you think, this is still unresolved, please let me know. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:28: if (status_ == STATUS_UNKNOWN || status_ == STATUS_UNAVAILABLE) { On 2012/06/01 22:48:23, Chris Evans wrote: > Style issue: these nested ifs are getting harder to read. I'd recommend early > returns? e.g. if (!isSingleThreaded()) return (status_ = STATUS_UNAVAILABLE) > perhaps? Then the subsequent indenting doesn't get crazy. Same for the initial > status check. Done. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:47: die(NULL); On 2012/06/01 22:48:23, Chris Evans wrote: > die() calls strlen() on the pointer so seems like this will crash. Good call. die() was already safe, when used in stand-alone mode. But it would in fact have crashed, when using Chrome's logging feature. I fixed die() to do the right thing. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:53: status_ = HANDLE_EINTR(waitpid(pid, &status, 0)) != pid On 2012/06/01 22:48:23, Chris Evans wrote: > Nested ternarys are a real PITA to read / parse, can we make it clearer? Done. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:58: } else if (sigprocmask(SIG_SETMASK, &oldMask, NULL)) { On 2012/06/01 22:48:23, Chris Evans wrote: > It looks like this branch is associated with fork() failing? It's hard to tell, > perhaps another argument for a "return early" indenting style as made above. > > If this were indeed the association, is it what you intended? Done. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:65: status_ = STATUS_UNAVAILABLE; On 2012/06/01 22:48:23, Chris Evans wrote: > Still would prefer a dedicated error, STATUS_BADTHREADINGSITUATIONOMGOMG? :D I don't really want the caller to have to know too much about the internals of the sandbox. There could be a lot of other run-time failure cases in the future, and I don't want callers to have to enumerate all of them, when deciding what to do with the return code from supportSeccompSandbox(). This is already somewhat difficult to use correctly. I wouldn't be opposed to more logging, if that's what we want to do. If so, please suggest what additional logging is needed. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:232: die("Unexpected SIGSYS received"); On 2012/06/01 22:48:23, Chris Evans wrote: > Bug: die() in the Chromium context uses LOG(), which I'm pretty sure won't be > safe in the signal context. This is really a tough call. The standalone version of die() is designed to be safe. But the Chrome version is likely not safe ... although hopefully safe enough, as this is fortunately a synchronous signal, making it a little less scary. Of course, if somebody just calls "kill -SIGSYS" from the command line, all bets are off. So, I certainly understand your concerns. But Julien correctly pointed out that we really want to benefit from Chrome's logging (and UMA reporting) capabilities if we fail anywhere in the sandbox. And I think that means we have to use the standard logging functions. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:235: int old_errno = errno; On 2012/06/01 22:48:23, Chris Evans wrote: > We don't seem to do anything with old_errno? That is correct. Non-exiting signal handlers must manually preserve errno or you get really subtle bugs. Of course, with this version of the CL, we don't actually do anything yet. So, we are not destroying "errno". But that's presumably going to happen once the user of the sandbox can provide its own signal handling code. https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.h (right): https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.h:212: _exit(1); On 2012/06/01 22:48:23, Chris Evans wrote: > Again, seems strange to alternate between syscall(exit) and exit() in such close > proximity. It's a last-ditch effort. If for some reason our direct system call didn't work, maybe glibc knows some trick we don't know. Realistically, I can't really see a situation where the system call would fail, but glibc would succeed. So, if you prefer, I'll take out the call into glibc.
On Fri, Jun 1, 2012 at 4:46 PM, <markus@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.cc (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode11<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode11> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:11: > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Redundant newline. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode27<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode27> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:27: Sandbox::SandboxStatus > Sandbox::**supportsSeccompSandbox(int proc_fd) { > I think, I explained the rationale in my other e-mail. If you think, > this is still unresolved, please let me know. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode28<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode28> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:28: if (status_ == > STATUS_UNKNOWN || status_ == STATUS_UNAVAILABLE) { > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Style issue: these nested ifs are getting harder to read. I'd >> > recommend early > >> returns? e.g. if (!isSingleThreaded()) return (status_ = >> > STATUS_UNAVAILABLE) > >> perhaps? Then the subsequent indenting doesn't get crazy. Same for the >> > initial > >> status check. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode47<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode47> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:47: die(NULL); > On 2012/06/01 22:48:23, Chris Evans wrote: > >> die() calls strlen() on the pointer so seems like this will crash. >> > > Good call. die() was already safe, when used in stand-alone mode. But it > would in fact have crashed, when using Chrome's logging feature. I fixed > die() to do the right thing. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode53<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode53> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:53: status_ = > HANDLE_EINTR(waitpid(pid, &status, 0)) != pid > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Nested ternarys are a real PITA to read / parse, can we make it >> > clearer? > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode58<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode58> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:58: } else if > (sigprocmask(SIG_SETMASK, &oldMask, NULL)) { > On 2012/06/01 22:48:23, Chris Evans wrote: > >> It looks like this branch is associated with fork() failing? It's hard >> > to tell, > >> perhaps another argument for a "return early" indenting style as made >> > above. > > If this were indeed the association, is it what you intended? >> > > Done. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode65<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode65> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:65: status_ = > STATUS_UNAVAILABLE; > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Still would prefer a dedicated error, >> > STATUS_**BADTHREADINGSITUATIONOMGOMG? :D > > I don't really want the caller to have to know too much about the > internals of the sandbox. From the callers case, it does seem to me like there are at least three important cases: - It worked! YAY - It didn't work because the current operating environment is too old - It didn't work because _YOU_ (the caller) messed up (your threading) WDYT? There could be a lot of other run-time failure > cases in the future, and I don't want callers to have to enumerate all > of them, when deciding what to do with the return code from > supportSeccompSandbox(). > > This is already somewhat difficult to use correctly. > > I wouldn't be opposed to more logging, if that's what we want to do. If > so, please suggest what additional logging is needed. > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode232<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode232> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:232: die("Unexpected SIGSYS > received"); > > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Bug: die() in the Chromium context uses LOG(), which I'm pretty sure >> > won't be > >> safe in the signal context. >> > > This is really a tough call. The standalone version of die() is designed > to be safe. > > But the Chrome version is likely not safe ... although hopefully safe > enough, as this is fortunately a synchronous signal, making it a little > less scary. Of course, if somebody just calls "kill -SIGSYS" from the > command line, all bets are off. So, I certainly understand your > concerns. > > But Julien correctly pointed out that we really want to benefit from > Chrome's logging (and UMA reporting) capabilities if we fail anywhere in > the sandbox. And I think that means we have to use the standard logging > functions. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.cc#newcode235<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.cc#newcode235> > sandbox/linux/seccomp_bpf/**sandbox_bpf.cc:235: int old_errno = errno; > On 2012/06/01 22:48:23, Chris Evans wrote: > >> We don't seem to do anything with old_errno? >> > > That is correct. > > Non-exiting signal handlers must manually preserve errno or you get > really subtle bugs. > > Of course, with this version of the CL, we don't actually do anything > yet. So, we are not destroying "errno". But that's presumably going to > happen once the user of the sandbox can provide its own signal handling > code. > > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.h<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.h> > File sandbox/linux/seccomp_bpf/**sandbox_bpf.h (right): > > https://chromiumcodereview.**appspot.com/10458040/diff/** > 18001/sandbox/linux/seccomp_**bpf/sandbox_bpf.h#newcode212<https://chromiumcodereview.appspot.com/10458040/diff/18001/sandbox/linux/seccomp_bpf/sandbox_bpf.h#newcode212> > sandbox/linux/seccomp_bpf/**sandbox_bpf.h:212: _exit(1); > On 2012/06/01 22:48:23, Chris Evans wrote: > >> Again, seems strange to alternate between syscall(exit) and exit() in >> > such close > >> proximity. >> > > It's a last-ditch effort. If for some reason our direct system call > didn't work, maybe glibc knows some trick we don't know. Realistically, > I can't really see a situation where the system call would fail, but > glibc would succeed. > > So, if you prefer, I'll take out the call into glibc. > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... >
On Fri, Jun 1, 2012 at 8:21 PM, Chris Evans <cevans@google.com> wrote: > From the callers case, it does seem to me like there are at least three > important cases: > I believe we are in violent agreement :-) > - It worked! YAY > STATUS_AVAILABLE > - It didn't work because the current operating environment is too old > STATUS_UNSUPPORTED > - It didn't work because _YOU_ (the caller) messed up (your threading) > STATUS_UNAVAILABLE In addition there is: - You never asked: STATUS_UNKNOWN (this should never be user-visible) - You already turned on sandboxing, dummy: STATUS_ENABLED Markus
I still need an official LGTM. Is there anything that is holding it up? If so, what do you still need me to do? On Fri, Jun 1, 2012 at 8:45 PM, Markus Gutschke <markus@chromium.org> wrote: > On Fri, Jun 1, 2012 at 8:21 PM, Chris Evans <cevans@google.com> wrote: > >> From the callers case, it does seem to me like there are at least three >> important cases: >> > > I believe we are in violent agreement :-) > > >> - It worked! YAY >> > > STATUS_AVAILABLE > > >> - It didn't work because the current operating environment is too old >> > > STATUS_UNSUPPORTED > > >> - It didn't work because _YOU_ (the caller) messed up (your threading) >> > > STATUS_UNAVAILABLE > > In addition there is: > > - You never asked: STATUS_UNKNOWN (this should never be user-visible) > > - You already turned on sandboxing, dummy: STATUS_ENABLED > > > Markus >
On Mon, Jun 4, 2012 at 10:52 AM, Markus Gutschke <markus@chromium.org>wrote: > I still need an official LGTM. Is there anything that is holding it up? If > so, what do you still need me to do? I will grab a coffee and take a final look right now :) > > > On Fri, Jun 1, 2012 at 8:45 PM, Markus Gutschke <markus@chromium.org>wrote: > >> On Fri, Jun 1, 2012 at 8:21 PM, Chris Evans <cevans@google.com> wrote: >> >>> From the callers case, it does seem to me like there are at least three >>> important cases: >>> >> >> I believe we are in violent agreement :-) >> >> >>> - It worked! YAY >>> >> >> STATUS_AVAILABLE >> >> >>> - It didn't work because the current operating environment is too old >>> >> >> STATUS_UNSUPPORTED >> >> >>> - It didn't work because _YOU_ (the caller) messed up (your threading) >>> >> >> STATUS_UNAVAILABLE >> >> In addition there is: >> >> - You never asked: STATUS_UNKNOWN (this should never be user-visible) >> >> - You already turned on sandboxing, dummy: STATUS_ENABLED >> >> >> Markus >> > >
https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:48: isSingleThreaded(proc_fd)) { Don't we go straight to STATUS_AVAILABLE at this time? Seems like this case might be the one we discussed in e-mail: the zygote was multi-threaded at the time of the warm-up and now we're in a single-threaded renderer and wish to avoid the main overhead of the check. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:55: if (status_ == STATUS_UNKNOWN) { This whole function is looking much clearer now, and to finish the look, how about moving this chunk to a separate function, e.g. "kernelSupportsSeccompFilters" or some such? https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:58: !sigprocmask(SIG_BLOCK, &newMask, &oldMask)) { This is another place where you can avoid the indentation, especially if you've broken this piece into another function. e.g. if (sig... || sig...) return STATUS_BLAH; Actually, it looks like you might want to die(), to be symmetrical with other cases lower down. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:71: die(NULL); I missed what we planned to do about the die(NULL)? https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:78: status_ = STATUS_UNAVAILABLE; Maybe use die() here? We know we forked just a single child so this we can expect it to work a bit more forcefully. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:151: if (proc_fd < 0 || Don't need to check proc_fd, you did it just above. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:265: die("Unexpected SIGSYS received"); I still don't think LOG(FATAL) (one particular expansion of die()) is async-signal-safe. At a minimum, I think it deserves an apologetic comment :) However, these really are should-not-ever-happen conditions so I'd be supportive of simply _exit(1) if you wanted.
PTAL https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:48: isSingleThreaded(proc_fd)) { OK, I changed the code. This is a little fragile, if somebody ever changes the state transitions. So, I put a big comment in there. Hopefully, that's good enough. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:55: if (status_ == STATUS_UNKNOWN) { On 2012/06/04 19:17:26, Chris Evans wrote: > This whole function is looking much clearer now, and to finish the look, how > about moving this chunk to a separate function, e.g. > "kernelSupportsSeccompFilters" or some such? Done. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:58: !sigprocmask(SIG_BLOCK, &newMask, &oldMask)) { On 2012/06/04 19:17:26, Chris Evans wrote: > This is another place where you can avoid the indentation, especially if you've > broken this piece into another function. e.g. if (sig... || sig...) return > STATUS_BLAH; > > Actually, it looks like you might want to die(), to be symmetrical with other > cases lower down. Done. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:71: die(NULL); On 2012/06/04 19:17:26, Chris Evans wrote: > I missed what we planned to do about the die(NULL)? die(NULL) is explicitly safe and support. For all practical purposes, think of it as our version of _exit(). https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:78: status_ = STATUS_UNAVAILABLE; On 2012/06/04 19:17:26, Chris Evans wrote: > Maybe use die() here? We know we forked just a single child so this we can > expect it to work a bit more forcefully. Done. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:85: // STATUS_UNSUPPORTED. In light of all the other changes, I felt if safer to now die() whenever fork() fails. Going back and forth with Julien on this decision, we feel that it probably is saner and easier to review that way. I added a new comment to explain our reasoning. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:151: if (proc_fd < 0 || On 2012/06/04 19:17:26, Chris Evans wrote: > Don't need to check proc_fd, you did it just above. Done. https://chromiumcodereview.appspot.com/10458040/diff/20003/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:265: die("Unexpected SIGSYS received"); On 2012/06/04 19:17:26, Chris Evans wrote: > I still don't think LOG(FATAL) (one particular expansion of die()) is > async-signal-safe. > > At a minimum, I think it deserves an apologetic comment :) However, these really > are should-not-ever-happen conditions so I'd be supportive of simply _exit(1) if > you wanted. I hear you -- you are really preaching to the choir, and I am usually the one who keeps yelling at people for not writing async-safe code. No, I am not very happy with LOG(FATAL). But Julien very correctly pointed out that it is the best we have right now, if we want proper logging and reporting. And I am stuck between getting the LGTM from you or getting the OK from Julien... In the long run, we probably need to come up with something a little better. But that'll introduce some crazy code complexity that I don't want to touch at this point (e.g. we could have a ring-buffer that is periodically checked by another thread; of course, when we are about to die, we have no guarantee that the other thread still runs; so, we'd have to think of some best-effort approach that is likely to succeed most of the time).
lgtm https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:232: // eventually. We'll probably want to fix this (to at least avoid walking all the syscalls every time) before switching from the previous implementation. It's great that it's already planned though =). I don't think it should block submitting though.
BTW are you planning on fixing the (valid) lint errors?
LGTM with 3 nits. Action the nits and I don't need to re-review. Thanks :D https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... File sandbox/linux/seccomp_bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:203: sigaddset(&mask, SIGSYS); Nit: we took the trouble to check the sigfillset() return value above, so we should probably check these too. https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:232: // eventually. Nit: note the actual run time in a comment. Julien and I both (initially) had a different impression so it'd be great to add a comment (and keep that comment in sync as we improve things). Looks like current run time for evaluating system call N is N? (Current run time in sandbox_init_linux.cc is more proportional to the number of syscalls _permitted_, call this P, and best we can do without BPF enhancements is probably log2(P) or so) https://chromiumcodereview.appspot.com/10458040/diff/34001/sandbox/linux/secc... sandbox/linux/seccomp_bpf/sandbox_bpf.cc:279: if (nr != SIGSYS || info->si_code != SYS_SECCOMP || !void_context) { Nit: add a comment that die() might call LOG(FATAL), which might non be async safe. One final idea: if we're worried about making sure we see a distinctive log here, why not just dereference 0x1 or some such <1 page constant? It would result in a very clearly readable crash report.
On 2012/06/04 22:17:47, Jorge Lucangeli Obes wrote: > BTW are you planning on fixing the (valid) lint errors? Which ones are you thinking of in particular? I fixed the C-style casts, except for one case where they seemed really counter-productive. The errors about missing NL at end of file seem incorrect, as far as I can tell. The NL is there. There are a handful of other lint errors that are clearly not appropriate to this code. There are two cases of missing spaces; I'll upload a new patch with these changes. There are a couple of complaints about asterisks being in the wrong place. If you really insist on it, I can fix that. But it is annoying in low-level code as it keep conflicting with the conventions used by everybody else -- especially by kernel headers that we need to reference. What else is there?
On 2012/06/04 22:24:43, Markus (顧孟勤) wrote: > On 2012/06/04 22:17:47, Jorge Lucangeli Obes wrote: > > BTW are you planning on fixing the (valid) lint errors? > > Which ones are you thinking of in particular? > > I fixed the C-style casts, except for one case where they seemed really > counter-productive. > > The errors about missing NL at end of file seem incorrect, as far as I can tell. > The NL is there. Cool =) > There are a handful of other lint errors that are clearly not appropriate to > this code. Agreed. > There are two cases of missing spaces; I'll upload a new patch with these > changes. Perfect. > There are a couple of complaints about asterisks being in the wrong place. If > you really insist on it, I can fix that. But it is annoying in low-level code as > it keep conflicting with the conventions used by everybody else -- especially by > kernel headers that we need to reference. I don't really care about those. > What else is there? Was mostly thinking about the newlines and spaces. So we're good. Thanks!
Whew. It's committed. Open the floodgates :-) Now, jln and everybody else can send CLs too... On Mon, Jun 4, 2012 at 3:27 PM, <jorgelo@chromium.org> wrote: > On 2012/06/04 22:24:43, Markus (顧孟勤) wrote: > >> On 2012/06/04 22:17:47, Jorge Lucangeli Obes wrote: >> > BTW are you planning on fixing the (valid) lint errors? >> > > Which ones are you thinking of in particular? >> > > I fixed the C-style casts, except for one case where they seemed really >> counter-productive. >> > > The errors about missing NL at end of file seem incorrect, as far as I can >> > tell. > >> The NL is there. >> > Cool =) > > There are a handful of other lint errors that are clearly not appropriate >> to >> this code. >> > Agreed. > > There are two cases of missing spaces; I'll upload a new patch with these >> changes. >> > Perfect. > > There are a couple of complaints about asterisks being in the wrong >> place. If >> you really insist on it, I can fix that. But it is annoying in low-level >> code >> > as > >> it keep conflicting with the conventions used by everybody else -- >> especially >> > by > >> kernel headers that we need to reference. >> > I don't really care about those. > >> What else is there? >> > > Was mostly thinking about the newlines and spaces. So we're good. Thanks! > > https://chromiumcodereview.**appspot.com/10458040/<https://chromiumcodereview... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
