From 3fd092e785f136390963e4f503b31bbd8a2cb997 Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Tue, 24 May 2022 16:50:12 -0400 Subject: 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 --- libs/binder/MemoryHeapBase.cpp | 11 ++++------- libs/binder/include/binder/MemoryHeapBase.h | 5 +++-- libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp | 7 +++++++ 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::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::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::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); -- cgit v1.2.3-59-g8ed1b