SECCOMP-BPF: Fix SandboxSyscall()

SECCOMP-BPF: Fix SandboxSyscall()

- Eliminate variadic arguments in favor of C++ templates.
  This makes ASAN and Valgrind much happier, as we are no
  longer accessing more arguments than what has been passed
  into our function (i.e. in the past, we'd always forward
  six arguments to the kernel, even if the system call
  needed fewer; now, we explicitly pass zeros).

- In the past, callers had to be very careful when passing
  NULL, as the C++ compiler was likely to treat this macro
  as a 32bit integer value rather than a 64bit pointer. We
  now always perform sign extension for expanding arguments
  to the full native word width.

- On x86-64, we could clobber up to eight (in some cases 16)
  bytes in the red zone. This would typically only happen
  when high optimization levels were turned on, and in many
  cases it ended up overwriting data that was no longer
  needed. But we have seen at least one case where we ended
  up clobbering a system call parameter. We now explicitly
  avoid the red zone and this problem can no longer happen.

BUG=163904, 162925
TEST=sandbox_linux_unittests
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170896

[Markus (顧孟勤), 2012-12-03 21:30:07]

Hopefully, this CL will take care of all the bugs we have received from both
ASAN and Valgrind. If it doesn't, then we probably do have a genuine bug
somewhere -- but my careful inspection of lots of disassembler output suggests
that we are doing the right thing everywhere now.

[jln (very slow on Chromium), 2012-12-03 21:54:43]

On 2012/12/03 21:30:07, Markus (顧孟勤) wrote:
> Hopefully, this CL will take care of all the bugs we have received from both
> ASAN and Valgrind. If it doesn't, then we probably do have a genuine bug
> somewhere -- but my careful inspection of lots of disassembler output suggests
> that we are doing the right thing everywhere now.

This looks good to me and it's definitely much nicer than the previous version.

However I'm not the right person to review it. Jeffrey, would you mind taking a
look ?

[Jeffrey Yasskin, 2012-12-03 22:27:35]

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:188: if (sizeof(void *) !=
sizeof(intptr_t)) {
You can compile-assert this. If you don't have COMPILE_ASSERT from basictypes.h
here, use
  typedef char
pointer_and_intptr_t_have_different_widths[(sizeof(void*)==sizeof(intptr_t)) ? 1
: -1];

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:192: const intptr_t args[6] = { p0, p1, p2,
p3, p4, p5 };
If the kernel could ever write on one of these arguments, you shouldn't define
the array elements as const.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t p0, intptr_t p1, intptr_t p2,
The way StrCat() and RE2 do this is to define a class like:

struct SyscallArg {
  SyscallArg(intptr_t i) : val(i) {}
  SyscallArg(const void* ptr) : val(reinterpret_cast<intptr_t>(ptr)) {}

  intptr_t val;
};

And then:
intptr_t SandboxSyscall(int nr,
    SyscallArg a0=SyscallArg(0), SyscallArg a1=SyscallArg(0),
    SyscallArg a2=SyscallArg(0), SyscallArg a3=SyscallArg(0),
    SyscallArg a4=SyscallArg(0), SyscallArg a5=SyscallArg(0));

This won't let you pass objects that have implicit conversions to intptr_t or
pointer types, but you couldn't generally pass those through ... anyway since
they're usually non-POD.

I don't have any problem with your current implementation; I'm just mentioning
this in case you prefer it.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:32: // We have to use C-style cast operators
as we want to be able to accept both
You could use reinterpret_cast<intptr_t>(p0), etc. That nicely flags the fact
that you could be casting pointers here, although you've also commented that, so
it doesn't help a huge amount.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:41: __attribute__((always_inline));
Is there a semantic reason this attribute is needed, or is it just for
performance? The compiler will almost always inline such short functions, so a
future maintainer will likely want to remove the attribute, so if it's needed
for correctness you should comment that, and if it's just for performance, you
should probably remove it.

[Jeffrey Yasskin, 2012-12-03 22:40:16]

https://codereview.chromium.org/11416326/diff/1/sandbox/linux/seccomp-bpf/sys...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://codereview.chromium.org/11416326/diff/1/sandbox/linux/seccomp-bpf/sys...
sandbox/linux/seccomp-bpf/syscall.cc:182: // that this would only be an issue
for IA64, which we are currently not
Apparently intptr_t is _not_ required to be big enough to hold a void(*)(), so
it's even less likely that it'll ever be a different size from void*, even on
IA64 where function pointers are bigger. It's still worth including the size
assertion, of course.

[Markus (顧孟勤), 2012-12-04 00:54:38]

Jeffrey, I think almost all of your comments were informational -- and I always
appreciate learning more about C++.

I addressed the two comments that seemed to ask for action. If I missed
anything, please let me know.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:182: // that this would only be an issue
for IA64, which we are currently not
I am actually somewhat positive this would work on architectures such as IA64.
But I really have no way to say for sure.

Also, just out of curiosity, do you happen to know if function pointers are
truly bigger than regular pointers. Or are they just pointers that point to a
descriptor rather than to the actual function. The information that I found
online seems to suggest that pointing to a statically allowed descriptor would
work fine in all cases, except for code that implements a JIT.

On 2012/12/03 22:40:16, Jeffrey Yasskin wrote:
> Apparently intptr_t is _not_ required to be big enough to hold a void(*)(), so
> it's even less likely that it'll ever be a different size from void*, even on
> IA64 where function pointers are bigger. It's still worth including the size
> assertion, of course.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:188: if (sizeof(void *) !=
sizeof(intptr_t)) {
Sweet. Yes, that's exactly what I needed here. I had forgotten about this trick.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> You can compile-assert this. If you don't have COMPILE_ASSERT from
basictypes.h
> here, use
>   typedef char
> pointer_and_intptr_t_have_different_widths[(sizeof(void*)==sizeof(intptr_t)) ?
1
> : -1];

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:192: const intptr_t args[6] = { p0, p1, p2,
p3, p4, p5 };
On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> If the kernel could ever write on one of these arguments, you shouldn't define
> the array elements as const.

If you carefully look at the assembly code, you'll see that we are copying from
this array to CPU registers, but we are never copying back and neither do we
pass the address of this array to the kernel. so, I think we are OK marking the
array as const.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t p0, intptr_t p1, intptr_t p2,
Yes, that would have worked too. I don't think it really buys us anything, and
it makes the code even more verbose and IMHO slightly harder to read. But I do
defer to your expertise, if for some reason you prefer this approach, I am happy
to switch to it.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> The way StrCat() and RE2 do this is to define a class like:
> 
> struct SyscallArg {
>   SyscallArg(intptr_t i) : val(i) {}
>   SyscallArg(const void* ptr) : val(reinterpret_cast<intptr_t>(ptr)) {}
> 
>   intptr_t val;
> };
> 
> And then:
> intptr_t SandboxSyscall(int nr,
>     SyscallArg a0=SyscallArg(0), SyscallArg a1=SyscallArg(0),
>     SyscallArg a2=SyscallArg(0), SyscallArg a3=SyscallArg(0),
>     SyscallArg a4=SyscallArg(0), SyscallArg a5=SyscallArg(0));
> 
> This won't let you pass objects that have implicit conversions to intptr_t or
> pointer types, but you couldn't generally pass those through ... anyway since
> they're usually non-POD.
> 
> I don't have any problem with your current implementation; I'm just mentioning
> this in case you prefer it.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:32: // We have to use C-style cast operators
as we want to be able to accept both
I am not sure, I fully understand your comment.

I am concerned that using C++-style comments, I will either warn me because a) I
am converting a pointer to an integer type, or b) converting a 32bit value to a
64bit value. I am a little weak on understanding all the different cast
operators, but I don't think there is one that allows both operations.

Maybe, that's the reason why StrCat() went with the more complicated solution
that has different function prototypes.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> You could use reinterpret_cast<intptr_t>(p0), etc. That nicely flags the fact
> that you could be casting pointers here, although you've also commented that,
so
> it doesn't help a huge amount.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:41: __attribute__((always_inline));
I'll add a comment. It is merely for the convenience of people looking at stack
traces in debug builds.

It seems, debug builds don't do the automatic inlining here, and it exposes
confusing implementation details in stack traces. I want to spare people from
trying to understand what is happening here, when they really couldn't care less
about the implementation of our system call API.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> Is there a semantic reason this attribute is needed, or is it just for
> performance? The compiler will almost always inline such short functions, so a
> future maintainer will likely want to remove the attribute, so if it's needed
> for correctness you should comment that, and if it's just for performance, you
> should probably remove it.

[Jeffrey Yasskin, 2012-12-04 01:18:48]

lgtm

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:182: // that this would only be an issue
for IA64, which we are currently not
On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:
> I am actually somewhat positive this would work on architectures such as IA64.
> But I really have no way to say for sure.
> 
> Also, just out of curiosity, do you happen to know if function pointers are
> truly bigger than regular pointers. Or are they just pointers that point to a
> descriptor rather than to the actual function. The information that I found
> online seems to suggest that pointing to a statically allowed descriptor would
> work fine in all cases, except for code that implements a JIT.

I don't know for sure what IA64 does for function pointers.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.cc:192: const intptr_t args[6] = { p0, p1, p2,
p3, p4, p5 };
On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:
> On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> > If the kernel could ever write on one of these arguments, you shouldn't
define
> > the array elements as const.
> 
> If you carefully look at the assembly code, you'll see that we are copying
from
> this array to CPU registers, but we are never copying back and neither do we
> pass the address of this array to the kernel. so, I think we are OK marking
the
> array as const.

Ok, sounds good.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t p0, intptr_t p1, intptr_t p2,
Nope, what you have is fine with me.

On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:
> Yes, that would have worked too. I don't think it really buys us anything, and
> it makes the code even more verbose and IMHO slightly harder to read. But I do
> defer to your expertise, if for some reason you prefer this approach, I am
happy
> to switch to it.
> 
> On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> > The way StrCat() and RE2 do this is to define a class like:
> > 
> > struct SyscallArg {
> >   SyscallArg(intptr_t i) : val(i) {}
> >   SyscallArg(const void* ptr) : val(reinterpret_cast<intptr_t>(ptr)) {}
> > 
> >   intptr_t val;
> > };
> > 
> > And then:
> > intptr_t SandboxSyscall(int nr,
> >     SyscallArg a0=SyscallArg(0), SyscallArg a1=SyscallArg(0),
> >     SyscallArg a2=SyscallArg(0), SyscallArg a3=SyscallArg(0),
> >     SyscallArg a4=SyscallArg(0), SyscallArg a5=SyscallArg(0));
> > 
> > This won't let you pass objects that have implicit conversions to intptr_t
or
> > pointer types, but you couldn't generally pass those through ... anyway
since
> > they're usually non-POD.
> > 
> > I don't have any problem with your current implementation; I'm just
mentioning
> > this in case you prefer it.

https://chromiumcodereview.appspot.com/11416326/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/syscall.h:32: // We have to use C-style cast operators
as we want to be able to accept both
Ah, yes, reinterpret_cast can't convert between different integer types, so the
C-style cast is required.

On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:
> I am not sure, I fully understand your comment.
> 
> I am concerned that using C++-style comments, I will either warn me because a)
I
> am converting a pointer to an integer type, or b) converting a 32bit value to
a
> 64bit value. I am a little weak on understanding all the different cast
> operators, but I don't think there is one that allows both operations.
> 
> Maybe, that's the reason why StrCat() went with the more complicated solution
> that has different function prototypes.
> 
> On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:
> > You could use reinterpret_cast<intptr_t>(p0), etc. That nicely flags the
fact
> > that you could be casting pointers here, although you've also commented
that,
> so
> > it doesn't help a huge amount.
>

[jln (very slow on Chromium), 2012-12-04 04:09:36]

lgtm

Sadly, linux_asan does not include sandbox_linux_unittests (which I should
change ASAP), but I tested locally.

[commit-bot: I haz the power, 2012-12-04 04:13:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/11416326/9001

[jln (very slow on Chromium), 2012-12-04 06:46:48]

On 2012/12/04 04:13:43, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/markus%40chromium.org/11416326/9001

mac_rel has unrelated failure. Committing directly?

[commit-bot: I haz the power, 2012-12-04 06:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/11416326/9001

[commit-bot: I haz the power, 2012-12-04 06:48:02]

Change committed as 170896