Update MemoryHeapBase to avoid abort

MemoryHeapBase currently logs fatal on passing invalid flags.
Since we control flags combinations as compile time constants within an
enum, we can enforce that all the enum values pass valid flag combos in
order to remove the fatal flag checks.

This avoids aborts in fuzzers consuming the enum value. If a user passes
MEMFD_ALLOW_SEALING_FLAG without FORCE_MEMFD set (not possible if using
the enum values), then the flag is silently ignored.

Test: atest binderUnitTest
Bug: 224667194
Change-Id: Iab90c8b0926ac32f01bb1489bf954c136dd2953f

3 files changed, 14 insertions, 9 deletions

diff --git a/libs/binder/MemoryHeapBase.cpp b/libs/binder/MemoryHeapBase.cpp
index 8132d46940..8fe1d2bb3d 100644
--- a/libs/binder/MemoryHeapBase.cpp
+++ b/libs/binder/MemoryHeapBase.cpp
@@ -74,7 +74,7 @@ MemoryHeapBase::MemoryHeapBase(size_t size, uint32_t flags, char const * name)
         fd = memfd_create_region(name ? name : "MemoryHeapBase", size);
         if (fd < 0 || (mapfd(fd, true, size) != NO_ERROR)) return;
         const int SEAL_FLAGS = ((mFlags & READ_ONLY) ? F_SEAL_FUTURE_WRITE : 0) |
-                ((mFlags & MEMFD_ALLOW_SEALING) ? 0 : F_SEAL_SEAL);
+                ((mFlags & MEMFD_ALLOW_SEALING_FLAG) ? 0 : F_SEAL_SEAL);
         if (SEAL_FLAGS && (fcntl(fd, F_ADD_SEALS, SEAL_FLAGS) == -1)) {
             ALOGE("MemoryHeapBase: MemFD %s sealing with flags %x failed with error  %s", name,
                   SEAL_FLAGS, strerror(errno));
@@ -85,12 +85,9 @@ MemoryHeapBase::MemoryHeapBase(size_t size, uint32_t flags, char const * name)
         }
         return;
 #else
-        mFlags &= ~(FORCE_MEMFD | MEMFD_ALLOW_SEALING);
+        mFlags &= ~(FORCE_MEMFD | MEMFD_ALLOW_SEALING_FLAG);
 #endif
     }
-    if (mFlags & MEMFD_ALLOW_SEALING) {
-      LOG_ALWAYS_FATAL("Invalid Flags. MEMFD_ALLOW_SEALING only valid with FORCE_MEMFD.");
-    }
     fd = ashmem_create_region(name ? name : "MemoryHeapBase", size);
     ALOGE_IF(fd < 0, "MemoryHeapBase: error creating ashmem region: %s", strerror(errno));
     if (fd < 0 || (mapfd(fd, true, size) != NO_ERROR)) return;
@@ -103,7 +100,7 @@ MemoryHeapBase::MemoryHeapBase(const char* device, size_t size, uint32_t flags)
     : mFD(-1), mSize(0), mBase(MAP_FAILED), mFlags(flags),
       mDevice(nullptr), mNeedUnmap(false), mOffset(0)
 {
-    if (flags & (FORCE_MEMFD | MEMFD_ALLOW_SEALING)) {
+    if (flags & (FORCE_MEMFD | MEMFD_ALLOW_SEALING_FLAG)) {
         LOG_ALWAYS_FATAL("FORCE_MEMFD, MEMFD_ALLOW_SEALING only valid with creating constructor");
     }
     int open_flags = O_RDWR;
@@ -125,7 +122,7 @@ MemoryHeapBase::MemoryHeapBase(int fd, size_t size, uint32_t flags, off_t offset
     : mFD(-1), mSize(0), mBase(MAP_FAILED), mFlags(flags),
       mDevice(nullptr), mNeedUnmap(false), mOffset(0)
 {
-    if (flags & (FORCE_MEMFD | MEMFD_ALLOW_SEALING)) {
+    if (flags & (FORCE_MEMFD | MEMFD_ALLOW_SEALING_FLAG)) {
         LOG_ALWAYS_FATAL("FORCE_MEMFD, MEMFD_ALLOW_SEALING only valid with creating constructor");
     }
     const size_t pagesize = getpagesize();
diff --git a/libs/binder/include/binder/MemoryHeapBase.h b/libs/binder/include/binder/MemoryHeapBase.h
index 15dd28f08e..c7177bd8eb 100644
--- a/libs/binder/include/binder/MemoryHeapBase.h
+++ b/libs/binder/include/binder/MemoryHeapBase.h
@@ -26,9 +26,10 @@ namespace android {
 
 // ---------------------------------------------------------------------------
 
-class MemoryHeapBase : public virtual BnMemoryHeap
+class MemoryHeapBase : public BnMemoryHeap
 {
 public:
+    static constexpr auto MEMFD_ALLOW_SEALING_FLAG = 0x00000800;
     enum {
         READ_ONLY = IMemoryHeap::READ_ONLY,
         // memory won't be mapped locally, but will be mapped in the remote
@@ -48,7 +49,7 @@ public:
         // Clients of shared files can seal at anytime via syscall, leading to
         // TOC/TOU issues if additional seals prevent access from the creating
         // process. Alternatively, seccomp fcntl().
-        MEMFD_ALLOW_SEALING = 0x00000800
+        MEMFD_ALLOW_SEALING = FORCE_MEMFD | MEMFD_ALLOW_SEALING_FLAG
     };
 
     /*
diff --git a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp
index 21cb70be17..278dd2bf81 100644
--- a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp
+++ b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp
@@ -23,6 +23,7 @@ using namespace android;
 #ifdef __BIONIC__
 TEST(MemoryHeapBase, ForceMemfdRespected) {
     auto mHeap = sp<MemoryHeapBase>::make(10, MemoryHeapBase::FORCE_MEMFD, "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     EXPECT_NE(fd, -1);
     EXPECT_FALSE(ashmem_valid(fd));
@@ -33,6 +34,7 @@ TEST(MemoryHeapBase, MemfdSealed) {
     auto mHeap = sp<MemoryHeapBase>::make(8192,
                                           MemoryHeapBase::FORCE_MEMFD,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     EXPECT_NE(fd, -1);
     EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL);
@@ -43,6 +45,7 @@ TEST(MemoryHeapBase, MemfdUnsealed) {
                                           MemoryHeapBase::FORCE_MEMFD |
                                           MemoryHeapBase::MEMFD_ALLOW_SEALING,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     EXPECT_NE(fd, -1);
     EXPECT_EQ(fcntl(fd, F_GET_SEALS), 0);
@@ -53,6 +56,7 @@ TEST(MemoryHeapBase, MemfdSealedProtected) {
                                           MemoryHeapBase::FORCE_MEMFD |
                                           MemoryHeapBase::READ_ONLY,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     EXPECT_NE(fd, -1);
     EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL | F_SEAL_FUTURE_WRITE);
@@ -64,6 +68,7 @@ TEST(MemoryHeapBase, MemfdUnsealedProtected) {
                                           MemoryHeapBase::READ_ONLY |
                                           MemoryHeapBase::MEMFD_ALLOW_SEALING,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     EXPECT_NE(fd, -1);
     EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_FUTURE_WRITE);
@@ -74,6 +79,7 @@ TEST(MemoryHeapBase, HostMemfdExpected) {
     auto mHeap = sp<MemoryHeapBase>::make(8192,
                                           MemoryHeapBase::READ_ONLY,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     void* ptr = mHeap->getBase();
     EXPECT_NE(ptr, MAP_FAILED);
@@ -87,6 +93,7 @@ TEST(MemoryHeapBase,HostMemfdException) {
                                           MemoryHeapBase::READ_ONLY |
                                           MemoryHeapBase::MEMFD_ALLOW_SEALING,
                                           "Test mapping");
+    ASSERT_NE(mHeap.get(), nullptr);
     int fd = mHeap->getHeapID();
     void* ptr = mHeap->getBase();
     EXPECT_EQ(mHeap->getFlags(), MemoryHeapBase::READ_ONLY);