MemoryHeapBase: Map as read-only when needed

When creating a MemoryHeapBase around a file descriptor provided by a
different process, either via an fd or a device name, the existing
code would attempt to map it with PROT_WRITE, unconditionally, which
would result in a failure to map.

With this change, we omit PROT_WRITE from the mapping whenever the
READ_ONLY flag is set, but only when accessing via one of these ctors.
The ctor that allocates a new ashmem region continues to work as
before, with the caller process having write access, but any other
process not having it.

Test: atest -p frameworks/native/libs/binder
Change-Id: Iab3583d841c3dceed1a7cb61e922a85104b4b00b

2 files changed, 12 insertions, 6 deletions

diff --git a/libs/binder/MemoryHeapBase.cpp b/libs/binder/MemoryHeapBase.cpp
index e4ea60f699..e1cbc1996d 100644
--- a/libs/binder/MemoryHeapBase.cpp
+++ b/libs/binder/MemoryHeapBase.cpp
@@ -49,7 +49,7 @@ MemoryHeapBase::MemoryHeapBase(size_t size, uint32_t flags, char const * name)
     int fd = ashmem_create_region(name == nullptr ? "MemoryHeapBase" : name, size);
     ALOGE_IF(fd<0, "error creating ashmem region: %s", strerror(errno));
     if (fd >= 0) {
-        if (mapfd(fd, size) == NO_ERROR) {
+        if (mapfd(fd, true, size) == NO_ERROR) {
             if (flags & READ_ONLY) {
                 ashmem_set_prot_region(fd, PROT_READ);
             }
@@ -70,7 +70,7 @@ MemoryHeapBase::MemoryHeapBase(const char* device, size_t size, uint32_t flags)
     if (fd >= 0) {
         const size_t pagesize = getpagesize();
         size = ((size + pagesize-1) & ~(pagesize-1));
-        if (mapfd(fd, size) == NO_ERROR) {
+        if (mapfd(fd, false, size) == NO_ERROR) {
             mDevice = device;
         }
     }
@@ -82,7 +82,7 @@ MemoryHeapBase::MemoryHeapBase(int fd, size_t size, uint32_t flags, off_t offset
 {
     const size_t pagesize = getpagesize();
     size = ((size + pagesize-1) & ~(pagesize-1));
-    mapfd(fcntl(fd, F_DUPFD_CLOEXEC, 0), size, offset);
+    mapfd(fcntl(fd, F_DUPFD_CLOEXEC, 0), false, size, offset);
 }
 
 status_t MemoryHeapBase::init(int fd, void *base, size_t size, int flags, const char* device)
@@ -98,7 +98,7 @@ status_t MemoryHeapBase::init(int fd, void *base, size_t size, int flags, const
     return NO_ERROR;
 }
 
-status_t MemoryHeapBase::mapfd(int fd, size_t size, off_t offset)
+status_t MemoryHeapBase::mapfd(int fd, bool writeableByCaller, size_t size, off_t offset)
 {
     if (size == 0) {
         // try to figure out the size automatically
@@ -116,8 +116,12 @@ status_t MemoryHeapBase::mapfd(int fd, size_t size, off_t offset)
     }
 
     if ((mFlags & DONT_MAP_LOCALLY) == 0) {
+        int prot = PROT_READ;
+        if (writeableByCaller || (mFlags & READ_ONLY) == 0) {
+            prot |= PROT_WRITE;
+        }
         void* base = (uint8_t*)mmap(nullptr, size,
-                PROT_READ|PROT_WRITE, MAP_SHARED, fd, offset);
+                prot, MAP_SHARED, fd, offset);
         if (base == MAP_FAILED) {
             ALOGE("mmap(fd=%d, size=%zu) failed (%s)",
                     fd, size, strerror(errno));
diff --git a/libs/binder/include/binder/MemoryHeapBase.h b/libs/binder/include/binder/MemoryHeapBase.h
index 52bd5decd4..0ece1215dd 100644
--- a/libs/binder/include/binder/MemoryHeapBase.h
+++ b/libs/binder/include/binder/MemoryHeapBase.h
@@ -51,6 +51,8 @@ public:
 
     /*
      * maps memory from ashmem, with the given name for debugging
+     * if the READ_ONLY flag is set, the memory will be writeable by the calling process,
+     * but not by others. this is NOT the case with the other ctors.
      */
     explicit MemoryHeapBase(size_t size, uint32_t flags = 0, char const* name = nullptr);
 
@@ -78,7 +80,7 @@ protected:
             int flags = 0, const char* device = nullptr);
 
 private:
-    status_t mapfd(int fd, size_t size, off_t offset = 0);
+    status_t mapfd(int fd, bool writeableByCaller, size_t size, off_t offset = 0);
 
     int         mFD;
     size_t      mSize;