Add workaround for mmap() with PROT_EXEC on Chrome OS.

Add workaround for mmap() with PROT_EXEC on Chrome OS.

mmap()'ing a file from Chrome's cache with PROT_EXEC fails on
Chrome OS because Chrome OS mounts the filesystem with noexec.
This CL adds to Non-SFI NaCl IRT a workaround where calling mmap
without PROT_EXEC and changing the permission later with mprotect.
This is the same trick done for SFI NaCl in the following CL.
https://src.chromium.org/viewvc/native_client?revision=11199&view=revision

TEST=App on ChromeOS works without mmap failure.
BUG=359094
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264662

[mazda, 2014-04-15 23:13:38]

Mark, Julien, Hidehiko,

Could you review this CL?

Thanks,

[Mark Seaborn, 2014-04-15 23:28:03]

LGTM

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_memory.cc (right):

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:51: // On Chrome OS, mmap will fail
if PROT_EXEC is set in |host_prot|,
Nit: "can fail"

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:53: // See the comments for
NaClHostDescMap in
X-refs in comments are a little bit of an anti-pattern, because the code
referenced might move or go away.  Maybe just say 'This is because Chrome OS
mounts writable filesystems with "noexec"' instead?

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:61: LOG(FATAL) << "IrtMMap: mprotect
to turn on PROT_EXEC failed, "
I was going to suggest doing "return errno" here, but that would leave the new
mapping behind.  Can you add a comment to say that this aborts because it can't
easily undo the mmap() call?

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:62: << "errno " << errno;
It's better to use LOG_ERRNO or PLOG -- they report errno for you, and decode
errno too.

[mazda, 2014-04-16 00:33:36]

Thank you for the prompt review.

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_memory.cc (right):

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:51: // On Chrome OS, mmap will fail
if PROT_EXEC is set in |host_prot|,
On 2014/04/15 23:28:03, Mark Seaborn wrote:
> Nit: "can fail"

Done.

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:53: // See the comments for
NaClHostDescMap in
On 2014/04/15 23:28:03, Mark Seaborn wrote:
> X-refs in comments are a little bit of an anti-pattern, because the code
> referenced might move or go away.  Maybe just say 'This is because Chrome OS
> mounts writable filesystems with "noexec"' instead?

Done.

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:61: LOG(FATAL) << "IrtMMap: mprotect
to turn on PROT_EXEC failed, "
On 2014/04/15 23:28:03, Mark Seaborn wrote:
> I was going to suggest doing "return errno" here, but that would leave the new
> mapping behind.  Can you add a comment to say that this aborts because it
can't
> easily undo the mmap() call?

Done.

https://codereview.chromium.org/239763005/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_memory.cc:62: << "errno " << errno;
On 2014/04/15 23:28:03, Mark Seaborn wrote:
> It's better to use LOG_ERRNO or PLOG -- they report errno for you, and decode
> errno too.

Done.

[hidehiko, 2014-04-16 01:28:14]

LGTM, but please wait for jln@'s review, too.

[mazda, 2014-04-16 22:55:10]

Julien,

Could you review this CL in terms of security?

Thanks,

[jln (very slow on Chromium), 2014-04-16 23:28:00]

Could we make sure that PROT_EXEC and PROT_WRITE are mutually exclusive?

I don't remember precisely the status of this discussion, but I very much hope
that they're never needed at the same time. (It's ok to transition between
PROT_WRITE and PROT_EXEC).

[jln (very slow on Chromium), 2014-04-16 23:28:08]

https://chromiumcodereview.appspot.com/239763005/diff/20001/components/nacl/l...
File components/nacl/loader/nonsfi/irt_memory.cc (right):

https://chromiumcodereview.appspot.com/239763005/diff/20001/components/nacl/l...
components/nacl/loader/nonsfi/irt_memory.cc:29: return prot;
Could we make sure here that PROT_WRITE and PROT_EXEC are never set at the same
time?

[mazda, 2014-04-17 04:23:07]

https://codereview.chromium.org/239763005/diff/20001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/irt_memory.cc (right):

https://codereview.chromium.org/239763005/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/irt_memory.cc:29: return prot;
On 2014/04/16 23:28:08, jln wrote:
> Could we make sure here that PROT_WRITE and PROT_EXEC are never set at the
same
> time?

Do you mean PROT_WRITE and PROT_EXEC should never be set at the same time in
|prot| for mprotect as well? As this function is used in both IrtMMap and
IrtMProtect, checking |prot| here affects both mmap and mmprotect. Is it what
you intended?

[jln (very slow on Chromium), 2014-04-17 04:36:47]

On 2014/04/17 04:23:07, mazda wrote:
>
https://codereview.chromium.org/239763005/diff/20001/components/nacl/loader/n...
> File components/nacl/loader/nonsfi/irt_memory.cc (right):
> 
>
https://codereview.chromium.org/239763005/diff/20001/components/nacl/loader/n...
> components/nacl/loader/nonsfi/irt_memory.cc:29: return prot;
> On 2014/04/16 23:28:08, jln wrote:
> > Could we make sure here that PROT_WRITE and PROT_EXEC are never set at the
> same
> > time?
> 
> Do you mean PROT_WRITE and PROT_EXEC should never be set at the same time in
> |prot| for mprotect as well? As this function is used in both IrtMMap and
> IrtMProtect, checking |prot| here affects both mmap and mmprotect. Is it what
> you intended?

Yes, could we avoid this at the moment? I know that we intend to have an
exception, probably for V8, but I would like to make sure that the exceptions
are carefully reviewed (we'd probably give them their own wrapper for instance).

[Mark Seaborn, 2014-04-17 16:10:16]

On 16 April 2014 16:28, <jln@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/239763005/diff/
> 20001/components/nacl/loader/nonsfi/irt_memory.cc
> File components/nacl/loader/nonsfi/irt_memory.cc (right):
>
> https://chromiumcodereview.appspot.com/239763005/diff/
> 20001/components/nacl/loader/nonsfi/irt_memory.cc#newcode29
> components/nacl/loader/nonsfi/irt_memory.cc:29: return prot;
> Could we make sure here that PROT_WRITE and PROT_EXEC are never set at
> the same time?


Could we leave that to a separate change, please?  Fixing PROT_EXEC to work
is logically separate from disallowing PROT_WRITE with PROT_EXEC.

irt_memory.cc can't enforce preventing WX mappings.  We do intend to
enforce that in the seccomp-bpf filter, and when we do that it will make
sense to update irt_memory.cc to match, so that the mprotect() and mmap()
calls will gracefully return an error rather than crashing.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-04-17 17:07:43]

> Could we leave that to a separate change, please?  Fixing PROT_EXEC to work
> is logically separate from disallowing PROT_WRITE with PROT_EXEC.

Ok, lgtm, let's do that in a separate change.

[mazda, 2014-04-17 17:17:45]

On 2014/04/17 17:07:43, jln wrote:
> > Could we leave that to a separate change, please?  Fixing PROT_EXEC to work
> > is logically separate from disallowing PROT_WRITE with PROT_EXEC.
> 
> Ok, lgtm, let's do that in a separate change.

Julien, Mark,

Thank you for the comments. I'll work on that in a separate change, and submit
this CL for now.

[mazda, 2014-04-17 17:18:02]

The CQ bit was checked by mazda@chromium.org

[commit-bot: I haz the power, 2014-04-17 17:22:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/239763005/20001

[commit-bot: I haz the power, 2014-04-17 22:51:12]

Change committed as 264662