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
diff --git a/libs/binder/MemoryHeapBase.cpp b/libs/binder/MemoryHeapBase.cpp
index 8132d46..8fe1d2b 100644
--- a/libs/binder/MemoryHeapBase.cpp
+++ b/libs/binder/MemoryHeapBase.cpp
@@ -74,7 +74,7 @@
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 @@
}
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 @@
: 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 @@
: 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 15dd28f..c7177bd 100644
--- a/libs/binder/include/binder/MemoryHeapBase.h
+++ b/libs/binder/include/binder/MemoryHeapBase.h
@@ -26,9 +26,10 @@
// ---------------------------------------------------------------------------
-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 @@
// 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 21cb70b..278dd2b 100644
--- a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp
+++ b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp
@@ -23,6 +23,7 @@
#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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);