Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3fd092e7 authored by Atneya Nair's avatar Atneya Nair
Browse files

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
parent 7863bb51

libs/binder/MemoryHeapBase.cpp

+4 −7
Original line number Diff line number Diff line
@@ -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();

libs/binder/include/binder/MemoryHeapBase.h

+3 −2
Original line number Diff line number Diff line
@@ -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

    };



    /*

libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp

+7 −0
Original line number Diff line number Diff line
@@ -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);