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

Commit aff1772c authored by Frederick Mayle's avatar Frederick Mayle
Browse files

binder: fix FD handling in continueWrite 

Only close FDs within the truncated part of the parcel.

This change also fixes a bug where a parcel truncated into the middle of
an object would not properly free that object. That could have resulted
in an OOB access in `Parcel::truncateRpcObjects`, so more bounds
checking is added.

The new tests show how to reproduce the bug by appending to or partially
truncating Parcels owned by the kernel. Two cases are disabled because
of a bug in the Parcel fdsan code (b/370824489).

Cherry-pick notes: Add truncateFileDescriptors method instead of
modifying closeFileDescriptors to avoid API change errors. Large diffs
in this branch because it didn't have the disruptive RPC FD support,
main diff is that the closeFileDescriptors call is move out of the
mOwner implementation. Tweaked the test to support older C++ and
android-base libs.

Flag: EXEMPT bugfix
Ignore-AOSP-First: security fix
Bug: 239222407, 359179312
Test: atest binderLibTest
Merged-In: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3
Change-Id: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3
parent 3f8ed181

libs/binder/IPCThreadState.cpp

+1 −2
Original line number Diff line number Diff line
@@ -1438,7 +1438,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) { 
    return ret;

}



void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,

void IPCThreadState::freeBuffer(Parcel* /*parcel*/, const uint8_t* data,

                                size_t /*dataSize*/,

                                const binder_size_t* /*objects*/,

                                size_t /*objectsSize*/)
@@ -1448,7 +1448,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data, 
        alog << "Writing BC_FREE_BUFFER for " << data << endl;

    }

    ALOG_ASSERT(data != NULL, "Called with NULL data");

    if (parcel != nullptr) parcel->closeFileDescriptors();

    IPCThreadState* state = self();

    state->mOut.writeInt32(BC_FREE_BUFFER);

    state->mOut.writePointer((uintptr_t)data);

libs/binder/Parcel.cpp

+18 −2
Original line number Diff line number Diff line
@@ -2076,11 +2076,15 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const 


void Parcel::closeFileDescriptors()

{

    truncateFileDescriptors(0);

}



void Parcel::truncateFileDescriptors(size_t newObjectsSize) {

    size_t i = mObjectsSize;

    if (i > 0) {

        //ALOGI("Closing file descriptors for %zu objects...", i);

    }

    while (i > 0) {

    while (i > newObjectsSize) {

        i--;

        const flat_binder_object* flat

            = reinterpret_cast<flat_binder_object*>(mData+mObjects[i]);
@@ -2228,6 +2232,7 @@ void Parcel::freeDataNoInit() 
    if (mOwner) {

        LOG_ALLOC("Parcel %p: freeing other owner data", this);

        //ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());

        closeFileDescriptors();

        mOwner(this, mData, mDataSize, mObjects, mObjectsSize);

    } else {

        LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2343,8 +2348,9 @@ status_t Parcel::continueWrite(size_t desired) 
        if (desired == 0) {

            objectsSize = 0;

        } else {

            validateReadData(mDataSize); // hack to sort the objects

            while (objectsSize > 0) {

                if (mObjects[objectsSize-1] < desired)

                if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)

                    break;

                objectsSize--;

            }
@@ -2389,8 +2395,18 @@ status_t Parcel::continueWrite(size_t desired) 
        }

        if (objects && mObjects) {

            memcpy(objects, mObjects, objectsSize*sizeof(binder_size_t));

            // All FDs are owned when `mOwner`, even when `cookie == 0`. When

            // we switch to `!mOwner`, we need to explicitly mark the FDs as

            // owned.

            for (size_t i = 0; i < objectsSize; i++) {

                flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);

                if (flat->hdr.type == BINDER_TYPE_FD) {

                    flat->cookie = 1;

                }

            }

        }

        //ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());

        truncateFileDescriptors(objectsSize);

        mOwner(this, mData, mDataSize, mObjects, mObjectsSize);

        mOwner = nullptr;

libs/binder/include/binder/Parcel.h

+3 −0
Original line number Diff line number Diff line
@@ -528,6 +528,9 @@ public: 
    void                print(TextOutput& to, uint32_t flags = 0) const;



private:

    // Close all file descriptors in the parcel at object positions >= newObjectsSize.

    __attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);



    typedef void        (*release_func)(Parcel* parcel,

                                        const uint8_t* data, size_t dataSize,

                                        const binder_size_t* objects, size_t objectsSize);

libs/binder/tests/Android.bp

+4 −0
Original line number Diff line number Diff line
@@ -60,7 +60,9 @@ cc_test { 
    defaults: ["binder_test_defaults"],

    srcs: ["binderLibTest.cpp"],

    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [
@@ -101,7 +103,9 @@ cc_test { 


    srcs: ["binderLibTest.cpp"],

    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [

libs/binder/tests/binderLibTest.cpp

+163 −0
Original line number Diff line number Diff line
@@ -26,6 +26,8 @@ 
#include <gmock/gmock.h>

#include <gtest/gtest.h>



#include <android-base/logging.h>

#include <android-base/unique_fd.h>

#include <binder/Binder.h>

#include <binder/IBinder.h>

#include <binder/IPCThreadState.h>
@@ -33,6 +35,7 @@ 


#include <linux/sched.h>

#include <sys/epoll.h>

#include <sys/mman.h>

#include <sys/prctl.h>



#include "../binder_module.h"
@@ -41,6 +44,7 @@ 
#define ARRAY_SIZE(array) (sizeof array / sizeof array[0])



using namespace android;

using android::base::unique_fd;

using testing::Not;



// e.g. EXPECT_THAT(expr, StatusEq(OK)) << "additional message";
@@ -85,6 +89,8 @@ enum BinderLibTestTranscationCode { 
    BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,

    BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,

    BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,

    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,

    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,

    BINDER_LIB_TEST_EXIT_TRANSACTION,

    BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,

    BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -404,6 +410,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE 
        };

};



ssize_t countFds() {

    DIR* dir = opendir("/proc/self/fd/");

    if (dir == nullptr) return -1;

    ssize_t ret = 0;

    dirent* ent;

    while ((ent = readdir(dir)) != nullptr) ret++;

    closedir(dir);

    return ret;

}



struct FdLeakDetector {

    int startCount;



    FdLeakDetector() {

        // This log statement is load bearing. We have to log something before

        // counting FDs to make sure the logging system is initialized, otherwise

        // the sockets it opens will look like a leak.

        ALOGW("FdLeakDetector counting FDs.");

        startCount = countFds();

    }

    ~FdLeakDetector() {

        int endCount = countFds();

        if (startCount != endCount) {

            ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount

                          << ") fd leak?";

        }

    }

};



TEST_F(BinderLibTest, NopTransaction) {

    Parcel data, reply;

    EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply),
@@ -812,6 +847,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) { 
    EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));

}



TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {

    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";



    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataPosition(reply.dataSize());

    reply.writeInt32(0);

    reply.setDataPosition(0);

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {

    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";



    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));

}



TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataPosition(reply.dataSize());

    reply.writeInt32(0);

    reply.setDataPosition(0);

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));

}



TEST_F(BinderLibTest, PromoteLocal) {

    sp<IBinder> strong = new BBinder();

    wp<IBinder> weak = strong;
@@ -1445,6 +1574,40 @@ class BinderLibTestService : public BBinder 
                if (ret != size) return UNKNOWN_ERROR;

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {

                unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));

                if (!fd1.ok()) {

                    PLOG(ERROR) << "memfd_create failed";

                    return UNKNOWN_ERROR;

                }

                unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));

                if (!fd2.ok()) {

                    PLOG(ERROR) << "memfd_create failed";

                    return UNKNOWN_ERROR;

                }

                status_t ret;

                ret = reply->writeFileDescriptor(fd1.release(), true);

                if (ret != NO_ERROR) {

                    return ret;

                }

                ret = reply->writeFileDescriptor(fd2.release(), true);

                if (ret != NO_ERROR) {

                    return ret;

                }

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {

                status_t ret;

                ret = reply->writeFileDescriptor(STDOUT_FILENO, false);

                if (ret != NO_ERROR) {

                    return ret;

                }

                ret = reply->writeFileDescriptor(STDERR_FILENO, false);

                if (ret != NO_ERROR) {

                    return ret;

                }

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:

                alarm(10);

                return NO_ERROR;