mprotect system call exposed to the untrusted code.

Add an IRT dev interface for mprotect()

The mprotect system call is now exposed by IRT.

Note that you still need to use '-a' option otherwise you will receive EACCES error after invocation.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=895
TESTS=small_tests

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=10108

[Mark Seaborn, 2012-10-14 00:47:16]

[+Roland]

The commit message is misleading: you're not changing what's exposed to
untrusted code.  You're changing what interfaces the IRT provides.

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h#newco...
src/untrusted/irt/irt.h:94: int (*mprotect)(void *addr, size_t len, int prot);
You can't just add to "struct nacl_irt_memory".  You need to either change the
version number (but keep the old interface version) or introduce a new
interface.

https://codereview.chromium.org/11141016/diff/1/tests/mmap/mmap_test.cc
File tests/mmap/mmap_test.cc (right):

https://codereview.chromium.org/11141016/diff/1/tests/mmap/mmap_test.cc#newco...
tests/mmap/mmap_test.cc:273: int rc = mprotect(addr, map_size, PROT_NONE);
For this to work, you'll need to add an mprotect() wrapper to nacl-glibc too.

[Petr Hosek, 2012-10-14 00:53:21]

I have updated the commit message.

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h#newco...
src/untrusted/irt/irt.h:94: int (*mprotect)(void *addr, size_t len, int prot);
On 2012/10/14 00:47:16, Mark Seaborn wrote:
> You can't just add to "struct nacl_irt_memory".  You need to either change the
> version number (but keep the old interface version) or introduce a new
> interface.

Which one of these two is the preferred way?

https://codereview.chromium.org/11141016/diff/1/tests/mmap/mmap_test.cc
File tests/mmap/mmap_test.cc (right):

https://codereview.chromium.org/11141016/diff/1/tests/mmap/mmap_test.cc#newco...
tests/mmap/mmap_test.cc:273: int rc = mprotect(addr, map_size, PROT_NONE);
On 2012/10/14 00:47:16, Mark Seaborn wrote:
> For this to work, you'll need to add an mprotect() wrapper to nacl-glibc too.

Undone.

[Mark Seaborn, 2012-10-14 00:59:49]

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/11141016/diff/1/src/untrusted/irt/irt.h#newco...
src/untrusted/irt/irt.h:94: int (*mprotect)(void *addr, size_t len, int prot);
On 2012/10/14 00:53:21, phosek1 wrote:
> On 2012/10/14 00:47:16, Mark Seaborn wrote:
> > You can't just add to "struct nacl_irt_memory".  You need to either
> > change the version number (but keep the old interface version) or
> > introduce a new interface.
> 
> Which one of these two is the preferred way?

There's no precedent here.  We haven't extended any interfaces so far.  My
preference would be for a new interface containing only mprotect(), since it
avoids compatibility and testing problems.  Roland might disagree.

[bsy, 2012-10-17 23:26:31]

lgtm

[Mark Seaborn, 2012-10-18 00:27:22]

Please don't commit this as-is.  I would like to get Roland's opinion on how to
add a call first.

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt.h#ne...
src/untrusted/irt/irt.h:95: int (*mprotect)(void *addr, size_t len, int prot);
Modifying struct nacl_irt_memory in-place means we've lost the information that
v0_1 didn't include mprotect().  I don't like that.  I think we should retain
that information in the headers as documentation in some way.

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt_inte...
File src/untrusted/irt/irt_interfaces.c (right):

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt_inte...
src/untrusted/irt/irt_interfaces.c:23: { NACL_IRT_MEMORY_v0_1, &nacl_irt_memory,
sizeof(nacl_irt_memory) },
The fact that querying for v0_1 with an overly-large size won't give an error is
rather dodgy...

[Mark Seaborn, 2012-10-18 01:42:16]

On 17 October 2012 17:27, <mseaborn@chromium.org> wrote:

> Please don't commit this as-is.  I would like to get Roland's opinion on
> how to
> add a call first.
>

I looked at this more closely, and it looks like this change will stop
existing nexes from working.

You've increased the size of struct nacl_irt_memory.  If you look at
nacl_irt_interface(), you'll see that it will reject requests if user code
passes it a size that is smaller than what is expects.  This will happen
when an existing nexe requests the old, smaller size from an IRT that uses
your new, larger size:

size_t nacl_irt_interface(const char *interface_ident,
                          void *table, size_t tablesize) {
  int i;
  for (i = 0; i < NACL_ARRAY_SIZE(irt_interfaces); ++i) {
    if (0 == strcmp(interface_ident, irt_interfaces[i].name)) {
      const size_t size = irt_interfaces[i].size;
      if (size <= tablesize) {
        memcpy(table, irt_interfaces[i].table, size);
        return size;
      }
      break;
    }
  }
  return 0;
}

The solution that involves the least pain is to add a new "mprotect"
interface, as a separate interface.  Furthermore, you should call it
"dev_mprotect" -- like the existing "dev_getpid" and
"dev_exception_handling" -- then I won't have any trouble approving the
change.  We can remove "dev" from the name at the point where we enable the
mprotect() syscall by default.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Roland McGrath, 2012-10-18 17:13:57]

If this is not yet ready to be available to all applications, then a new dev
interface is definitely the right place to start.  After it's in as dev, we can
put some more thought into how to expose it in the permanent ABI (separate
interface or new version of nacl_irt_memory).

[bsy, 2012-10-18 19:46:09]

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/11141016/diff/4011/src/untrusted/irt/irt.h#ne...
src/untrusted/irt/irt.h:95: int (*mprotect)(void *addr, size_t len, int prot);
the pattern should be something like:

struct nacl_irt_memory {
  int (*sysbrk)(...);
  int (*mmap)(...);
  int (*munmap)(...);
};

struct nacl_irt_memory_v0_2 {
  struct nacl_irt_memory base;
  int (*mprotect)(...);
};

so information that the new version of the interface is a strict extension of
the older one is obvious.  if we were willing to use --fms-extensions or
-fplan9-extensions, "base" could be elided.

[Petr Hosek, 2012-10-25 16:48:56]

On 2012/10/18 17:13:57, Roland McGrath wrote:
> If this is not yet ready to be available to all applications, then a new dev
> interface is definitely the right place to start.  After it's in as dev, we
can
> put some more thought into how to expose it in the permanent ABI (separate
> interface or new version of nacl_irt_memory).

I think it kind of defeats the purpose of having the versioned interface, but if
it's just a temporary solution then fine. I've changed the implementation and
moved the mprotect to a separate dev interface.

[Mark Seaborn, 2012-10-25 17:05:36]

This is better, but there's some leftover code...

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt.h (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt.h:94: int (*mprotect)(void *addr, size_t len, int prot);
Don't add mprotect() to this interface.  I assume this is just a mistake left
over from a previous version.

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt_dev_mprotect.c (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt_dev_mprotect.c:7: #include <errno.h>
Nit: I don't think you need errno.h or stdint.h.

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt_memory.c (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt_memory.c:56: nacl_irt_mprotect,
You shouldn't be adding mprotect here.

[Petr Hosek, 2012-10-25 18:46:57]

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt.h (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt.h:94: int (*mprotect)(void *addr, size_t len, int prot);
On 2012/10/25 17:05:36, Mark Seaborn wrote:
> Don't add mprotect() to this interface.  I assume this is just a mistake left
> over from a previous version.

Yes, sorry I have completely forgotten to remove mprotect from the interface.

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt_dev_mprotect.c (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt_dev_mprotect.c:7: #include <errno.h>
On 2012/10/25 17:05:36, Mark Seaborn wrote:
> Nit: I don't think you need errno.h or stdint.h.

Right, this is leftover.

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
File src/untrusted/irt/irt_memory.c (right):

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt_memory.c:48: static int nacl_irt_mprotect(void *addr,
size_t len, int prot) {
Removed as well.

https://chromiumcodereview.appspot.com/11141016/diff/21001/src/untrusted/irt/...
src/untrusted/irt/irt_memory.c:56: nacl_irt_mprotect,
On 2012/10/25 17:05:36, Mark Seaborn wrote:
> You shouldn't be adding mprotect here.

Removed.

[Mark Seaborn, 2012-10-26 00:25:23]

LGTM