Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158793003:
Enable one PID namespace per process for NaCl processes. (Closed)
|
Created:
5 years, 7 months ago by rickyz (no longer on Chrome) Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, rickyz+watch_chromium.org, jln+watch_chromium.org, hidehiko Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable one PID namespace per process for NaCl processes.
This CL does two things:
- Make the NaCl helper fork each process into a new PID namespace via
ForkInNewPidNamespace.
- Bring the non-NaCl process termination exit codes in line with NaCl's
default signal handlers, that is exit with an exit code of -sig &
0xff.
This change depends on https://codereview.chromium.org/1159803003, which
adds termination signals handlers in SFI NaCl.
BUG=460972
Committed: https://crrev.com/d423ad87b1fdc8a59a7c945c000bfbbd9d8b2245
Cr-Commit-Position: refs/heads/master@{#335223}
Patch Set 1 #Patch Set 2 : Get rid of kDefaultExitCode. #
Total comments: 14
Patch Set 3 : Respond to comments. #
Total comments: 2
Patch Set 4 : Respond to comments. #Patch Set 5 : Split out nonSFI bits. #
Total comments: 8
Patch Set 6 : More comments. #
Total comments: 7
Patch Set 7 : Install signal handlers for nonsfi mode. #Patch Set 8 : Fix embarassing compile fail. #
Total comments: 1
Patch Set 9 : Enable PID namespace per process for nonsfi newlib NaCl as well. #Messages
Total messages: 48 (15 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org, mseaborn@chromium.org
Here's the chromium side of the change. Should only be submitted after https://codereview.chromium.org/1159803003/ is submitted and rolled.
jln@chromium.org changed reviewers: + mdempsky@chromium.org
Looks ok in general. Added mdempsky@ since I'm OOO. - How come NONSFI doesn't support the namespace class yet? I thought this was done. - For the cases where we don't drop CAP_SYS_ADMIN, we need to have a way to guarantee that it will be dropped at some point. Dropping it when engaging the layer2 sandbox (or in SealSandbox() when layer2 is not engaged) seems like it could work, but it feels a little like spaghetti. thoughts? https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... File components/nacl/loader/nacl_helper_linux.cc (right): https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... components/nacl/loader/nacl_helper_linux.cc:197: if (sandbox::NamespaceSandbox::InNewUserNamespace()) { Hidehiko, Mark, what's missing for this to compile in OS_NACL_NONSFI? One PID per NS is very desirable for NONSFI. https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... components/nacl/loader/nacl_helper_linux.cc:204: child_pid = fork(); We never drop all capabilities in this codepath! https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:143: // class is keeping a file descriptor to /proc/. Doesn't this comment apply to both the setuid sandbox case and the userNS case? https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:147: // process in its own PID namespace later on. This is unfortunate. This should be dropped as part of a mandatory mechanism like SealLayerOneSandbox. Except that the layer 2 sandbox will prevent us from doing it. Perhaps do it as part of engaging the layer 2 sandbox? (And also as part of SealLayerOneSandbox when layer 2 is not engaged). We need to be careful to make all of this clear and not look like spaghetti. https://chromiumcodereview.appspot.com/1158793003/diff/20001/content/zygote/z... File content/zygote/zygote_linux.cc (right): https://chromiumcodereview.appspot.com/1158793003/diff/20001/content/zygote/z... content/zygote/zygote_linux.cc:405: sandbox::NamespaceSandbox::InstallTerminationSignalHandler( Could you think of a way to rename this to indicate that it doesn't do anything if a signal handler is already present? https://chromiumcodereview.appspot.com/1158793003/diff/20001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/1158793003/diff/20001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:21: inline int SignalExitCode(int sig) { Let's qualify this in NamespaceSandbox:: as a static member. sandbox::something should pretty much not exist.
https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... File components/nacl/loader/nacl_helper_linux.cc (right): https://chromiumcodereview.appspot.com/1158793003/diff/20001/components/nacl/... components/nacl/loader/nacl_helper_linux.cc:205: #if !defined(OS_NACL_NONSFI) nit: Having an #if block just for a } is kind of ugly. I'd suggest either reshuffling the "} else { #endif" to "} else #endif {" so you can have the close brace unconditionally, or the more intrusive option of something like namespace { #if defined(OS_NACL_NONSFI) pid_t ForkChild() { return sandbox::NamespaceSandbox::ForkInNewPidNamespace(...); } #else pid_t ForkChild() { return fork(); } #endif } // namespace Your call.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
FYI. https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/nacl_helper_linux.cc:197: if (sandbox::NamespaceSandbox::InNewUserNamespace()) { On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > Hidehiko, Mark, what's missing for this to compile in OS_NACL_NONSFI? > > One PID per NS is very desirable for NONSFI. base::ForkWithFlags in base/process/launch_posix.cc. It is not built in libbase_nacl_nonsfi.a now. Supporting ForkWithFlags *only* would not be much difficult, IIUC. Though, other code in the file would hit problems if we build with PNaCl toolchain, especially around signals, so I just dropped the file at all, as we didn't need it. Also, in ForkInNewPidNamespace, we need to use LINUX_SIGCHLD instead of SIGCHLD for the workaround of signal ABI problem.
https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/nacl_helper_linux.cc:197: if (sandbox::NamespaceSandbox::InNewUserNamespace()) { On 2015/05/29 05:46:58, hidehiko wrote: > On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > > Hidehiko, Mark, what's missing for this to compile in OS_NACL_NONSFI? > > > > One PID per NS is very desirable for NONSFI. > > base::ForkWithFlags in base/process/launch_posix.cc. > It is not built in libbase_nacl_nonsfi.a now. > > Supporting ForkWithFlags *only* would not be much difficult, IIUC. > Though, other code in the file would hit problems if we build with PNaCl > toolchain, especially around signals, so I just dropped the file at all, as we > didn't need it. > > Also, in ForkInNewPidNamespace, we need to use LINUX_SIGCHLD instead of SIGCHLD > for the workaround of signal ABI problem. FYI: Like this https://codereview.chromium.org/1161933003/
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/nacl_helper_linux.cc:197: if (sandbox::NamespaceSandbox::InNewUserNamespace()) { On 2015/05/29 06:14:52, hidehiko wrote: > On 2015/05/29 05:46:58, hidehiko wrote: > > On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > > > Hidehiko, Mark, what's missing for this to compile in OS_NACL_NONSFI? > > > > > > One PID per NS is very desirable for NONSFI. > > > > base::ForkWithFlags in base/process/launch_posix.cc. > > It is not built in libbase_nacl_nonsfi.a now. > > > > Supporting ForkWithFlags *only* would not be much difficult, IIUC. > > Though, other code in the file would hit problems if we build with PNaCl > > toolchain, especially around signals, so I just dropped the file at all, as we > > didn't need it. > > > > Also, in ForkInNewPidNamespace, we need to use LINUX_SIGCHLD instead of > SIGCHLD > > for the workaround of signal ABI problem. > > FYI: Like this https://codereview.chromium.org/1161933003/ > Thanks for the comments, Hidehiko - it sounds like there might be a little extra work involved for nonsfi, so I'll start working on that in a separate CL. https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/nacl_helper_linux.cc:204: child_pid = fork(); On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > We never drop all capabilities in this codepath! The combination of keeping CAP_SYS_ADMIN and hitting this codepath is probably impossible, but agreed, the separation of responsibility is really unclear :-( What do you think about this change? I added a helper that forks and drops capabilities, and call that instead of fork(). https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/nacl_helper_linux.cc:205: #if !defined(OS_NACL_NONSFI) On 2015/05/28 21:23:23, mdempsky wrote: > nit: Having an #if block just for a } is kind of ugly. I'd suggest either > reshuffling the "} else { #endif" to "} else #endif {" so you can have the close > brace unconditionally, or the more intrusive option of something like > > namespace { > #if defined(OS_NACL_NONSFI) > pid_t ForkChild() { > return sandbox::NamespaceSandbox::ForkInNewPidNamespace(...); > } > #else > pid_t ForkChild() { > return fork(); > } > #endif > } // namespace > > Your call. Done. https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/1158793003/diff/20001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:143: // class is keeping a file descriptor to /proc/. On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > Doesn't this comment apply to both the setuid sandbox case and the userNS case? Yeah, removed this comment, it made more sense in the place I copy/pasted it from. https://codereview.chromium.org/1158793003/diff/20001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/1158793003/diff/20001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:21: inline int SignalExitCode(int sig) { On 2015/05/28 09:02:33, jln (OOO til. Jun 7th) wrote: > Let's qualify this in NamespaceSandbox:: as a static member. > > sandbox::something should pretty much not exist. Done.
I assume you're not waiting for me on this one? jln and mdempsky know the sandbox/ code better than me, so I can take a look after they've signed off on this and after we've rolled in https://codereview.chromium.org/1159803003/.
On 2015/06/04 20:34:33, Mark Seaborn wrote: > I assume you're not waiting for me on this one? jln and mdempsky know the > sandbox/ code better than me, so I can take a look after they've signed off on > this and after we've rolled in https://codereview.chromium.org/1159803003/. Oops, my mistake, I should have added you jln/mdempsky LGed. Thanks for checking!
The CQ bit was checked by jln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158793003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but please add a comment to the bug that NonSFI still needs support for this!
https://chromiumcodereview.appspot.com/1158793003/diff/60001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/1158793003/diff/60001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:86: static inline int SignalExitCode(int sig) { return -sig & 0xff; } Let's add a comment to describe this. Mention that it matches NaCl. You can also remove "inline", the compiler is going to inline this anyways.
https://codereview.chromium.org/1158793003/diff/60001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/1158793003/diff/60001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:86: static inline int SignalExitCode(int sig) { return -sig & 0xff; } On 2015/06/09 21:18:10, jln wrote: > Let's add a comment to describe this. Mention that it matches NaCl. You can also > remove "inline", the compiler is going to inline this anyways. Done.
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org Link to the patchset: https://codereview.chromium.org/1158793003/#ps80001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158793003/80001
The CQ bit was unchecked by rickyz@chromium.org
On 2015/06/09 22:59:19, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1158793003/80001 Oops, un-CQing this, I accidentally uploaded my change to enable this for nonSFI. Splitting that out of this one before submitting.
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org Link to the patchset: https://codereview.chromium.org/1158793003/#ps100001 (title: "Split out nonSFI bits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158793003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Ah oops, I'm not thinking straight. Can you take a look at components/nacl, mseaborn@?
LGTM for components/nacl/ https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:196: #if !defined(OS_NACL_NONSFI) Should this instead be "if (!uses_nonsfi_mode)"? Note that OS_NACL_NONSFI only applies to the new, newlib-based implementation (nacl_helper_nonsfi). In the currently-deployed implementation, one binary (nacl_helper) implements both SFI and Non-SFI NaCl. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:198: // The NaCl runtime will install signal handlers for SIGINT, SIGTERM, etc. Should this say "SFI NaCl's trusted runtime" to be more specific? https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:199: // so we do not need to install termination signal handlers ourselves. As an aside: also, SFI NaCl complains if they are registered. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (left): https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:142: // This relies on SealLayerOneSandbox() to be called later since this Isn't this still true? We still call SealLayerOneSandbox() later and the requirement is still there, isn't it?
Thanks! Mind taking a quick look at the changes here? I didn't realize that non-newlib non-SFI existed, so I made some changes to attempt to support that as well. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:196: #if !defined(OS_NACL_NONSFI) On 2015/06/09 23:44:19, Mark Seaborn wrote: > Should this instead be "if (!uses_nonsfi_mode)"? > > Note that OS_NACL_NONSFI only applies to the new, newlib-based implementation > (nacl_helper_nonsfi). In the currently-deployed implementation, one binary > (nacl_helper) implements both SFI and Non-SFI NaCl. Ah, I didn't realize that non-SFI without newlib was a thing. My intention was actually to enable one PID namespace per process for everything but newlib non-SFI. I've changed nonsfi/irt_exception_handling.cc to bring it in line with nacl_signal.c. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:198: // The NaCl runtime will install signal handlers for SIGINT, SIGTERM, etc. On 2015/06/09 23:44:19, Mark Seaborn wrote: > Should this say "SFI NaCl's trusted runtime" to be more specific? See above. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/nacl_helper_linux.cc:199: // so we do not need to install termination signal handlers ourselves. On 2015/06/09 23:44:19, Mark Seaborn wrote: > As an aside: also, SFI NaCl complains if they are registered. Done. https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (left): https://codereview.chromium.org/1158793003/diff/100001/components/nacl/loader... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:142: // This relies on SealLayerOneSandbox() to be called later since this On 2015/06/09 23:44:19, Mark Seaborn wrote: > Isn't this still true? We still call SealLayerOneSandbox() later and the > requirement is still there, isn't it? Yeah, it's still true - I removed the comment to be consistent with the fact that we don't explicitly call out the need for SealLayerOneSandbox when dropping FS access in the suid sandbox case. The fact that we continue to use proc_fd_ after DropFileSystemAccess hopefully makes it clear that there's still a dir fd around. https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); I didn't want to change this file a ton in this CL, but the behavior of SignalCatch looks a little weird here - I don't see how signal_handler_function_pointer ever gets the signal number, and the signal handler unconditionally exits with no apparent way to tell whether it was handled with a custom handler or not.
LGTM https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:32: SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGTRAP, SIGBUS, SIGFPE, SIGSEGV, SIGTERM, Do you need to add SIGHUP and SIGTERM, or is it just for consistency? The original purpose of this list is just for synchronous hardware exceptions that we want to pass on to the app. So SIGINT didn't need to be here, though it doesn't really hurt to have async signals here. If you need SIGHUP/SIGTERM, you might want to add them to the newlib-based implementation in native_client/src/nonsfi/linux/irt_exception_handling.c, which is slated to replace components/nacl/loader/nonsfi/irt_exception_handling.cc. https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); On 2015/06/10 01:31:43, rickyz wrote: > I didn't want to change this file a ton in this CL, but the behavior of > SignalCatch looks a little weird here - I don't see how > signal_handler_function_pointer ever gets the signal number, and the signal > handler unconditionally exits with no apparent way to tell whether it was > handled with a custom handler or not. Yes, that's all intentional. This implements the NaCl IRT interface for handling hardware exceptions, defined here: https://chromium.googlesource.com/native_client/src/native_client/+/master/sr... This interface currently doesn't report the type of hardware exception. It also doesn't support returning from the exception handler (though you can longjmp() out).
https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:32: SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGTRAP, SIGBUS, SIGFPE, SIGSEGV, SIGTERM, On 2015/06/10 18:53:12, Mark Seaborn wrote: > Do you need to add SIGHUP and SIGTERM, or is it just for consistency? > > The original purpose of this list is just for synchronous hardware exceptions > that we want to pass on to the app. So SIGINT didn't need to be here, though it > doesn't really hurt to have async signals here. > > If you need SIGHUP/SIGTERM, you might want to add them to the newlib-based > implementation in native_client/src/nonsfi/linux/irt_exception_handling.c, which > is slated to replace components/nacl/loader/nonsfi/irt_exception_handling.cc. I want to add any signals that might be sent to the process in an attempt to terminate it. When the NaCl loader is the init process in a pid namespace, Linux drops signals other than SIGSTOP and SIGKILL unless the process has an explicit signal handler set, which is why I want to make sure that there is a signal handler which will terminate the process by default. Will send a change for the replacement in native_client once I get a better grasp of what the correct change is :-) https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); On 2015/06/10 18:53:12, Mark Seaborn wrote: > On 2015/06/10 01:31:43, rickyz wrote: > > I didn't want to change this file a ton in this CL, but the behavior of > > SignalCatch looks a little weird here - I don't see how > > signal_handler_function_pointer ever gets the signal number, and the signal > > handler unconditionally exits with no apparent way to tell whether it was > > handled with a custom handler or not. > > Yes, that's all intentional. > > This implements the NaCl IRT interface for handling hardware exceptions, defined > here: > https://chromium.googlesource.com/native_client/src/native_client/+/master/sr... > > This interface currently doesn't report the type of hardware exception. It also > doesn't support returning from the exception handler (though you can longjmp() > out). Ah, in that case, is it incorrect to run the exception handler on SIGINT/SIGHUP/SIGTERM? Also, out of curiosity, is there a real need for the hardware exception interface on non-SFI? It wouldn't make things any less safe to let non-SFI programs set whatever signal handlers they want, right?
On 2015/06/10 22:42:34, rickyz wrote: > https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... > File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): > > https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... > components/nacl/loader/nonsfi/irt_exception_handling.cc:32: SIGHUP, SIGINT, > SIGQUIT, SIGILL, SIGTRAP, SIGBUS, SIGFPE, SIGSEGV, SIGTERM, > On 2015/06/10 18:53:12, Mark Seaborn wrote: > > Do you need to add SIGHUP and SIGTERM, or is it just for consistency? > > > > The original purpose of this list is just for synchronous hardware exceptions > > that we want to pass on to the app. So SIGINT didn't need to be here, though > it > > doesn't really hurt to have async signals here. > > > > If you need SIGHUP/SIGTERM, you might want to add them to the newlib-based > > implementation in native_client/src/nonsfi/linux/irt_exception_handling.c, > which > > is slated to replace components/nacl/loader/nonsfi/irt_exception_handling.cc. > > I want to add any signals that might be sent to the process in an attempt to > terminate it. When the NaCl loader is the init process in a pid namespace, Linux > drops signals other than SIGSTOP and SIGKILL unless the process has an explicit > signal handler set, which is why I want to make sure that there is a signal > handler which will terminate the process by default. > > Will send a change for the replacement in native_client once I get a better > grasp of what the correct change is :-) > > https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... > components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); > On 2015/06/10 18:53:12, Mark Seaborn wrote: > > On 2015/06/10 01:31:43, rickyz wrote: > > > I didn't want to change this file a ton in this CL, but the behavior of > > > SignalCatch looks a little weird here - I don't see how > > > signal_handler_function_pointer ever gets the signal number, and the signal > > > handler unconditionally exits with no apparent way to tell whether it was > > > handled with a custom handler or not. > > > > Yes, that's all intentional. > > > > This implements the NaCl IRT interface for handling hardware exceptions, > defined > > here: > > > https://chromium.googlesource.com/native_client/src/native_client/+/master/sr... > > > > This interface currently doesn't report the type of hardware exception. It > also > > doesn't support returning from the exception handler (though you can longjmp() > > out). > > Ah, in that case, is it incorrect to run the exception handler on > SIGINT/SIGHUP/SIGTERM? Also, out of curiosity, is there a real need for the > hardware exception interface on non-SFI? It wouldn't make things any less safe > to let non-SFI programs set whatever signal handlers they want, right? Hey, just wanted to double check on that last question before submitting this (and making the equiv change in native_client). Specifically aobout whether the exception handler should be run or not in response to SIGINT/SIGHUB/SIGTERM - thanks!
https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); On 2015/06/10 22:42:34, rickyz wrote: > On 2015/06/10 18:53:12, Mark Seaborn wrote: > > On 2015/06/10 01:31:43, rickyz wrote: > > > I didn't want to change this file a ton in this CL, but the behavior of > > > SignalCatch looks a little weird here - I don't see how > > > signal_handler_function_pointer ever gets the signal number, and the signal > > > handler unconditionally exits with no apparent way to tell whether it was > > > handled with a custom handler or not. > > > > Yes, that's all intentional. > > > > This implements the NaCl IRT interface for handling hardware exceptions, > defined > > here: > > > https://chromium.googlesource.com/native_client/src/native_client/+/master/sr... > > > > This interface currently doesn't report the type of hardware exception. It > also > > doesn't support returning from the exception handler (though you can longjmp() > > out). > > Ah, in that case, is it incorrect to run the exception handler on > SIGINT/SIGHUP/SIGTERM? Yes, it's technically incorrect, although it might not matter in practice. If having handlers for SIGINT/SIGHUP/SIGTERM is required by the sandbox code, it would be better to have the sandbox layer register those handlers, mostly for the sake of clarity. > Also, out of curiosity, is there a real need for the > hardware exception interface on non-SFI? It wouldn't make things any less safe > to let non-SFI programs set whatever signal handlers they want, right? It depends what level you're asking about: * the security of Non-SFI NaCl's Linux sandbox * the definition of Non-SFI NaCl's stable ABI For security: We currently disable the sigaction() syscall in the seccomp-bpf sandbox. We could allow it. It's *probably* safe. But we don't need to allow it, because we can pre-register a handler and have it forward. So as general attack surface reduction, we disable sigaction(). For the ABI definition: The interface that user programs are meant to use is *not* Linux syscalls. Non-SFI NaCl provides IRT interfaces instead (roughly the same interfaces as what SFI NaCl provides). We don't want to support the full generality of what Linux signals can do.
https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://codereview.chromium.org/1158793003/diff/120001/components/nacl/loader... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); On 2015/06/15 22:29:04, Mark Seaborn wrote: > On 2015/06/10 22:42:34, rickyz wrote: > > On 2015/06/10 18:53:12, Mark Seaborn wrote: > > > On 2015/06/10 01:31:43, rickyz wrote: > > > > I didn't want to change this file a ton in this CL, but the behavior of > > > > SignalCatch looks a little weird here - I don't see how > > > > signal_handler_function_pointer ever gets the signal number, and the > signal > > > > handler unconditionally exits with no apparent way to tell whether it was > > > > handled with a custom handler or not. > > > > > > Yes, that's all intentional. > > > > > > This implements the NaCl IRT interface for handling hardware exceptions, > > defined > > > here: > > > > > > https://chromium.googlesource.com/native_client/src/native_client/+/master/sr... > > > > > > This interface currently doesn't report the type of hardware exception. It > > also > > > doesn't support returning from the exception handler (though you can > longjmp() > > > out). > > > > Ah, in that case, is it incorrect to run the exception handler on > > SIGINT/SIGHUP/SIGTERM? > > Yes, it's technically incorrect, although it might not matter in practice. > > If having handlers for SIGINT/SIGHUP/SIGTERM is required by the sandbox code, it > would be better to have the sandbox layer register those handlers, mostly for > the sake of clarity. > > > Also, out of curiosity, is there a real need for the > > hardware exception interface on non-SFI? It wouldn't make things any less safe > > to let non-SFI programs set whatever signal handlers they want, right? > > It depends what level you're asking about: > * the security of Non-SFI NaCl's Linux sandbox > * the definition of Non-SFI NaCl's stable ABI > > For security: We currently disable the sigaction() syscall in the seccomp-bpf > sandbox. We could allow it. It's *probably* safe. But we don't need to allow > it, because we can pre-register a handler and have it forward. So as general > attack surface reduction, we disable sigaction(). > > For the ABI definition: The interface that user programs are meant to use is > *not* Linux syscalls. Non-SFI NaCl provides IRT interfaces instead (roughly the > same interfaces as what SFI NaCl provides). We don't want to support the full > generality of what Linux signals can do. Ah, thank you for the detailed explanation. In that case, I've switched to installing termination signal handlers in the nacl helper for nonsfi mode. For SFI mode, I'll leave the signal handlers to NaCl since the segments/signal stack stuff is tricky and security-sensitive.
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
FYI https://chromiumcodereview.appspot.com/1158793003/diff/160001/components/nacl... File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): https://chromiumcodereview.appspot.com/1158793003/diff/160001/components/nacl... components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); If you want to depend on this behavior, you might need to update src/native_client/src/nonsfi/linux/irt_exception_handling.c too which is the newlib-based NonSFI NaCl. We're trying to switch from glibc based NonSFI-NaCl to newlib based one.
On 2015/06/15 23:58:35, Junichi Uekawa wrote: > FYI > > https://chromiumcodereview.appspot.com/1158793003/diff/160001/components/nacl... > File components/nacl/loader/nonsfi/irt_exception_handling.cc (right): > > https://chromiumcodereview.appspot.com/1158793003/diff/160001/components/nacl... > components/nacl/loader/nonsfi/irt_exception_handling.cc:52: _exit(-sig); > If you want to depend on this behavior, you might need to update > src/native_client/src/nonsfi/linux/irt_exception_handling.c too > which is the newlib-based NonSFI NaCl. > We're trying to switch from glibc based NonSFI-NaCl to newlib based one. Thanks for the reminder - I just sent out https://codereview.chromium.org/1184233002/ to do this.
On 2015/06/15 23:23:51, rickyz wrote: > Ah, thank you for the detailed explanation. In that case, I've switched to > installing termination signal handlers in the nacl helper for nonsfi mode. For > SFI mode, I'll leave the signal handlers to NaCl since the segments/signal stack > stuff is tricky and security-sensitive. LGTM still. Thanks for checking up about that!
On 2015/06/16 21:41:38, Mark Seaborn wrote: > On 2015/06/15 23:23:51, rickyz wrote: > > Ah, thank you for the detailed explanation. In that case, I've switched to > > installing termination signal handlers in the nacl helper for nonsfi mode. For > > SFI mode, I'll leave the signal handlers to NaCl since the segments/signal > stack > > stuff is tricky and security-sensitive. > > LGTM still. Thanks for checking up about that! Thanks - just to update what's going on with this - the current plan is to wait until https://codereview.chromium.org/1176413003/ lands - then we can enable one PID namespace per process for newlib nonsfi as well in one shot in this change.
https://codereview.chromium.org/1176413003/ landed, so I enabled one PID namespace for process for all NaCl processes, including nonsfi newlib NaCl. Finally submitting :-)
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org, mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/1158793003/#ps180001 (title: "Enable PID namespace per process for nonsfi newlib NaCl as well.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158793003/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d423ad87b1fdc8a59a7c945c000bfbbd9d8b2245 Cr-Commit-Position: refs/heads/master@{#335223} |
