Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
260793003:
[MIPS] Add seccomp bpf support (Closed)
|
Created:
6 years, 7 months ago by nedeljko Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, agl, jln+watch_chromium.org, Kees Cook, Will Drewry, szager1 Base URL:
https://git.chromium.org/git/chromium/src.git@master Visibility:
Public. |
Description[MIPS] Add seccomp bpf support
Add support for seccomp bpf sandboxing on MIPS architecture for O32 ABI.
Enable testing of seccomp bpf sandbox.
Support for seccomp bpf for MIPS was added in Linux kernel version 3.15.
BUG=369594
TEST= sandbox_linux_unittests
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 36
Patch Set 3 : Update per code review #Patch Set 4 : Fix problem with truncation of syscall value in CrashSIGSYS_Handler #
Total comments: 35
Patch Set 5 : Update per code review #
Total comments: 31
Patch Set 6 : Update per code review #
Total comments: 25
Patch Set 7 : Update per code review #
Total comments: 18
Patch Set 8 : Update per code review #
Total comments: 11
Patch Set 9 : Update per code review #
Total comments: 2
Patch Set 10 : Add 8 args syscall support for Mips #
Total comments: 22
Patch Set 11 : Update per code review #Messages
Total messages: 65 (0 generated)
Please take a look at the change. Thanks, Nedeljko
The CQ bit was checked by nedeljko.babic@imgtec.com
The CQ bit was unchecked by nedeljko.babic@imgtec.com
Here is a first batch of comments. I have created https://crbug.com/369594 to track this and linked this bug number in your CL. I imagine that all the tests from sandbox_linux_unittests pass with your CL? https://chromiumcodereview.appspot.com/260793003/diff/10001/content/common/sa... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/content/common/sa... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:101: return EPERM; Ohh, this is very unfortunate :( Let's add a helper in sandbox/linux/services/kernel_to_errno.h With: inline int KernelRetToErrno(int kernel_ret) {} inline int ErrnoToKernelRet(int errno) {} That does what's required on each platform and let's wrap all of these. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/san... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/san... sandbox/linux/sandbox_linux.gypi:15: ['OS=="linux" and (target_arch=="ia32" or target_arch=="x64" or target_arch=="mipsel")', { Please, wrap https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:52: if ((sysno < __NR_Linux) || (sysno >= __NR_Linux + __NR_Linux_syscalls)) What is the value of __NR_linux? We need to make sure that sysno can be printed by the code below (with 4 digits). We might want to do if (sysno >= __NR_linux) sysno = sysno - __NR_linux; else sysno = 0; (and keep the following if (sysno >= 1024) sysno = 0) https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:83: // On MIPS syscall numbers are in different range than on x86 and ARM Let's make a function TruncSysnoToValu(int sysno, int max_value); instead of duplicating. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/linux_seccomp.h:219: #define SECCOMP_PARM5(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, 29)+4)) Nit: wrap https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/linux_seccomp.h:220: #define SECCOMP_PARM6(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, 29)+5)) Nit: wrap https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:587: sysno == __NR_pipe Let's change pipe() below to socketpair instead. This would bypass the test. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1681: #if defined(__mips__) Let's use a wrapper in services/ (see earlier comment). https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1811: &pid) == EPERM); Same remark. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.cc:171: #elif defined(__mips__) I can't review this, do you know someone who can? https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator.cc:22: COMPILE_ASSERT(MIN_SYSCALL == __NR_Linux, min_syscall_should_be_4000); The syscall iterator should always start at 0, this is why this compile assert was here. So, in the constructor, keep initializing num_ to 0. Here, do something similar to the version in this CL: https://chromiumcodereview.appspot.com/11096012/diff/1/sandbox/linux/seccomp-... i.e. if MIN_SYSCALL is not 0, the iterator should first return MIN_SYSCALL -1 before continuing to iterate. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator.h (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator.h:39: : invalid_only_(invalid_only), done_(false), num_(4000) {} Is there a defined symbol instad of "4000"? https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:22: SANDBOX_ASSERT(next == 4000); This adds no value, we need to re-work these tests to make them compatible with MIN_SYSCALL > 0. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:25: SANDBOX_ASSERT(next == 0); This should still hold on MIPS. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:162: #if defined (__mips__) style: #if defined(), no space. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:167: RAW_SANDBOX_DIE("Sanity checks are failing after receiving SIGSYS."); I don't think this is correct. The code is no longer dying if "sigsys.arch != SECCOMP_ARCH" for instance. do something like: bool sigsys_nr_is_correct = false; #if defined(__mips__) sigsys_nr_is_correct = sigsys.nr == static_cast<int>(SECCOMP_PARM1(ctx) #else sigsys_nr == static_cast<int>(SECCOMP_SYSCALL(ctx)) #endif And use this bool in the if() https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:216: rc = -rc; Let's use a wrapper for this (see comment in other files).
https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc:48: SyscallSets::IsMipsSpecific(sysno) || ...Private vs ...Specific ?
New patch added. Please take a look. Thanks, Nedeljko https://chromiumcodereview.appspot.com/260793003/diff/10001/content/common/sa... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/content/common/sa... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:101: return EPERM; On 2014/05/02 20:42:04, jln wrote: > Ohh, this is very unfortunate :( > > Let's add a helper in sandbox/linux/services/kernel_to_errno.h > > With: > > inline int KernelRetToErrno(int kernel_ret) {} > > inline int ErrnoToKernelRet(int errno) {} > > That does what's required on each platform and let's wrap all of these. Helper added. I created RetValToKernel instead of ErrnoToKernel because MIPS is different from other architectures (x86 and ARM) in the way in which it distinguishes correct return value from error. On MIPS, kernel returns two values. One is regular ret val or errno and second is value that can be checked to see if syscall was executed successfully. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/san... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/san... sandbox/linux/sandbox_linux.gypi:15: ['OS=="linux" and (target_arch=="ia32" or target_arch=="x64" or target_arch=="mipsel")', { On 2014/05/02 20:42:04, jln wrote: > Please, wrap Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc:48: SyscallSets::IsMipsSpecific(sysno) || On 2014/05/02 22:06:35, Kees Cook wrote: > ...Private vs ...Specific ? There is no private syscall range on mips like there is on arm. On the other hand, syscalls from this function are basically the same as syscalls from IsArmPrivate, so I changed function name to IsMipsPrivate to adhere to naming convention used for arm. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:52: if ((sysno < __NR_Linux) || (sysno >= __NR_Linux + __NR_Linux_syscalls)) On 2014/05/02 20:42:04, jln wrote: > What is the value of __NR_linux? > > We need to make sure that sysno can be printed by the code below (with 4 > digits). > > We might want to do > if (sysno >= __NR_linux) sysno = sysno - __NR_linux; else sysno = 0; > > (and keep the following if (sysno >= 1024) sysno = 0) Value of __NR_Linux for is either 4000, 5000 or 6000 depending on ABI and max value for syscall is __NR_Linux + __NR_Linux_syscalls. Nevertheless, I decided to truncate value as you suggested because of the function CrashSIGSYS_Handler that uses only 12 bits for syscall number. As for keeping if (sysno >= 1024) sysno = 0), on mips max offset from min syscall number to max number is 350 (__NR_Linux_syscalls) so I didnt keep that if statement. One more thing: on O32 ABI the start of syscall range (4000) is also valid syscall number, so I decided to use the value that is out of range for appropriate ABI (__NR_Linux + 1000) https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:83: // On MIPS syscall numbers are in different range than on x86 and ARM On 2014/05/02 20:42:04, jln wrote: > Let's make a function TruncSysnoToValu(int sysno, int max_value); instead of > duplicating. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/linux_seccomp.h:219: #define SECCOMP_PARM5(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, 29)+4)) On 2014/05/02 20:42:04, jln wrote: > Nit: wrap Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/linux_seccomp.h:220: #define SECCOMP_PARM6(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, 29)+5)) On 2014/05/02 20:42:04, jln wrote: > Nit: wrap Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:587: sysno == __NR_pipe On 2014/05/02 20:42:04, jln wrote: > Let's change pipe() below to socketpair instead. This would bypass the test. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1681: #if defined(__mips__) On 2014/05/02 20:42:04, jln wrote: > Let's use a wrapper in services/ (see earlier comment). Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1811: &pid) == EPERM); On 2014/05/02 20:42:04, jln wrote: > Same remark. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.cc:171: #elif defined(__mips__) On 2014/05/02 20:42:04, jln wrote: > I can't review this, do you know someone who can? petarj will do a review of this. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator.cc:22: COMPILE_ASSERT(MIN_SYSCALL == __NR_Linux, min_syscall_should_be_4000); On 2014/05/02 20:42:04, jln wrote: > The syscall iterator should always start at 0, this is why this compile assert > was here. > So, in the constructor, keep initializing num_ to 0. > > Here, do something similar to the version in this CL: > https://chromiumcodereview.appspot.com/11096012/diff/1/sandbox/linux/seccomp-... > > i.e. if MIN_SYSCALL is not 0, the iterator should first return MIN_SYSCALL -1 > before continuing to iterate. I initialized num_ to 0 in constructor in new patch. However this assert checks value of MIN_SYSCALL and on MIPS this value is not zero. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator.h (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator.h:39: : invalid_only_(invalid_only), done_(false), num_(4000) {} On 2014/05/02 20:42:04, jln wrote: > Is there a defined symbol instad of "4000"? __NR_Linux can be used, although I need to include appropriate header file for this. On the other hand, I will use num_(0) here in new patch... https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:22: SANDBOX_ASSERT(next == 4000); On 2014/05/02 20:42:04, jln wrote: > This adds no value, we need to re-work these tests to make them compatible with > MIN_SYSCALL > 0. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:25: SANDBOX_ASSERT(next == 0); On 2014/05/02 20:42:04, jln wrote: > This should still hold on MIPS. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:162: #if defined (__mips__) On 2014/05/02 20:42:04, jln wrote: > style: #if defined(), no space. Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:167: RAW_SANDBOX_DIE("Sanity checks are failing after receiving SIGSYS."); On 2014/05/02 20:42:04, jln wrote: > I don't think this is correct. The code is no longer dying if "sigsys.arch != > SECCOMP_ARCH" for instance. > > do something like: > > bool sigsys_nr_is_correct = false; > #if defined(__mips__) > sigsys_nr_is_correct = sigsys.nr == static_cast<int>(SECCOMP_PARM1(ctx) > #else > sigsys_nr == static_cast<int>(SECCOMP_SYSCALL(ctx)) > #endif > > And use this bool in the if() Done. https://chromiumcodereview.appspot.com/260793003/diff/10001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:216: rc = -rc; On 2014/05/02 20:42:04, jln wrote: > Let's use a wrapper for this (see comment in other files). Done.
PING
Hello, Can someone check if the newest patch set is ok, please. Thanks, Nedeljko
On 2014/05/16 13:00:49, nedeljko wrote: > Hello, > > Can someone check if the newest patch set is ok, please. Sorry for the long delay. Will do today.
Sorry for the long delay. I wanted to land a new unit test suite (basline_policy_unittests.cc) before you could land this and I didn't manage to finish it yet. I have done another pass today. We're not too far :) https://chromiumcodereview.appspot.com/260793003/diff/50001/content/common/sa... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/content/common/sa... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:100: return KernelRetToErrno(EPERM); This function is misnamed in this context. EPERM is an errno. We want to convert it to a kernel return value. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:54: sysno = sysno - __NR_Linux + 1000; I don't understand what this does. Please document the function above in a comment. Invalid syscall values should be transformed to zero. Valid syscall values should be transformed to sysno - __NR_linux. Please choose another name as well. The name should be something along the lines of: "SyscallNumberToOffsetFromBase()" https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1681: return KernelRetToErrno(EPERM); ErrnoToKernelRet() https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:38: if (MIN_SYSCALL) { Let's just create Two Tests: SyscallIteratorIntelArm and SyscallIteratorMIPS. This way, we can have both test check that the iterator does what we expect for the architecture without any complicated logic. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:119: // (this is currently valid for Intel and ARM EABI), except Again, let's split this into two tests, one for MIPS, one for Intel + ARM. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:156: // When indirect syscall (syscall(__NR_foo, ...)) is made on Mips, number "number" -> "the number" https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:157: // in register SECCOMP_SYSCALL(ctx) is always __NR_syscall and real "real number" -> "the real number" https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:218: static_cast<greg_t>(RetValToKernelRet(static_cast<int>(rc), ctx)); This is too commplicated. Let's have a function that can put a "return code" rc into a context (ctx). See my comment in kernel_to_errno.h. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... File sandbox/linux/services/kernel_to_errno.h (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. - Please, add a _unittest.cc file for this with a least a few basic unit tests. - Let's rename it something more generic like "kernel_return_value_helpers.h" maybe? https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:8: namespace { This should be "namespace sandbox". https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:10: inline int KernelRetToErrno(int kernel_ret) { In practice I've only seen this used taking an errno and returning a kernel return value. Shouldn't this be ErrnoToKernelret() instead? It's possible that we need both though. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { Please add valid #include for ucontext_t. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { Let's make this a function that returns void instead. Maybe "void PutValueInUcontext(int ret_val, ucontext_t* ctx)" ? It should both set the return code to the valid value in the adequate register and, on MIPS, also modify a3.
https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... File sandbox/linux/services/kernel_to_errno.h (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { Let's have the implementation hidden in a .cc file instead of an inline function. We win very little by having the function inline and we win a lot by hiding the implementation with proper #include and unit tests.
Thank you on your review. I have a few questions. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:54: sysno = sysno - __NR_Linux + 1000; On MIPS O32 ABI value of __NR_Linux is 4000. The value of the first syscall (__NR_syscall) is also 4000. With sysno - __NR_Linux this valid syscall value would be transformed to zero. That is the reason why I transformed invalid values to 1000 + offset. On the other hand, I see that this is complicating things to much just for one syscall in one of the ABIs, so I'll do as you said in your comment and transform invalid values to zero. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:38: if (MIN_SYSCALL) { Should I create just two tests (that would be PublicSyscallRangeIntelArm and PublicSyscallRangeMIPS), or two iterators (SyscallIteratorIntelArm and SyscallIteratorMIPS). In later case I will have to rewrite all the places where this iterator is being used in order to support new iterators. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... File sandbox/linux/services/kernel_to_errno.h (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. These are pretty basic functions. Since functions in this file are only converting values from positive to negative and vice verse, I am not quit sure what should I test...
https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:38: if (MIN_SYSCALL) { On 2014/05/19 17:37:23, nedeljko wrote: > Should I create just two tests (that would be PublicSyscallRangeIntelArm and > PublicSyscallRangeMIPS), or two iterators (SyscallIteratorIntelArm and > SyscallIteratorMIPS). No, just two tests. The problem is that the SyscallIterator class is a little complicated because of all the different architectures. So you can make architecture-dependant unit tests that know about low-level kernel implementation of their architecture and check that things are sane there. Or, to put it another way: the unit tests would look like another implementation, but one that has a different function for every architecture. This redundancy allows us to test things properly. https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... File sandbox/linux/services/kernel_to_errno.h (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/ser... sandbox/linux/services/kernel_to_errno.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/05/19 17:37:23, nedeljko wrote: > These are pretty basic functions. > > Since functions in this file are only converting values from positive to > negative and vice verse, I am not quit sure what should I test... What should be tested is what these function do. A. For ErrnoToKernelRet(), you could have a test that looks like this: TEST(XXXX, ErrnoToKernelRet) { const int expected_kernel_ret = low_level_syscall(__NR_dup, -1); ASSERT_EQ(expected_kernel_ret, ErrnoToKernelRet(EBADF)); } Of course it's problematic if you don't have a low_level_syscall() implementation. You could want to use the one you added in seccomp-bpf, but services/ should not depend on seccomp-bpf/. Does the libc on MIPS give you anything? Any other approach you could think of? You could also simply put this in seccomp-bpf/ instead and create a test that uses the SandboxSyscall() function. B. For PutValueInUcontext(), things seem more difficult. - Is there anything clever to do with makecontext(3) and swapcontext(3)? It seems hard. - You could do a SANDBOX_TEST that does the following: 1. Create a new process with the sandbox::ScopedProcess class. 2. The child process runs a closure that does the following: - set-up a new signal handler (for instance for SIGSYS). The signal handler uses PutValueInUcontext() to put (for instance) EBADF as a return value in the ucontext_t. - the child calls int blah = syscall(__NR_pause); Then the child checks "blah" to be -1 and errno and return an error code depending on whether errno is "EBADF". 3. The parent signals the child with SIGSYS, then waits for it to exit. Then checks that the return code for the child is correct. This is a bit complicated. If you want I can write the unit test for Intel/ARM and you can simply make sure it works for MIPS.
https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... File sandbox/linux/services/kernel_to_errno.h (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/05/20 01:57:47, jln wrote: > On 2014/05/19 17:37:23, nedeljko wrote: > > These are pretty basic functions. > > > > Since functions in this file are only converting values from positive to > > negative and vice verse, I am not quit sure what should I test... > > What should be tested is what these function do. > > A. For ErrnoToKernelRet(), you could have a test that looks like this: > > TEST(XXXX, ErrnoToKernelRet) { > > const int expected_kernel_ret = low_level_syscall(__NR_dup, -1); > ASSERT_EQ(expected_kernel_ret, ErrnoToKernelRet(EBADF)); > } > > Of course it's problematic if you don't have a low_level_syscall() > implementation. You could want to use the one you added in seccomp-bpf, but > services/ should not depend on seccomp-bpf/. > > Does the libc on MIPS give you anything? Any other approach you could think of? > > You could also simply put this in seccomp-bpf/ instead and create a test that > uses the SandboxSyscall() function. > I was thinking to move this (implementation of functions in kernel_to_errno.h) to seccomp-bpf anyhow since I will need macros from linux_seccomp.h file for implementation of PutValueInUcontext(), so this is probably the best approach. > B. For PutValueInUcontext(), things seem more difficult. > > - Is there anything clever to do with makecontext(3) and swapcontext(3)? It > seems hard. > > - You could do a SANDBOX_TEST that does the following: > 1. Create a new process with the sandbox::ScopedProcess class. > 2. The child process runs a closure that does the following: > - set-up a new signal handler (for instance for SIGSYS). The signal handler uses > PutValueInUcontext() to put (for instance) EBADF as a return value in the > ucontext_t. > - the child calls int blah = syscall(__NR_pause); > Then the child checks "blah" to be -1 and errno and return an error code > depending on whether errno is "EBADF". > > 3. The parent signals the child with SIGSYS, then waits for it to exit. Then > checks that the return code for the child is correct. > > This is a bit complicated. If you want I can write the unit test for Intel/ARM > and you can simply make sure it works for MIPS. I would appreciate if you are willing to do this. Thanks.
I changed patch set according to previous review and rebased it to the newest version. Please take a look. Thanks, Nedeljko https://codereview.chromium.org/260793003/diff/50001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/260793003/diff/50001/content/common/sandbox_l... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:100: return KernelRetToErrno(EPERM); On 2014/05/16 19:30:17, jln wrote: > This function is misnamed in this context. EPERM is an errno. We want to convert > it to a kernel return value. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:54: sysno = sysno - __NR_Linux + 1000; On 2014/05/19 17:37:23, nedeljko wrote: > On MIPS O32 ABI value of __NR_Linux is 4000. > > The value of the first syscall (__NR_syscall) is also 4000. > > With sysno - __NR_Linux this valid syscall value would be transformed to zero. > That is the reason why I transformed invalid values to 1000 + offset. > > On the other hand, I see that this is complicating things to much just for one > syscall in one of the ABIs, so I'll do as you said in your comment and transform > invalid values to zero. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1681: return KernelRetToErrno(EPERM); On 2014/05/16 19:30:17, jln wrote: > ErrnoToKernelRet() Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:38: if (MIN_SYSCALL) { On 2014/05/20 01:57:47, jln wrote: > On 2014/05/19 17:37:23, nedeljko wrote: > > Should I create just two tests (that would be PublicSyscallRangeIntelArm and > > PublicSyscallRangeMIPS), or two iterators (SyscallIteratorIntelArm and > > SyscallIteratorMIPS). > > No, just two tests. > > The problem is that the SyscallIterator class is a little complicated because of > all the different architectures. > So you can make architecture-dependant unit tests that know about low-level > kernel implementation of their architecture and check that things are sane > there. > > Or, to put it another way: the unit tests would look like another > implementation, but one that has a different function for every architecture. > This redundancy allows us to test things properly. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:119: // (this is currently valid for Intel and ARM EABI), except On 2014/05/16 19:30:17, jln wrote: > Again, let's split this into two tests, one for MIPS, one for Intel + ARM. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/trap.cc (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/trap.cc:156: // When indirect syscall (syscall(__NR_foo, ...)) is made on Mips, number On 2014/05/16 19:30:17, jln wrote: > "number" -> "the number" Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/trap.cc:157: // in register SECCOMP_SYSCALL(ctx) is always __NR_syscall and real On 2014/05/16 19:30:17, jln wrote: > "real number" -> "the real number" Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/trap.cc:218: static_cast<greg_t>(RetValToKernelRet(static_cast<int>(rc), ctx)); On 2014/05/16 19:30:17, jln wrote: > This is too commplicated. Let's have a function that can put a "return code" rc > into a context (ctx). See my comment in kernel_to_errno.h. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... File sandbox/linux/services/kernel_to_errno.h (right): https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/05/16 19:30:17, jln wrote: > - Please, add a _unittest.cc file for this with a least a few basic unit tests. > > - Let's rename it something more generic like "kernel_return_value_helpers.h" > maybe? Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:8: namespace { On 2014/05/16 19:30:17, jln wrote: > This should be "namespace sandbox". Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:10: inline int KernelRetToErrno(int kernel_ret) { On 2014/05/16 19:30:17, jln wrote: > In practice I've only seen this used taking an errno and returning a kernel > return value. Shouldn't this be ErrnoToKernelret() instead? It's possible that > we need both though. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { On 2014/05/16 19:30:17, jln wrote: > Please add valid #include for ucontext_t. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { On 2014/05/16 19:30:17, jln wrote: > Let's make this a function that returns void instead. > > Maybe "void PutValueInUcontext(int ret_val, ucontext_t* ctx)" ? > > It should both set the return code to the valid value in the adequate register > and, on MIPS, also modify a3. Done. https://codereview.chromium.org/260793003/diff/50001/sandbox/linux/services/k... sandbox/linux/services/kernel_to_errno.h:19: inline int RetValToKernelRet(int ret_val, ucontext_t* ctx) { On 2014/05/16 21:27:10, jln wrote: > Let's have the implementation hidden in a .cc file instead of an inline > function. > > We win very little by having the function inline and we win a lot by hiding the > implementation with proper #include and unit tests. Done.
https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/50001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.cc:171: #elif defined(__mips__) Could Petarj take a look at this? https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:51: // we are truncating valid syscall value to offset from base. Let's add the guarantee explicitly that sysno will be between 0 and 1023 in the comment. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:62: if (sysno >= 1024) Let's do this unconditionally. On MIPS, this should be a no-op, but it makes it easy to assert that this function will never return something bigger than 1023. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:105: PrintSyscallError(syscall); The truncation here is important for security, as we are dereferencing this as an address. You still need to call SyscallNumberToOffsetFromBase() https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:5: #ifndef SANDBOX_LINUX_SERVICES_KERNEL_RETURN_VALUE_HELPERS_H Nit: the header guard requires a trailing "_" https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:12: int ErrnoToKernelRet(int kernel_ret); Please, document both function in this header file. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc:27: expected_kernel_ret = - expected_kernel_ret; This looks very strange as it doesn't really like we're testing anything. Is the __SYSCALL() macro available on MIPS? Maybe it could be a way to perform a raw syscall? https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.cc:210: "subu $v0, $zero, $v0\n" How about doing this negation in C, later. We could expose two methods: SandboxSyscallRaw(), which returns whatever the kernel returns. And SandboxSyscall() could be a small wrapper which negates the value on MIPS. That way we can use SandboxSyscallRaw in unit tests. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:40: while (next < MIN_SYSCALL - 1) { Don't perform a while loop here. This test should explicitly show what Next() does. It should do 0, then skip to MIN_SYSCALL - 1 immediately (or something similar). This test should be a great way to know exactly what happens without reading the code. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:66: #endif Nit: add "// defined(__mips__)" https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:130: uint32_t next = iter.Next(); You should ASSERT the value of iter.Next() here. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:140: printf("count: %d\n",count++); Did you forget to remove this printf? You should instead assert that the iterator never returns a public syscall. https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:174: #endif Nit: add "// defined(__arm__)" https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:176: #endif Nit: add "// defined(__mips__)
https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:51: // we are truncating valid syscall value to offset from base. On 2014/05/28 00:06:57, jln wrote: > Let's add the guarantee explicitly that sysno will be between 0 and 1023 in the > comment. On MIPS, sysno will be between 0 and __NR_Linux_syscalls, so I'll add something like: for MIPS 0 to __NR_Linux_syscalls, for Intel/Arm 0 to 1023. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:62: if (sysno >= 1024) On 2014/05/28 00:06:57, jln wrote: > Let's do this unconditionally. On MIPS, this should be a no-op, but it makes it > easy to assert that this function will never return something bigger than 1023. I am not sure that I understand you correctly. If I remove this condition, the function will truncate both invalid and valid sysno to zero on Intel/Arm. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:105: PrintSyscallError(syscall); On 2014/05/28 00:06:57, jln wrote: > The truncation here is important for security, as we are dereferencing this as > an address. > > You still need to call SyscallNumberToOffsetFromBase() Calling SyscallNumberToOffsetFromBase() here is a problem on MIPS. First call would truncate valid syscall numbers to offset from base, but then the second call (in PrintSyscallError()) would truncate this offset to zero, since offset is not a valid sysno... Maybe I should call SyscallNumberToOffsetFromBase() here and in PrintSyscallError() just do truncation: if (sysno >= 1024) sysno = 0; This way it will work correctly on MIPS also. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc:27: expected_kernel_ret = - expected_kernel_ret; On 2014/05/28 00:06:57, jln wrote: > This looks very strange as it doesn't really like we're testing anything. > > Is the __SYSCALL() macro available on MIPS? Maybe it could be a way to perform a > raw syscall? I could use SandboxSyscallRaw() as you suggested in syscall.cc https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:140: printf("count: %d\n",count++); On 2014/05/28 00:06:57, jln wrote: > Did you forget to remove this printf? Yes, sorry... > > You should instead assert that the iterator never returns a public syscall. But the counter of this loop is going over public syscall range. On the other hand, I should probably remove this loop and instead do one iteration. This way assert after this loop will check iterator correctly,
Please take a look. Thanks, Nedeljko https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:51: // we are truncating valid syscall value to offset from base. On 2014/05/28 00:06:57, jln wrote: > Let's add the guarantee explicitly that sysno will be between 0 and 1023 in the > comment. Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:5: #ifndef SANDBOX_LINUX_SERVICES_KERNEL_RETURN_VALUE_HELPERS_H On 2014/05/28 00:06:57, jln wrote: > Nit: the header guard requires a trailing "_" Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:12: int ErrnoToKernelRet(int kernel_ret); On 2014/05/28 00:06:57, jln wrote: > Please, document both function in this header file. Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/kernel_return_value_helpers_unittest.cc:27: expected_kernel_ret = - expected_kernel_ret; On 2014/05/28 09:33:43, nedeljko wrote: > On 2014/05/28 00:06:57, jln wrote: > > This looks very strange as it doesn't really like we're testing anything. > > > > Is the __SYSCALL() macro available on MIPS? Maybe it could be a way to perform > a > > raw syscall? > > I could use SandboxSyscallRaw() as you suggested in syscall.cc Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall.cc:210: "subu $v0, $zero, $v0\n" On 2014/05/28 00:06:57, jln wrote: > How about doing this negation in C, later. > > We could expose two methods: > > SandboxSyscallRaw(), which returns whatever the kernel returns. > > And SandboxSyscall() could be a small wrapper which negates the value on MIPS. > > That way we can use SandboxSyscallRaw in unit tests. I created SandboxSyscallRaw() as you suggested, but only for Mips, since it is only architecture that needs it. And only unit test that has some benefit of using it is KernelReturnValueHelpers.ErrnoToKernelRet https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall.cc:210: "subu $v0, $zero, $v0\n" On 2014/05/28 00:06:57, jln wrote: > How about doing this negation in C, later. > > We could expose two methods: > > SandboxSyscallRaw(), which returns whatever the kernel returns. > > And SandboxSyscall() could be a small wrapper which negates the value on MIPS. > > That way we can use SandboxSyscallRaw in unit tests. Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:40: while (next < MIN_SYSCALL - 1) { On 2014/05/28 00:06:57, jln wrote: > Don't perform a while loop here. This test should explicitly show what Next() > does. > > It should do 0, then skip to MIN_SYSCALL - 1 immediately (or something similar). > > This test should be a great way to know exactly what happens without reading the > code. Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:66: #endif On 2014/05/28 00:06:57, jln wrote: > Nit: add "// defined(__mips__)" Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:130: uint32_t next = iter.Next(); On 2014/05/28 00:06:57, jln wrote: > You should ASSERT the value of iter.Next() here. Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:140: printf("count: %d\n",count++); On 2014/05/28 09:33:43, nedeljko wrote: > On 2014/05/28 00:06:57, jln wrote: > > Did you forget to remove this printf? > > Yes, sorry... > > > > > You should instead assert that the iterator never returns a public syscall. > > But the counter of this loop is going over public syscall range. > > On the other hand, I should probably remove this loop and instead do one > iteration. > This way assert after this loop will check iterator correctly, Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:174: #endif On 2014/05/28 00:06:57, jln wrote: > Nit: add "// defined(__arm__)" Done. https://codereview.chromium.org/260793003/diff/70001/sandbox/linux/seccomp-bp... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:176: #endif On 2014/05/28 00:06:57, jln wrote: > Nit: add "// defined(__mips__) Done.
I'm getting very confused by the interface and the conversions between errno and -errno back and forth. (There is also silent truncating in trap.cc) Let's make sure we agree on the design. How about this: 1. I think we settled on having SandboxSyscall() return -errno on all architectures, even MIPS, correct? There is at least one instance in sandbox_bpf_unittest.cc, where I don't understand why this requires conversion. How come this test did continue to pass? 2. Let's declare that the trap handlers, such as the one in content/common/sandbox_linux/bpf_gpu_policy_linux.cc and the ones in sandbox_bpf_unittests.cc will return -errno on all architecture. Then let's do the conversion in PutValueInUcontext() and let's get rid of ErrnoToKernelRet() entirely. One major issue is that on MIPS, PutValueInUcontext() *must* know whether or not the return value is an error. And this information has been lost at that point. So unfortunately, I think that in the trap handlers function (i.e. things such as EnomemHandler in sandbox_bpf_unittest.cc) we should take an extra parameter to return whether or not there was a failure. If you want, I can take care of the conversion to this new interface and you can make it work on MIPS. What do you think? https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/70001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:62: if (sysno >= 1024) On 2014/05/28 09:33:43, nedeljko wrote: > On 2014/05/28 00:06:57, jln wrote: > > Let's do this unconditionally. On MIPS, this should be a no-op, but it makes > it > > easy to assert that this function will never return something bigger than > 1023. > > I am not sure that I understand you correctly. > If I remove this condition, the function will truncate both invalid and valid > sysno to zero on Intel/Arm. The "if (sysno >= 1024) sysno = 0" should be kept there on all architectures. That way, we can guarantee that the function will return a value between 0 and 1023. With that guarantee, we know that dereferencing this value will end-up in the first page of the address space. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:50: // This function returns values between 0 an 1023 on archs other than Mips. s/an/and/ Also, this statement should be "on all architectures". On MIPS the range is narrower, but this statement is still true. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc:22: if (ret_val < 0) { This can't work for instance with mmap(2). mmap() can return negative values that are not an error. We have a fundamental issue here, because this function requires information that has been lost at this point. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:5: #ifndef SANDBOX_LINUX_SERVICES_KERNEL_RETURN_VALUE_HELPERS_H__ Only one trailing "_". https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:16: int ErrnoToKernelRet(int kernel_ret); The argument is an errno, not |kernel_ret| The documentation for this interface should not mention implementation details (these should be documented in the .cc file). // Convert |errno| to the value returned by the kernel when it wants to return |errno| to userland after a system call. int ErrnoToKernelRet(int errno); https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:18: // The return value from the syscall is written to appropriate register. // Set the registers in |ctx| to match what they would be after a system call returning |ret_val|. Also ret_val shouldn't be an int. On a 64 bits architecture, it should be 64 bits. Let's make it an intptr_t? https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:188: return -ENOMEM; How come this didn't have to be changed? https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1902: &pid) == ErrnoToKernelRet(EPERM)); This should be -EPERM, no? SandboxSyscall() always returns -errno on error now, doesn't it? Why did this pass? https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.cc:280: if(err_status) Nit: space after if and backets {} since it's a multi-line statement because of the comments. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:14: // We have to make sure that we have a single "magic" return address for Could you add some explicit documentation that this will return the kernel return value on success and -errno on error on all architectures, even MIPS? https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:34: // directly (like in the most tests). "like in most tests" https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:35: intptr_t SandboxSyscallRaw(int nr, Please, document |args| and |err_stat| https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:36: const intptr_t *args, Nit: chromium style is "type* variable". https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:211: rc = err.fnc_(data, err.aux_); I think we'll need to change the interface of the trap handlers unfortunately :( From (const struct arch_seccomp_data& args, void* aux) to something like: (const struct arch_seccomp_data& args, bool* is_error, void* aux). This will require quite a bit of refactor, sorry about that. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:217: PutValueInUcontext(static_cast<int>(rc), ctx); You're silently truncating |rc|. This is not correct on 64 bits architectures.
On 2014/06/03 01:00:32, jln wrote: > I'm getting very confused by the interface and the conversions between errno and > -errno back and forth. > (There is also silent truncating in trap.cc) > > Let's make sure we agree on the design. How about this: > > 1. I think we settled on having SandboxSyscall() return -errno on all > architectures, even MIPS, correct? > Correct. > There is at least one instance in sandbox_bpf_unittest.cc, where I don't > understand why this requires conversion. How come this test did continue to > pass? > There was the same conversion in appropriate handler, so they "canceled out" each other (so to speak...). > 2. Let's declare that the trap handlers, such as the one in > content/common/sandbox_linux/bpf_gpu_policy_linux.cc and the ones in > sandbox_bpf_unittests.cc will return -errno on all architecture. > > Then let's do the conversion in PutValueInUcontext() and let's get rid of > ErrnoToKernelRet() entirely. > I removed ErrnoToKernelRet() from all the places where it was used in sandbox/ folder. Currently the only place where it is used is in content/common/sandbox_linux/bpf_gpu_policy_linux.cc and I will remove it from there once the new interface is ready. > One major issue is that on MIPS, PutValueInUcontext() *must* know whether or not > the return value is an error. And this information has been lost at that point. > So unfortunately, I think that in the trap handlers function (i.e. things such > as EnomemHandler in sandbox_bpf_unittest.cc) we should take an extra parameter > to return whether or not there was a failure. > > If you want, I can take care of the conversion to this new interface and you can > make it work on MIPS. > > What do you think? > I agree. Once you do a conversion to new interface, I will make necessary changes so it works on MIPS. Thanks.
This patch set has all the changes that don't depend on the new interface. Please take a look if something else needs to be changed. As soon as new interface is ready, I can make necessary changes in parts of the patch set (and rest of the code) that do depend on that interface. Thanks, Nedeljko https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:50: // This function returns values between 0 an 1023 on archs other than Mips. On 2014/06/03 01:00:33, jln wrote: > s/an/and/ > > Also, this statement should be "on all architectures". On MIPS the range is > narrower, but this statement is still true. Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:5: #ifndef SANDBOX_LINUX_SERVICES_KERNEL_RETURN_VALUE_HELPERS_H__ On 2014/06/03 01:00:33, jln wrote: > Only one trailing "_". I looked in most of the *.h files in seccomp-bpf folder and in all the files that I looked there are two trailing "_" ... https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:16: int ErrnoToKernelRet(int kernel_ret); On 2014/06/03 01:00:33, jln wrote: > The argument is an errno, not |kernel_ret| > > The documentation for this interface should not mention implementation details > (these should be documented in the .cc file). > > // Convert |errno| to the value returned by the kernel when it wants to return > |errno| to userland after a system call. > int ErrnoToKernelRet(int errno); Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:18: // The return value from the syscall is written to appropriate register. On 2014/06/03 01:00:33, jln wrote: > // Set the registers in |ctx| to match what they would be after a system call > returning |ret_val|. > > Also ret_val shouldn't be an int. On a 64 bits architecture, it should be 64 > bits. Let's make it an intptr_t? Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:188: return -ENOMEM; On 2014/06/03 01:00:33, jln wrote: > How come this didn't have to be changed? It works correctly. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1902: &pid) == ErrnoToKernelRet(EPERM)); On 2014/06/03 01:00:33, jln wrote: > This should be -EPERM, no? > > SandboxSyscall() always returns -errno on error now, doesn't it? > You are correct. > Why did this pass? Because there is "return ErrnoToKernelRet(EPERM)" in PthreadTrapHandler(). I will revert this back to -EPERM. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:14: // We have to make sure that we have a single "magic" return address for On 2014/06/03 01:00:33, jln wrote: > Could you add some explicit documentation that this will return the kernel > return value on success and -errno on error on all architectures, even MIPS? Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:34: // directly (like in the most tests). On 2014/06/03 01:00:33, jln wrote: > "like in most tests" Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:35: intptr_t SandboxSyscallRaw(int nr, On 2014/06/03 01:00:33, jln wrote: > Please, document |args| and |err_stat| Done. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/syscall.h:36: const intptr_t *args, On 2014/06/03 01:00:33, jln wrote: > Nit: chromium style is "type* variable". I was looking at the syscall.cc when I wrote this and there is "type *variable" in use there, but I can see now that this is wrong. https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/90001/sandbox/linux/sec... sandbox/linux/seccomp-bpf/trap.cc:217: PutValueInUcontext(static_cast<int>(rc), ctx); On 2014/06/03 01:00:33, jln wrote: > You're silently truncating |rc|. This is not correct on 64 bits architectures. I changed this.
Ping. Do I need to do any additional changes before new interface is created? Thanks, Nedeljko
On 2014/06/10 08:44:11, nedeljko wrote: > Ping. > > Do I need to do any additional changes before new interface is created? Sorry for the delay. I should be able to do this this week. Hopefully we can wrap this up soon!
On 2014/06/03 15:06:12, nedeljko wrote: > On 2014/06/03 01:00:32, jln wrote: > > One major issue is that on MIPS, PutValueInUcontext() *must* know whether or > not > > the return value is an error. And this information has been lost at that > point. > > So unfortunately, I think that in the trap handlers function (i.e. things such > > as EnomemHandler in sandbox_bpf_unittest.cc) we should take an extra parameter > > to return whether or not there was a failure. > > > > If you want, I can take care of the conversion to this new interface and you > can > > make it work on MIPS. > > > > What do you think? > > > > I agree. Once you do a conversion to new interface, I will make necessary > changes > so it works on MIPS. > > Thanks. I've spent some time today on this new interface and migrating all the code to it. However, at some point I realized that I was incorrect. On X86, the kernel actually does guarantee that errors and return values between -1 and -4095 are equivalent. So I'm revising my position: SandboxSyscall() (which I'm renaming Syscall::Call() in https://chromiumcodereview.appspot.com/330723003/) must return -errno. Similarly, I think all SIGSYS trap handlers should be able to return -errno on error. So in other words: let's keep the X86-style interface for everything and make sure that we translate things appropriately for MIPS. I'll take a look at the current version of your CL and make the appropriate comments.
Sorry for the back and forth :( This is not far from ready. As mentioned above, I think we should simply: - Have SandboxSyscall() follow the X86 ABI. - Have trap handlers (Trap::TrapFnc) follow the same interface and continue to return -errno. I think it is a sufficient interface, since afaik, we can assume that an error happened with error codes between -1 and -4095. That is, I think it is guaranteed that no system call would eve return a value between -1 and -4095 on success. https://chromiumcodereview.appspot.com/260793003/diff/110001/content/common/s... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/content/common/s... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:104: return sandbox::ErrnoToKernelRet(EPERM); Let's just declare that trap handlers have the same interface as SandboxSyscall() and return -errno. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc:26: if (ret_val < 0) { This should be if (retval <= -1 && ret_val >= -4095) (or we could define MAX_ERRNO in syscall.h instead) and use that. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc:28: // SandboxSyscall() in case of Mips in order for SandboxSyscall() to This code must not have knowledge of the codepath that called it. Instead, it should reference the interface documentation, which should say that |ret_val| is -errno on error. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:15: int ErrnoToKernelRet(int errno_val); Let's keep the interface of trap handlers as returning -errno on error, even on MIPS. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:18: // returning |ret_val|. Let's add: |ret_val| must must follow the SandboxSyscall() convention of being -errno on errors. We should simply add this method to the new Syscall class, once it's available. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:34: // function in order for SandboxSyscall() to behave more like on othere nit: othere -> other https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:41: const intptr_t *args, Nit: indent (simply use "git cl format"). https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:159: sigsys_nr_is_bad = sigsys.nr != static_cast<int>(SECCOMP_SYSCALL(ctx)) && Let's just declare this bool here. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:162: sigsys_nr_is_bad = sigsys.nr != static_cast<int>(SECCOMP_SYSCALL(ctx)); And here.
Hey! Just making sure that you saw the previous update. Also FYI, I'll be leaving for a few weeks in a week and a half. It would be great if we can finish before then, otherwise I'll find someone to take over the review.
Hello, I made requested changes, so ErrnoToKernelRet() function is eliminated and PutValueInUcontext() function is moved to the new Syscall class. Appropriate kernel_return_value_helpers* files are removed. I hope that we will finish this before you leave :) Please take a look. Thanks, Nedeljko https://chromiumcodereview.appspot.com/260793003/diff/110001/content/common/s... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/content/common/s... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:104: return sandbox::ErrnoToKernelRet(EPERM); On 2014/06/13 02:47:36, jln wrote: > Let's just declare that trap handlers have the same interface as > SandboxSyscall() and return -errno. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc:26: if (ret_val < 0) { On 2014/06/13 02:47:36, jln wrote: > This should be if (retval <= -1 && ret_val >= -4095) (or we could define > MAX_ERRNO in syscall.h instead) and use that. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.cc:28: // SandboxSyscall() in case of Mips in order for SandboxSyscall() to On 2014/06/13 02:47:36, jln wrote: > This code must not have knowledge of the codepath that called it. > > Instead, it should reference the interface documentation, which should say that > |ret_val| is -errno on error. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:15: int ErrnoToKernelRet(int errno_val); On 2014/06/13 02:47:36, jln wrote: > Let's keep the interface of trap handlers as returning -errno on error, even on > MIPS. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/kernel_return_value_helpers.h:18: // returning |ret_val|. On 2014/06/13 02:47:36, jln wrote: > Let's add: |ret_val| must must follow the SandboxSyscall() convention of being > -errno on errors. > > We should simply add this method to the new Syscall class, once it's available. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:34: // function in order for SandboxSyscall() to behave more like on othere On 2014/06/13 02:47:36, jln wrote: > nit: othere -> other Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:41: const intptr_t *args, On 2014/06/13 02:47:36, jln wrote: > Nit: indent (simply use "git cl format"). Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:159: sigsys_nr_is_bad = sigsys.nr != static_cast<int>(SECCOMP_SYSCALL(ctx)) && On 2014/06/13 02:47:36, jln wrote: > Let's just declare this bool here. Done. https://chromiumcodereview.appspot.com/260793003/diff/110001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:162: sigsys_nr_is_bad = sigsys.nr != static_cast<int>(SECCOMP_SYSCALL(ctx)); On 2014/06/13 02:47:36, jln wrote: > And here. Done.
This is pretty much done. What remains: - Have petarj@ review syscall.cc - Some concern about indirct system calls - Checking that seccomp-bpf kernel support does the right thing and performs a copy of stack arguments to kernel space before seccomp-bpf runs. - Some minor nits https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/linux_seccomp.h:318: // The rest are on the stack. By the way, how does the kernel handle that? The kernel, of course, would need to copy these arguments before inspecting them with seccomp-bpf (otherwise another thread could switch them under it). Is it what it's doing? https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:103: intptr_t SandboxSyscallRaw(int nr, Let's add this as a private member of the class instead. https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:35: uint32_t next = iter.Next(); Assert that's its 0? https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:158: && sigsys.nr != static_cast<int>(SECCOMP_PARM1(ctx)); Nit: indent. (please use "git cl format") https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:187: SECCOMP_PARM6(ctx)); What happens in MIPS if something makes an indirect syscall with 6 parameters? Should MIPS take a 7th parameter? Or alternatively, should we hanadle indirect syscalls in a separate codepath here?
Requested changes done. Question regarding indirect syscalls with 6 arguments on MIPS still remains. Please take a look. Thanks, Nedeljko https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/linux_seccomp.h:318: // The rest are on the stack. On 2014/06/20 00:37:06, jln wrote: > By the way, how does the kernel handle that? > > The kernel, of course, would need to copy these arguments before inspecting them > with seccomp-bpf (otherwise another thread could switch them under it). > > Is it what it's doing? Yes, you are correct. Kernel copies arguments before inspecting them. https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:103: intptr_t SandboxSyscallRaw(int nr, On 2014/06/20 00:37:06, jln wrote: > Let's add this as a private member of the class instead. Done. https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:35: uint32_t next = iter.Next(); On 2014/06/20 00:37:06, jln wrote: > Assert that's its 0? Done. https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:158: && sigsys.nr != static_cast<int>(SECCOMP_PARM1(ctx)); On 2014/06/20 00:37:06, jln wrote: > Nit: indent. (please use "git cl format") Done. https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:187: SECCOMP_PARM6(ctx)); On 2014/06/20 00:37:06, jln wrote: > What happens in MIPS if something makes an indirect syscall with 6 parameters? > Should MIPS take a 7th parameter? > > Or alternatively, should we hanadle indirect syscalls in a separate codepath > here? For indirect syscall with 6 parameters MIPS should indeed take 7th parameter also. I am not sure what is best approach to handle this... I suppose that I could add one more parameter in Syscall::Call(), but should I create additional unittest in that case? And if so, this would be meaningful only on MIPS with O32 ABI, so this looks like an overkill. On the other hand I could create additional Syscall::IndirectCall() function to handle this, but should this new function then be used in all the places where indirect syscall can occur? Or maybe, since this problem is quite specific and it is connected only with MIPS and then only with O32 ABI and since there is no particular gain in using indirect syscall, I can disallow indirect syscalls with 6 arguments here by checking if it is not zero if indirect syscall is made and handling this case. That would have the smallest impact on the code. What do you think?
On 2014/06/20 00:37:07, jln wrote: > This is pretty much done. What remains: > > - Have petarj@ review syscall.cc lgtm for syscall.cc with the following remark: this code is valid only for system calls up to 6 parameters and each of the arguments has to be 32-bit value. So, for instance, pread64 can not be executed.
https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/trap.cc (right): https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... sandbox/linux/seccomp-bpf/trap.cc:187: SECCOMP_PARM6(ctx)); On 2014/06/20 14:09:51, nedeljko wrote: > On 2014/06/20 00:37:06, jln wrote: > > What happens in MIPS if something makes an indirect syscall with 6 parameters? > > Should MIPS take a 7th parameter? > > > > Or alternatively, should we hanadle indirect syscalls in a separate codepath > > here? > > For indirect syscall with 6 parameters MIPS should indeed take 7th parameter > also. > > I am not sure what is best approach to handle this... > > I suppose that I could add one more parameter in Syscall::Call(), but should I > create additional unittest in that case? And if so, this would be meaningful > only on MIPS with O32 ABI, so this looks like an overkill. > > On the other hand I could create additional Syscall::IndirectCall() function to > handle this, but should this new function then be used in all the places where > indirect syscall can occur? > > Or maybe, since this problem is quite specific and it is connected only with > MIPS and then only with O32 ABI and since there is no particular gain in using > indirect syscall, I can disallow indirect syscalls with 6 arguments here by > checking if it is not zero if indirect syscall is made and handling this case. > That would have the smallest impact on the code. > > What do you think? What is the maximum number of parameters for a Linux MIPS system call? Is it 6 except for indirect system calls (so 7) or is it more? petarj also mentioned 64 bits arguments, but these simply get split as two 32 bits arguments, correct? I think we should add support for more arguments. I'll add a test for pread64 and you can simply make sure it does pass. If that seems good, I can add a CL to change the syscall.h interface to support an extra parameter on certain achitectures and you can just make sure it works on MIPS. https://chromiumcodereview.appspot.com/260793003/diff/150001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/syscall.h (right): https://chromiumcodereview.appspot.com/260793003/diff/150001/sandbox/linux/se... sandbox/linux/seccomp-bpf/syscall.h:90: DISALLOW_IMPLICIT_CONSTRUCTORS(Syscall); This should always be at the bottom of the private section (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Format)
On 2014/06/20 15:16:24, petarj wrote: > On 2014/06/20 00:37:07, jln wrote: > > This is pretty much done. What remains: > > > > - Have petarj@ review syscall.cc > > lgtm for syscall.cc with the following remark: > > this code is valid only for system calls up to 6 parameters and each > of the arguments has to be 32-bit value. So, for instance, pread64 can > not be executed. Ahh yeah, I was just discussing that in https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... How many parameters can Linux MIPS support at max? Also for system calls that take 64 bits parameters (such as pread64()), the 64 bits parameter is simply sent to the kernel as two separate 32 bits parameters, right?
On 2014/06/20 21:19:40, jln wrote: > On 2014/06/20 15:16:24, petarj wrote: > > On 2014/06/20 00:37:07, jln wrote: > > > This is pretty much done. What remains: > > > > > > - Have petarj@ review syscall.cc > > > > lgtm for syscall.cc with the following remark: > > > > this code is valid only for system calls up to 6 parameters and each > > of the arguments has to be 32-bit value. So, for instance, pread64 can > > not be executed. > > Ahh yeah, I was just discussing that in > https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... > > How many parameters can Linux MIPS support at max? > sys_syscall will take up to 8 arguments. > Also for system calls that take 64 bits parameters (such as pread64()), the 64 > bits parameter is simply sent to the kernel as two separate 32 bits parameters, > right? Not simply. The value is passed either in even-odd register pair or on stack. Making a layout for different system calls with differently sized arguments is more complex than this. This is something valid for ARM too. linux-syscall-support can stand as a reference. Regards, Petar
On 2014/06/21 01:21:18, petarj wrote: > On 2014/06/20 21:19:40, jln wrote: > > On 2014/06/20 15:16:24, petarj wrote: > > > On 2014/06/20 00:37:07, jln wrote: > > > > This is pretty much done. What remains: > > > > > > > > - Have petarj@ review syscall.cc > > > > > > lgtm for syscall.cc with the following remark: > > > > > > this code is valid only for system calls up to 6 parameters and each > > > of the arguments has to be 32-bit value. So, for instance, pread64 can > > > not be executed. > > > > Ahh yeah, I was just discussing that in > > > https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... > > > > How many parameters can Linux MIPS support at max? > > > > sys_syscall will take up to 8 arguments. I have no problem in changing the assembly to take more arguments once the interface is prepared for this. > > > Also for system calls that take 64 bits parameters (such as pread64()), the 64 > > bits parameter is simply sent to the kernel as two separate 32 bits > parameters, > > right? > > Not simply. The value is passed either in even-odd register pair or on > stack. Making a layout for different system calls with differently sized > arguments is more complex than this. This is something valid for ARM too. > > linux-syscall-support can stand as a reference. > > Regards, > Petar I was writing assembly so it complies to what was already done for other currently supported architectures and current interface in syscall.h. If we need to support all possible combinations of syscalls and arguments, then some significant changes should be done to both assembly and interface. We indeed need to do something similar to what was done in linux-syscall-support. Maybe the best approach would be to use linux_syscall_support.h since all needed implementations are already there and just to change syscall.h interface so it can use linux_syscall_support.h (especially since this approach can solve problems for some other architectures also)? Regards, Nedeljko
On 2014/06/23 10:33:50, nedeljko wrote: > On 2014/06/21 01:21:18, petarj wrote: > > On 2014/06/20 21:19:40, jln wrote: > > > On 2014/06/20 15:16:24, petarj wrote: > > > > On 2014/06/20 00:37:07, jln wrote: > > > > > This is pretty much done. What remains: > > > > > > > > > > - Have petarj@ review syscall.cc > > > > > > > > lgtm for syscall.cc with the following remark: > > > > > > > > this code is valid only for system calls up to 6 parameters and each > > > > of the arguments has to be 32-bit value. So, for instance, pread64 can > > > > not be executed. > > > > > > Ahh yeah, I was just discussing that in > > > > > > https://chromiumcodereview.appspot.com/260793003/diff/130001/sandbox/linux/se... > > > > > > How many parameters can Linux MIPS support at max? > > > > > > > sys_syscall will take up to 8 arguments. > > I have no problem in changing the assembly to take more arguments once > the interface is prepared for this. > > > > > > Also for system calls that take 64 bits parameters (such as pread64()), the > 64 > > > bits parameter is simply sent to the kernel as two separate 32 bits > > parameters, > > > right? > > > > Not simply. The value is passed either in even-odd register pair or on > > stack. Making a layout for different system calls with differently sized > > arguments is more complex than this. This is something valid for ARM too. > > > > linux-syscall-support can stand as a reference. > > > > Regards, > > Petar > > I was writing assembly so it complies to what was already done for other > currently supported architectures and current interface in syscall.h. > > If we need to support all possible combinations of syscalls and arguments, > then some significant changes should be done to both assembly and > interface. We indeed need to do something similar to what was done in > linux-syscall-support. > > Maybe the best approach would be to use linux_syscall_support.h since all > needed implementations are already there and just to change syscall.h > interface so it can use linux_syscall_support.h (especially since this > approach can solve problems for some other architectures also)? > > Regards, > Nedeljko Ping. How shall we proceed with this? Thanks, Nedeljko
On 2014/06/26 08:10:25, nedeljko wrote: > Ping. > > How shall we proceed with this? > > > Thanks, > Nedeljko On 2014/06/26 08:10:25, nedeljko wrote: > How shall we proceed with this? I'm sorry, running behind on everything :( I would rather avoid using LSS. It's barely maintained and has a number of issues. I think it's perfectly ok for now if it's hard to perform a syscall such as pread64 manually with Syscall::Call(). We can consider Syscall::Call() as a very low level interface. What should work though is ForwardSyscall(). If I'm not mistaken, there wouldn't be much work needed for this. Only adding support for 8 native-size parameters in Syscall::Call() and making sure they can all get forwarded from a trap handler. I'm adding a test for pread64 forwarding in https://chromiumcodereview.appspot.com/348853006/ I will try to send a CL for adding space for 8 parameters to the Syscall() class today, but if I fail, it should be easy to do: - Make the main Syscall::Call() method private. Keep the templated methods public. - Make Syscall::Call() take up to 8 parameters - Have the templated methods take a number of parameters that makes sense for the architecture. What do you think? mdempsky@ volunteered being a point of contact for this CL for the next two weeks while I'm away. I've cc-ed him to this CL.
On 2014/06/26 19:22:01, jln (OOO til July 15th) wrote: > On 2014/06/26 08:10:25, nedeljko wrote: > > > Ping. > > > > How shall we proceed with this? > > > > > > Thanks, > > Nedeljko > > On 2014/06/26 08:10:25, nedeljko wrote: > > > How shall we proceed with this? > > I'm sorry, running behind on everything :( > > I would rather avoid using LSS. It's barely maintained and has a number of > issues. > > I think it's perfectly ok for now if it's hard to perform a syscall such as > pread64 manually with Syscall::Call(). We can consider Syscall::Call() as a very > low level interface. What should work though is ForwardSyscall(). If I'm not > mistaken, there wouldn't be much work needed for this. Only adding support for 8 > native-size parameters in Syscall::Call() and making sure they can all get > forwarded from a trap handler. > > I'm adding a test for pread64 forwarding in > https://chromiumcodereview.appspot.com/348853006/ > > I will try to send a CL for adding space for 8 parameters to the Syscall() class > today, but if I fail, it should be easy to do: > > - Make the main Syscall::Call() method private. Keep the templated methods > public. > - Make Syscall::Call() take up to 8 parameters > - Have the templated methods take a number of parameters that makes sense for > the architecture. > > What do you think? > > mdempsky@ volunteered being a point of contact for this CL for the next two > weeks while I'm away. I've cc-ed him to this CL. I am ok with this. I run test for pread64 that you added and it is working correctly on mips. So, let me see if I understand you correctly what needs to be done: 1. Create CL for adding space for 8 parameters to the Syscall() class. I can do this if you don't have time. 2. Regarding this CL, I need to enable 8 parameters in asm and to enable passing 8 parameters in trap.cc for Mips after step 1 is done (and of course to move DISALLOW_IMPLICIT_CONSTRUCTORS(Syscall); to the end of the private section). Are these two steps enough for this CL to be accepted, or am I missing something? Thanks, Nedeljko
On 2014/06/27 14:11:50, nedeljko wrote: > I am ok with this. > > I run test for pread64 that you added and it is working correctly on mips. > > So, let me see if I understand you correctly what needs to be done: > > 1. Create CL for adding space for 8 parameters to the Syscall() class. > I can do this if you don't have time. > 2. Regarding this CL, I need to enable 8 parameters in asm and to enable passing > > 8 parameters in trap.cc for Mips after step 1 is done > (and of course to move DISALLOW_IMPLICIT_CONSTRUCTORS(Syscall); to the end of > the private section). > > Are these two steps enough for this CL to be accepted, or am I missing > something? Ok, I'll let you take care of (1) since I'm running out of time (try to follow the steps outline above, with the main Syscall::Call() being private. I think this is roughly what's needed to complete this CL indeed. mdempsky@ will take over the review while I'm away.
Usage of 8 arguments for syscall enabled for Mips. However, more than six arguments can only be used for UnsafeTrap() handler. Seccomp bpf can check only 6 arguments, so enabling 8 arguments for it doesn't make sense. Please have a look. Thanks. https://codereview.chromium.org/260793003/diff/150001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/syscall.h (right): https://codereview.chromium.org/260793003/diff/150001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.h:90: DISALLOW_IMPLICIT_CONSTRUCTORS(Syscall); On 2014/06/20 21:18:18, jln (OOO til July 15th) wrote: > This should always be at the bottom of the private section > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Format) Done.
Ping. Is this change ok? Thanks, Nedeljko
Sorry for the delays. Seems generally okay to me, just a handful of nits I noticed. https://codereview.chromium.org/260793003/diff/190001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/260793003/diff/190001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:58: #elif defined(__i386__) || defined(__mips__) Adding "|| defined(__mips__)" here doesn't really make sense, since this case will be caught by the #if above. Also, does MIPS really use socketcall()? socketcall() passes all of its arguments indirectly, so we can't filter it in seccomp-BPF. If it actually supports accept(), setsockopt(), etc., then it would be preferable to only allow those I think. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc:247: case __NR_ioperm: // Intel privilege. Do these comments need to be updated? https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc:494: case __NR_vm86: Doesn't really matter since it's disallowed here anyway, but is vm86 really a system call on MIPS? The Linux man page makes it sound like its x86-32 only. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/linux_seccomp.h:27: // sys/user.h in eglibc misses size_t definition If you only need size_t, I'd suggest including <stddef.h> instead. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/linux_seccomp.h:321: #define SECCOMP_PARM5(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, \ How about: #define SECCOMP_STACKPARM(_ctx, n) (((long *)SECCOMP_REG(_ctx, 29))[(n)]) #define SECCOMP_PARM6(_ctx) SECCOMP_STACKPARM(_ctx, 4) #define SECCOMP_PARM7(_ctx) SECCOMP_STACKPARM(_ctx, 5) ... https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:666: BPF_ASSERT(socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0); Why switch from a pipe to a socket pair? https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1995: // TODO: Figure out how to support specificity of handeling indirect syscalls Typo: handling https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.cc:199: "sw $t0, 28($sp)\n" Does MIPS still need load delay slots? Would it be worth tweaking this code like: lw $a3, 28($a0) lw $a2, 24($a0) lw $a1, 20($a0) lw $t0, 16($a0) sw $a3, 28($sp) sw $a2, 24($sp) sw $a1, 20($sp) sw $t0, 16($sp) lw $a3, 12($a0) lw $a2, 8($a0) lw $a1, 4($a0) lw $a0, 0($a0) to reduce memory stalls? https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/syscall.h (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.h:141: // handelling syscall errors, SandboxSyscall() is made as a wrapper for this typo: handling https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.h:142: // function in order for SandboxSyscall() to behave more like on othere typo: other https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/trap.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/trap.cc:212: static_cast<int> SECCOMP_SYSCALL(ctx), Please include parenthesis for the static cast here.
Please have a look. Thanks. https://codereview.chromium.org/260793003/diff/190001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/260793003/diff/190001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:58: #elif defined(__i386__) || defined(__mips__) On 2014/07/14 18:10:50, mdempsky wrote: > Adding "|| defined(__mips__)" here doesn't really make sense, since this case > will be caught by the #if above. > > Also, does MIPS really use socketcall()? socketcall() passes all of its > arguments indirectly, so we can't filter it in seccomp-BPF. If it actually > supports accept(), setsockopt(), etc., then it would be preferable to only allow > those I think. Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc:247: case __NR_ioperm: // Intel privilege. On 2014/07/14 18:10:50, mdempsky wrote: > Do these comments need to be updated? No they don't. I will remove defined(__mips__) from here. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc:494: case __NR_vm86: On 2014/07/14 18:10:50, mdempsky wrote: > Doesn't really matter since it's disallowed here anyway, but is vm86 really a > system call on MIPS? The Linux man page makes it sound like its x86-32 only. There is a definition for this system call, but it returns ENOSYS, so I will remove defined(__mips__) here. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/linux_seccomp.h (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/linux_seccomp.h:27: // sys/user.h in eglibc misses size_t definition On 2014/07/14 18:10:50, mdempsky wrote: > If you only need size_t, I'd suggest including <stddef.h> instead. Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/linux_seccomp.h:321: #define SECCOMP_PARM5(_ctx) (long int)(*((intptr_t*)SECCOMP_REG(_ctx, \ On 2014/07/14 18:10:50, mdempsky wrote: > How about: > > #define SECCOMP_STACKPARM(_ctx, n) (((long *)SECCOMP_REG(_ctx, 29))[(n)]) > #define SECCOMP_PARM6(_ctx) SECCOMP_STACKPARM(_ctx, 4) > #define SECCOMP_PARM7(_ctx) SECCOMP_STACKPARM(_ctx, 5) > ... Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:666: BPF_ASSERT(socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0); On 2014/07/14 18:10:51, mdempsky wrote: > Why switch from a pipe to a socket pair? In short, there is a problem in using pipe() here on Mips because of they way it is implemented on Mips. Please see comments from Patch Set 2 for more detailed explanation. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1995: // TODO: Figure out how to support specificity of handeling indirect syscalls On 2014/07/14 18:10:51, mdempsky wrote: > Typo: handling Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/syscall.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.cc:199: "sw $t0, 28($sp)\n" On 2014/07/14 18:10:51, mdempsky wrote: > Does MIPS still need load delay slots? Would it be worth tweaking this code > like: > > lw $a3, 28($a0) > lw $a2, 24($a0) > lw $a1, 20($a0) > lw $t0, 16($a0) > sw $a3, 28($sp) > sw $a2, 24($sp) > sw $a1, 20($sp) > sw $t0, 16($sp) > lw $a3, 12($a0) > lw $a2, 8($a0) > lw $a1, 4($a0) > lw $a0, 0($a0) > > to reduce memory stalls? Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/syscall.h (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.h:141: // handelling syscall errors, SandboxSyscall() is made as a wrapper for this On 2014/07/14 18:10:51, mdempsky wrote: > typo: handling Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/syscall.h:142: // function in order for SandboxSyscall() to behave more like on othere On 2014/07/14 18:10:51, mdempsky wrote: > typo: other Done. https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf/trap.cc (right): https://codereview.chromium.org/260793003/diff/190001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf/trap.cc:212: static_cast<int> SECCOMP_SYSCALL(ctx), On 2014/07/14 18:10:51, mdempsky wrote: > Please include parenthesis for the static cast here. Done.
lgtm It looks like there are a couple of other system calls that probably aren't actually needed by MIPS (e.g., modify_ldt) and the related socketcall() helpers probably aren't needed on MIPS either if we're not going to allow it, but I think this is at least good enough to submit and we can fix those later.
The CQ bit was checked by mdempsky@chromium.org
The CQ bit was unchecked by mdempsky@chromium.org
The CQ bit was checked by nedeljko.babic@imgtec.com
The CQ bit was unchecked by nedeljko.babic@imgtec.com
The CQ bit was checked by nedeljko.babic@imgtec.com
The CQ bit was unchecked by nedeljko.babic@imgtec.com
The CQ bit was checked by nedeljko.babic@imgtec.com
On 2014/07/17 12:06:24, nedeljko wrote: > The CQ bit was checked by mailto:nedeljko.babic@imgtec.com Should I do something else in order for commit-bot to take over? Thanks.
The CQ bit was unchecked by mdempsky@chromium.org
The CQ bit was checked by mdempsky@chromium.org
On 2014/07/18 10:44:54, nedeljko wrote: > Should I do something else in order for commit-bot to take over? Hm, I thought it should "just work". I've re-checked it myself, and if that doesn't do anything, I'll escalate to chrome-infra to figure out what's going on.
On 2014/07/18 17:08:27, mdempsky wrote: > On 2014/07/18 10:44:54, nedeljko wrote: > > Should I do something else in order for commit-bot to take over? > > Hm, I thought it should "just work". I've re-checked it myself, and if that > doesn't do anything, I'll escalate to chrome-infra to figure out what's going > on. Could the problem be in that my base URL is wrong? My base URL is the base URL from before code migrated to new location. I continue to use this URL since the branch from which I made first commit is in repo connected to this URL. If this is the problem, should I resend Patch Set from repo clone with new base URL, or is there some more elegant solution for this? Can I connect branch from another repo with this CL? Thanks
On 2014/07/21 15:45:18, nedeljko wrote: > On 2014/07/18 17:08:27, mdempsky wrote: > > On 2014/07/18 10:44:54, nedeljko wrote: > > > Should I do something else in order for commit-bot to take over? > > > > Hm, I thought it should "just work". I've re-checked it myself, and if that > > doesn't do anything, I'll escalate to chrome-infra to figure out what's going > > on. > > Could the problem be in that my base URL is wrong? > > My base URL is the base URL from before code migrated to new location. > > I continue to use this URL since the branch from which I made first commit is in > repo connected to this URL. > > If this is the problem, should I resend Patch Set from repo clone with new base > URL, or is there some more elegant solution for this? > > Can I connect branch from another repo with this CL? > > Thanks Yeah, this might be your problem. You can try to change your remote repo from git.chromium.org to chromium.googlesource.com See https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... I don't know why I didn't get migrated automatically. Stefan, any idea? What's the best way for him to migrate his CL?
On 2014/07/21 17:06:08, jln wrote: > On 2014/07/21 15:45:18, nedeljko wrote: > > On 2014/07/18 17:08:27, mdempsky wrote: > > > On 2014/07/18 10:44:54, nedeljko wrote: > > > > Should I do something else in order for commit-bot to take over? > > > > > > Hm, I thought it should "just work". I've re-checked it myself, and if that > > > doesn't do anything, I'll escalate to chrome-infra to figure out what's > going > > > on. > > > > Could the problem be in that my base URL is wrong? > > > > My base URL is the base URL from before code migrated to new location. > > > > I continue to use this URL since the branch from which I made first commit is > in > > repo connected to this URL. > > > > If this is the problem, should I resend Patch Set from repo clone with new > base > > URL, or is there some more elegant solution for this? > > > > Can I connect branch from another repo with this CL? > > > > Thanks > > Yeah, this might be your problem. You can try to change your remote repo from > http://git.chromium.org to http://chromium.googlesource.com > > See > https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... > > I don't know why I didn't get migrated automatically. Stefan, any idea? What's > the best way for him to migrate his CL? I already changed remote repo location in .git/config few weeks ago, but that change was not propagated to CL.
On 2014/07/22 11:58:46, nedeljko wrote: > On 2014/07/21 17:06:08, jln wrote: > > On 2014/07/21 15:45:18, nedeljko wrote: > > > On 2014/07/18 17:08:27, mdempsky wrote: > > > > On 2014/07/18 10:44:54, nedeljko wrote: > > > > > Should I do something else in order for commit-bot to take over? > > > > > > > > Hm, I thought it should "just work". I've re-checked it myself, and if > that > > > > doesn't do anything, I'll escalate to chrome-infra to figure out what's > > going > > > > on. > > > > > > Could the problem be in that my base URL is wrong? > > > > > > My base URL is the base URL from before code migrated to new location. > > > > > > I continue to use this URL since the branch from which I made first commit > is > > in > > > repo connected to this URL. > > > > > > If this is the problem, should I resend Patch Set from repo clone with new > > base > > > URL, or is there some more elegant solution for this? > > > > > > Can I connect branch from another repo with this CL? > > > > > > Thanks > > > > Yeah, this might be your problem. You can try to change your remote repo from > > http://git.chromium.org to http://chromium.googlesource.com > > > > See > > > https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... > > > > I don't know why I didn't get migrated automatically. Stefan, any idea? What's > > the best way for him to migrate his CL? > > I already changed remote repo location in .git/config few weeks ago, but that > change was not propagated to CL. You could ask chromium-dev@ for help. Or the alternative is to simply create a new CL (git cl issue 0 && git cl upload) (please make sure that it's strictly identical to this one. If you need to rebase, please upload as-is at first and then upload another revision for the rebase) and we can approve that one instead.
On 2014/07/22 21:21:13, jln wrote: > > You could ask chromium-dev@ for help. > > Or the alternative is to simply create a new CL (git cl issue 0 && git cl > upload) (please make sure that it's strictly identical to this one. If you need > to rebase, please upload as-is at first and then upload another revision for the > rebase) and we can approve that one instead. New CL (409403003) created as suggested: https://codereview.chromium.org/409403003/ Please have a look. Should I close this CL, or wait until new CL is merged? Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nedeljko.babic@imgtec.com/260793003/21...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Yeah, let's close (but not delete) this one. (It looks like the CQ is finally working actually... but never mind). |
