MinSFI: Passing arguments to the entry function

Added support for passing arguments to MinSFI's entry function

This patch modifies the MinSFI invocation interface to accept arguments
in the standard UNIX argc,argv format. The runtime copies these onto
the untrusted stack and invokes the _start_minsfi function with a pointer
to the beginning of the data structure. The sandboxed code will use it
to access the arguments but also to initialize its stack pointer.

The CL also introduces the untrusted support library and functions for
converting between native and untrusted pointers.

BUG= https://code.google.com/p/chromium/issues/detail?id=397177
TEST= ./scons bitcode=1 minsfi=1 minsfi_tests
NOTRY=true

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=13728

[dbrazdil, 2014-09-10 00:04:33]

Patchset #1 (id:1) has been deleted

[dbrazdil, 2014-09-10 00:30:42]

Patchset #1 (id:20001) has been deleted

[dbrazdil, 2014-09-10 02:18:03]

Patchset #1 (id:40001) has been deleted

[dbrazdil, 2014-09-10 15:28:27]

Patchset #1 (id:60001) has been deleted

[dbrazdil, 2014-09-10 15:31:30]

dbrazdil@google.com changed reviewers:
+ jvoung@chromium.org, mseaborn@chromium.org

[dbrazdil, 2014-09-10 15:31:31]

This won't pass tests until the LLVM deps are updated, but it's ready.

[jvoung (off chromium), 2014-09-10 17:04:30]

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv.h
File src/include/minsfi_priv.h (right):

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv....
src/include/minsfi_priv.h:15: /* Definition of the untrusted pointer type */
It seems more like this is analogous to uintptr_t -- an integer type capable of
holding a value converted from an untrusted pointer, and can be converted back
without loss of information, etc.

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv....
src/include/minsfi_priv.h:20: uint32_t dataseg_offset;
should this be an sfiptr_t also?

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_ptr.h
File src/include/minsfi_ptr.h (right):

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_ptr.h...
src/include/minsfi_ptr.h:39: static inline sfiptr_t ToMinsfiPtr(char *ptr, const
MinsfiSandbox *sb) {
you could probably make this a const char *ptr

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry.c
File src/minsfi/trusted/entry.c (right):

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry...
src/minsfi/trusted/entry.c:86: return 0;
free(info), on early exit?

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry...
src/minsfi/trusted/entry.c:95: return 0;
free(info)?

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/untrusted/sta...
File src/minsfi/untrusted/start.c (right):

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/untrusted/sta...
src/minsfi/untrusted/start.c:13: * See nacl_startup.h for the layout at the
argument pointer.
I don't think nacl_startup.h will describe the same layout.

You would have to point to src/minsfi/trusted/entry.c instead?

The NaCl one has info[0] == NULL, info[1] == envc, then argc/argv, then envp,
the auxv.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/nacl.scons#...
tests/minsfi/nacl.scons:16: supportlib=True):
maybe call it support_lib to separate the words

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_initia...
File tests/minsfi/test_initializer.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_initia...
tests/minsfi/test_initializer.c:26: char *argv[] = { "foo" };
could just make these static

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
File tests/minsfi/test_invoke_args.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:37: /* Helper function which creates a
NULL-terminated string that occupies
/*
 * xyz
 */

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:48: char *argv_long_arg[] = { get_str(32 *
page_size) };
Maybe minsfi_priv.h should have a function or something to return the size of
the initial thread's stack (well, the only stack right now). Then that can
change in sync with the tests.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:65: 
How about a test where it does barely fit and exits cleanly.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_pointe...
File tests/minsfi/test_pointer_conversion.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_pointe...
tests/minsfi/test_pointer_conversion.c:21: MinsfiSandbox sb;
Could be static too. I think we sometimes prefix globals w/ "g_" also.

[dbrazdil, 2014-09-10 18:45:11]

Patchset #2 (id:100001) has been deleted

[dbrazdil, 2014-09-10 18:49:48]

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv.h
File src/include/minsfi_priv.h (right):

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv....
src/include/minsfi_priv.h:15: /* Definition of the untrusted pointer type */
On 2014/09/10 17:04:29, jvoung wrote:
> It seems more like this is analogous to uintptr_t -- an integer type capable
of
> holding a value converted from an untrusted pointer, and can be converted back
> without loss of information, etc.

Done.

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_priv....
src/include/minsfi_priv.h:20: uint32_t dataseg_offset;
On 2014/09/10 17:04:29, jvoung wrote:
> should this be an sfiptr_t also?

I thought about that and I see how it would make sense, but the value is not
used in the same context. It's only used by GenerateMemoryLayout to compute the
bounds of the individual memory regions and is never actually added to a base.
That's the job of layout.dataseg.offset

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_ptr.h
File src/include/minsfi_ptr.h (right):

https://codereview.chromium.org/546883003/diff/80001/src/include/minsfi_ptr.h...
src/include/minsfi_ptr.h:39: static inline sfiptr_t ToMinsfiPtr(char *ptr, const
MinsfiSandbox *sb) {
On 2014/09/10 17:04:29, jvoung wrote:
> you could probably make this a const char *ptr

Done.

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry.c
File src/minsfi/trusted/entry.c (right):

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry...
src/minsfi/trusted/entry.c:86: return 0;
On 2014/09/10 17:04:29, jvoung wrote:
> free(info), on early exit?

Done. Thanks

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/trusted/entry...
src/minsfi/trusted/entry.c:95: return 0;
On 2014/09/10 17:04:29, jvoung wrote:
> free(info)?

Done.

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/untrusted/sta...
File src/minsfi/untrusted/start.c (right):

https://codereview.chromium.org/546883003/diff/80001/src/minsfi/untrusted/sta...
src/minsfi/untrusted/start.c:13: * See nacl_startup.h for the layout at the
argument pointer.
On 2014/09/10 17:04:29, jvoung wrote:
> I don't think nacl_startup.h will describe the same layout.
> 
> You would have to point to src/minsfi/trusted/entry.c instead?
> 
> The NaCl one has info[0] == NULL, info[1] == envc, then argc/argv, then envp,
> the auxv.

Done.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/nacl.scons#...
tests/minsfi/nacl.scons:16: supportlib=True):
On 2014/09/10 17:04:29, jvoung wrote:
> maybe call it support_lib to separate the words

Done.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_initia...
File tests/minsfi/test_initializer.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_initia...
tests/minsfi/test_initializer.c:26: char *argv[] = { "foo" };
On 2014/09/10 17:04:29, jvoung wrote:
> could just make these static

Done.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
File tests/minsfi/test_invoke_args.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:37: /* Helper function which creates a
NULL-terminated string that occupies
On 2014/09/10 17:04:29, jvoung wrote:
> /*
>  * xyz
>  */

Done. (and below...)

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:48: char *argv_long_arg[] = { get_str(32 *
page_size) };
On 2014/09/10 17:04:30, jvoung wrote:
> Maybe minsfi_priv.h should have a function or something to return the size of
> the initial thread's stack (well, the only stack right now). Then that can
> change in sync with the tests.

Queries the active sandbox. Now tests invoke CopyArguments directly and check
the output which is much more reliable. They still run InvokeSandbox to verify
that it will fail.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_invoke...
tests/minsfi/test_invoke_args.c:65: 
On 2014/09/10 17:04:30, jvoung wrote:
> How about a test where it does barely fit and exits cleanly.

Done.

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_pointe...
File tests/minsfi/test_pointer_conversion.c (right):

https://codereview.chromium.org/546883003/diff/80001/tests/minsfi/test_pointe...
tests/minsfi/test_pointer_conversion.c:21: MinsfiSandbox sb;
On 2014/09/10 17:04:30, jvoung wrote:
> Could be static too. I think we sometimes prefix globals w/ "g_" also.

Done.

[jvoung (off chromium), 2014-09-11 01:17:38]

lgtm

[dbrazdil, 2014-09-11 01:18:42]

The CQ bit was checked by dbrazdil@google.com

[commit-bot: I haz the power, 2014-09-11 01:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/546883003/120001

[commit-bot: I haz the power, 2014-09-11 01:37:07]

Committed patchset #2 (id:120001) as 13728