Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10836243:
Add basic ARM policy to seccomp-bpf sandbox. (Closed)
|
Created:
8 years, 4 months ago by Jorge Lucangeli Obes Modified:
8 years, 4 months ago Reviewers:
jln (very slow on Chromium) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, agl, jln+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd basic ARM policy to seccomp-bpf sandbox.
BUG=141157
TEST=about:sandbox on daisy shows "Seccomp-BPF Yes".
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151650
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address jln's comments. #Patch Set 3 : Add newline. #
Total comments: 10
Patch Set 4 : Addressed more comments. #
Total comments: 2
Patch Set 5 : Fix nits. #
Messages
Total messages: 10 (0 generated)
Still trying to decide whether to include all ARM syscalls now, or just include the ones needed for the BlacklistPtrace policy. In any case, that's orthogonal to 99% of the review. PTAL.
https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:51: inline bool IsARM() { You're not using it at the moment. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:163: #if defined(__i386__) || defined(__x86_64__) Did you miss this? https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:167: // system calls. Also you probably want to add something to this comment: "We also make sure this can compile on ARM, but we didn't list arm-specific system calls yet. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:187: #if !defined(__arm__) It's better to whitelist: #if defined(__i386__) || defined(__x86_64__) https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:766: #if !defined(__arm__) Same here. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:959: void LogSandboxStarted(const std::string& sandbox_name, Rebase issue ? This should not exist in this file. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1230: #if !defined(__arm__) Same remark about whitelisting. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1303: // On ARM, we don't block anything yet. This seems at odd with what you're doing. Did you mean AllowAllPolicy ? If not, please merge both in a elif defined i386 || arm. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.h (left): https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.h:31: New lines at the end of files is mandated by the style guide. I don't think we're super strict about it, but for sure, we shouldn't go out of our way to remove them. https://chromiumcodereview.appspot.com/10836243/diff/1/sandbox/linux/services... File sandbox/linux/services/arm_linux_syscalls.h (right): https://chromiumcodereview.appspot.com/10836243/diff/1/sandbox/linux/services... sandbox/linux/services/arm_linux_syscalls.h:13: #include <asm/unistd.h> Please, add a comment stating that this is a diff too the older unistd.h we're expected to support for this architecture.
https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:38: #include "sandbox/linux/services/arm_linux_syscalls.h" Once you're confident arm is well covered, please rename x86_linux_syscalls.h to linux_syscalls.h instead and include the arm file from there.
PTAL https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:38: #include "sandbox/linux/services/arm_linux_syscalls.h" On 2012/08/14 20:11:50, Julien Tinnes wrote: > Once you're confident arm is well covered, please rename x86_linux_syscalls.h to > linux_syscalls.h instead and include the arm file from there. Will do. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:51: inline bool IsARM() { On 2012/08/14 19:10:35, Julien Tinnes wrote: > You're not using it at the moment. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:187: #if !defined(__arm__) On 2012/08/14 19:10:35, Julien Tinnes wrote: > It's better to whitelist: #if defined(__i386__) || defined(__x86_64__) Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:766: #if !defined(__arm__) On 2012/08/14 19:10:35, Julien Tinnes wrote: > Same here. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:959: void LogSandboxStarted(const std::string& sandbox_name, On 2012/08/14 19:10:35, Julien Tinnes wrote: > Rebase issue ? This should not exist in this file. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1230: #if !defined(__arm__) On 2012/08/14 19:10:35, Julien Tinnes wrote: > Same remark about whitelisting. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1303: // On ARM, we don't block anything yet. On 2012/08/14 19:10:35, Julien Tinnes wrote: > This seems at odd with what you're doing. Did you mean AllowAllPolicy ? > > If not, please merge both in a elif defined i386 || arm. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.h (left): https://chromiumcodereview.appspot.com/10836243/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.h:31: On 2012/08/14 19:10:35, Julien Tinnes wrote: > New lines at the end of files is mandated by the style guide. > > I don't think we're super strict about it, but for sure, we shouldn't go out of > our way to remove them. Have no idea how that got in there. Done. https://chromiumcodereview.appspot.com/10836243/diff/1/sandbox/linux/services... File sandbox/linux/services/arm_linux_syscalls.h (right): https://chromiumcodereview.appspot.com/10836243/diff/1/sandbox/linux/services... sandbox/linux/services/arm_linux_syscalls.h:13: #include <asm/unistd.h> On 2012/08/14 19:10:35, Julien Tinnes wrote: > Please, add a comment stating that this is a diff too the older unistd.h we're > expected to support for this architecture. The reason we need unistd is that NR_SYSCALL_BASE is defined there.
Looks good with a few minor issues. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:38: #include "sandbox/linux/services/arm_linux_syscalls.h" Please add a comment saying that this file is not complete yet as well. When it'll be complete, x86_linux_syscalls.h should be renamed linux_syscalls.h and the arm file should be included there directly. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1200: // __NR_migrate_pages is not available for EABI ARM. I think it's fine to leave this without a comment at all. Or at least remove the second line. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1270: #elif defined(__i386__) || defined(__arm__) I would prefer just leaving a #else here. It simplifies the codepath, but also it's a nice default to have. I someone added an architecture, they would quickly see a compile-error and make BlacklistPtracePolicy work. You can leave a comment such as: "On other architectures (currently IA32 or ARM), we only have a small blacklist at the moment. At the moment, the change is not even correct, because in release mode, NOTREACHED() is a no-op, so this function wouldn't have a return statement which would produce a compile-time error even though it's not what we expect. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1283: const std::string& process_type) { Nit: re-indent. https://chromiumcodereview.appspot.com/10836243/diff/8002/sandbox/linux/servi... File sandbox/linux/services/arm_linux_syscalls.h (right): https://chromiumcodereview.appspot.com/10836243/diff/8002/sandbox/linux/servi... sandbox/linux/services/arm_linux_syscalls.h:8: Please add a comment somewhere stating that this file is WIP and doesn't list all system calls yet.
PTAL. Thanks! https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:38: #include "sandbox/linux/services/arm_linux_syscalls.h" On 2012/08/14 22:19:32, Julien Tinnes wrote: > Please add a comment saying that this file is not complete yet as well. > > When it'll be complete, x86_linux_syscalls.h should be renamed linux_syscalls.h > and the arm file should be included there directly. Done. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1200: // __NR_migrate_pages is not available for EABI ARM. On 2012/08/14 22:19:32, Julien Tinnes wrote: > I think it's fine to leave this without a comment at all. > Or at least remove the second line. Done. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1270: #elif defined(__i386__) || defined(__arm__) On 2012/08/14 22:19:32, Julien Tinnes wrote: > I would prefer just leaving a #else here. > > It simplifies the codepath, but also it's a nice default to have. > > I someone added an architecture, they would quickly see a compile-error and make > BlacklistPtracePolicy work. > > You can leave a comment such as: "On other architectures (currently IA32 or > ARM), we only have a small blacklist at the moment. > > At the moment, the change is not even correct, because in release mode, > NOTREACHED() is a no-op, so this function wouldn't have a return statement which > would produce a compile-time error even though it's not what we expect. Done. https://chromiumcodereview.appspot.com/10836243/diff/8002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1283: const std::string& process_type) { On 2012/08/14 22:19:32, Julien Tinnes wrote: > Nit: re-indent. Done. https://chromiumcodereview.appspot.com/10836243/diff/8002/sandbox/linux/servi... File sandbox/linux/services/arm_linux_syscalls.h (right): https://chromiumcodereview.appspot.com/10836243/diff/8002/sandbox/linux/servi... sandbox/linux/services/arm_linux_syscalls.h:8: On 2012/08/14 22:19:32, Julien Tinnes wrote: > Please add a comment somewhere stating that this file is WIP and doesn't list > all system calls yet. Done.
LGTM (with nits so minor that you should feel free to skip). https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:1192: #endif // defined(__x86_64__) || defined(__i386__) This comment has the wrong order, feel free to correct it. https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:1201: #if defined(__x86_64__) || defined(__i386__) The rest of this file does i386 || x86_64, would be nice to do the same.
On 2012/08/14 22:51:45, Julien Tinnes wrote: > LGTM > > (with nits so minor that you should feel free to skip). > > https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... > content/common/sandbox_seccomp_bpf_linux.cc:1192: #endif // defined(__x86_64__) > || defined(__i386__) > This comment has the wrong order, feel free to correct it. > > https://chromiumcodereview.appspot.com/10836243/diff/11001/content/common/san... > content/common/sandbox_seccomp_bpf_linux.cc:1201: #if defined(__x86_64__) || > defined(__i386__) > The rest of this file does i386 || x86_64, would be nice to do the same. linux_rel failures seem unrelated. Let's see what the CQ thinks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10836243/12001
Change committed as 151650 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
