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

Commit 09b9490d authored by Jim Shargo's avatar Jim Shargo
Browse files

Surface: Use fewer raw pointers to help avoid UAF issues 

We've been seeing an increasing and concerning number of buffer-related
UAFs across a number of tests as we roll out more users of Surface.

This change hardens the implementation to use proper refcounts for all
buffers within Surface, as well as use the appropriate function for
converting a ANativeWindowBuffer* to an sp<GraphicBuffer>.

Bug: 413059222
Bug: 418318000
Bug: 420318909
Bug: 421921018
Flag: EXEMPT refactor
Test: existing tests

Change-Id: Id09c300163629467b30c13c36add91a7a4be6576
parent bd52f5a3

libs/gui/Surface.cpp

+48 −42
Original line number Diff line number Diff line
@@ -462,13 +462,21 @@ int Surface::hook_dequeueBuffer(ANativeWindow* window, 
            return interceptor(window, Surface::dequeueBufferInternal, data, buffer, fenceFd);

        }

    }

    return c->dequeueBuffer(buffer, fenceFd);



    sp<GraphicBuffer> graphicBuffer;

    status_t result = c->dequeueBuffer(&graphicBuffer, fenceFd);

    *buffer = graphicBuffer.get();

    return result;

}



int Surface::dequeueBufferInternal(ANativeWindow* window, ANativeWindowBuffer** buffer,

                                   int* fenceFd) {

    Surface* c = getSelf(window);

    return c->dequeueBuffer(buffer, fenceFd);



    sp<GraphicBuffer> graphicBuffer;

    status_t result = c->dequeueBuffer(&graphicBuffer, fenceFd);

    *buffer = graphicBuffer.get();

    return result;

}



int Surface::hook_cancelBuffer(ANativeWindow* window,
@@ -482,12 +490,13 @@ int Surface::hook_cancelBuffer(ANativeWindow* window, 
            return interceptor(window, Surface::cancelBufferInternal, data, buffer, fenceFd);

        }

    }

    return c->cancelBuffer(buffer, fenceFd);

    return c->cancelBuffer(GraphicBuffer::from(buffer), fenceFd);

}



int Surface::cancelBufferInternal(ANativeWindow* window, ANativeWindowBuffer* buffer, int fenceFd) {

    Surface* c = getSelf(window);

    return c->cancelBuffer(buffer, fenceFd);

    sp<GraphicBuffer> graphicBuffer = GraphicBuffer::from(buffer);

    return c->cancelBuffer(GraphicBuffer::from(buffer), fenceFd);

}



int Surface::hook_queueBuffer(ANativeWindow* window,
@@ -501,18 +510,18 @@ int Surface::hook_queueBuffer(ANativeWindow* window, 
            return interceptor(window, Surface::queueBufferInternal, data, buffer, fenceFd);

        }

    }

    return c->queueBuffer(buffer, fenceFd);

    return c->queueBuffer(GraphicBuffer::from(buffer), fenceFd);

}



int Surface::queueBufferInternal(ANativeWindow* window, ANativeWindowBuffer* buffer, int fenceFd) {

    Surface* c = getSelf(window);

    return c->queueBuffer(buffer, fenceFd);

    return c->queueBuffer(GraphicBuffer::from(buffer), fenceFd);

}



int Surface::hook_dequeueBuffer_DEPRECATED(ANativeWindow* window,

        ANativeWindowBuffer** buffer) {

    Surface* c = getSelf(window);

    ANativeWindowBuffer* buf;

    sp<GraphicBuffer> buf;

    int fenceFd = -1;

    int result = c->dequeueBuffer(&buf, &fenceFd);

    if (result != OK) {
@@ -523,29 +532,29 @@ int Surface::hook_dequeueBuffer_DEPRECATED(ANativeWindow* window, 
    if (waitResult != OK) {

        ALOGE("dequeueBuffer_DEPRECATED: Fence::wait returned an error: %d",

                waitResult);

        c->cancelBuffer(buf, -1);

        c->cancelBuffer(std::move(buf), -1);

        return waitResult;

    }

    *buffer = buf;

    *buffer = buf.get();

    return result;

}



int Surface::hook_cancelBuffer_DEPRECATED(ANativeWindow* window,

        ANativeWindowBuffer* buffer) {

    Surface* c = getSelf(window);

    return c->cancelBuffer(buffer, -1);

    return c->cancelBuffer(GraphicBuffer::from(buffer), -1);

}



int Surface::hook_lockBuffer_DEPRECATED(ANativeWindow* window,

        ANativeWindowBuffer* buffer) {

    Surface* c = getSelf(window);

    return c->lockBuffer_DEPRECATED(buffer);

    return c->lockBuffer_DEPRECATED(GraphicBuffer::from(buffer));

}



int Surface::hook_queueBuffer_DEPRECATED(ANativeWindow* window,

        ANativeWindowBuffer* buffer) {

    Surface* c = getSelf(window);

    return c->queueBuffer(buffer, -1);

    return c->queueBuffer(GraphicBuffer::from(buffer), -1);

}



int Surface::hook_perform(ANativeWindow* window, int operation, ...) {
@@ -628,7 +637,7 @@ void Surface::getDequeueBufferInputLocked( 
    dequeueInput->getTimestamps = mEnableFrameTimestamps;

}



int Surface::dequeueBuffer(android_native_buffer_t** buffer, int* fenceFd) {

int Surface::dequeueBuffer(sp<GraphicBuffer>* buffer, int* fenceFd) {

    ATRACE_FORMAT("dequeueBuffer - %s", getDebugName());

    ALOGV("Surface::dequeueBuffer");
@@ -645,7 +654,7 @@ int Surface::dequeueBuffer(android_native_buffer_t** buffer, int* fenceFd) { 
                BufferItem::INVALID_BUFFER_SLOT) {

            sp<GraphicBuffer>& gbuf(mSlots[mSharedBufferSlot].buffer);

            if (gbuf != nullptr) {

                *buffer = gbuf.get();

                *buffer = gbuf;

                *fenceFd = -1;

                return OK;

            }
@@ -728,7 +737,7 @@ int Surface::dequeueBuffer(android_native_buffer_t** buffer, int* fenceFd) { 
        *fenceFd = -1;

    }



    *buffer = gbuf.get();

    *buffer = gbuf;



    if (mSharedBufferMode && mAutoRefresh) {

        mSharedBufferSlot = buf;
@@ -748,15 +757,15 @@ status_t Surface::dequeueBuffer(sp<GraphicBuffer>* buffer, sp<Fence>* outFence) 
        return BAD_VALUE;

    }



    android_native_buffer_t* anb = nullptr;

    sp<GraphicBuffer> tmpBuffer;

    int fd = -1;

    status_t res = dequeueBuffer(&anb, &fd);

    status_t res = dequeueBuffer(&tmpBuffer, &fd);

    if (res != NO_ERROR) {

        ALOGV("dequeueBuffer() returned %d", res);

        return res;

    }



    *buffer = GraphicBuffer::from(anb);

    *buffer = tmpBuffer;

    *outFence = sp<Fence>::make(fd);

    return res;

}
@@ -971,8 +980,7 @@ int Surface::dequeueBuffers(std::vector<BatchBuffer>* buffers) { 
    return OK;

}



int Surface::cancelBuffer(android_native_buffer_t* buffer,

        int fenceFd) {

int Surface::cancelBuffer(sp<GraphicBuffer>&& buffer, int fenceFd) {

    ATRACE_CALL();

    ALOGV("Surface::cancelBuffer");

    Mutex::Autolock lock(mMutex);
@@ -1018,7 +1026,8 @@ int Surface::cancelBuffers(const std::vector<BatchBuffer>& buffers) { 
    size_t numBuffersCancelled = 0;

    int badSlotResult = 0;

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

        int slot = getSlotFromBufferLocked(buffers[i].buffer);

        sp<GraphicBuffer> buffer = GraphicBuffer::from(buffers[i].buffer);

        int slot = getSlotFromBufferLocked(buffer);

        int fenceFd = buffers[i].fenceFd;

        if (slot < 0) {

            if (fenceFd >= 0) {
@@ -1046,15 +1055,12 @@ int Surface::cancelBuffers(const std::vector<BatchBuffer>& buffers) { 
    return OK;

}



int Surface::getSlotFromBufferLocked(

        android_native_buffer_t* buffer) const {

int Surface::getSlotFromBufferLocked(const sp<GraphicBuffer>& buffer) const {

    if (buffer == nullptr) {

        ALOGE("%s: input buffer is null!", __FUNCTION__);

        return BAD_VALUE;

    }



    sp<GraphicBuffer> graphicBuffer = GraphicBuffer::from(buffer);



    FatVector<int> slots; // FatVector (default size 4) to prevent heap allocations most of the time

#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_UNLIMITED_SLOTS)

    for (int i = 0; i < (int)mSlots.size(); i++) {
@@ -1063,8 +1069,8 @@ int Surface::getSlotFromBufferLocked( 
#endif

        const sp<GraphicBuffer>& slotBuffer = mSlots[i].buffer;

        if (slotBuffer != nullptr &&

            ((slotBuffer == graphicBuffer) || (slotBuffer->handle == graphicBuffer->handle) ||

             (slotBuffer->getId() == graphicBuffer->getId()))) {

            ((slotBuffer == buffer) || (slotBuffer->handle == buffer->handle) ||

             (slotBuffer->getId() == buffer->getId()))) {

            slots.push_back(i);

        }

    }
@@ -1072,25 +1078,25 @@ int Surface::getSlotFromBufferLocked( 
    if (slots.size() >= 1) {

        ALOGE_IF(slots.size() != 1,

                 "%s: More than one slot found for buffer handle=%p id=%" PRIu64 " slots=[%s]",

                 __FUNCTION__, buffer->handle, graphicBuffer->getId(),

                 base::Join(slots, ", ").c_str());

                 __FUNCTION__, buffer->handle, buffer->getId(), base::Join(slots, ", ").c_str());



        return slots[0];

    }



    ALOGE("%s: unknown buffer: handle=%p id=%" PRIu64, __FUNCTION__, buffer->handle,

          graphicBuffer->getId());

          buffer->getId());

    return BAD_VALUE;

}



int Surface::lockBuffer_DEPRECATED(android_native_buffer_t* buffer __attribute__((unused))) {

int Surface::lockBuffer_DEPRECATED(const sp<GraphicBuffer>& buffer __attribute__((unused))) {

    ALOGV("Surface::lockBuffer");

    Mutex::Autolock lock(mMutex);

    return OK;

}



void Surface::getQueueBufferInputLocked(android_native_buffer_t* buffer, int fenceFd,

        nsecs_t timestamp, IGraphicBufferProducer::QueueBufferInput* out) {

void Surface::getQueueBufferInputLocked(const sp<GraphicBuffer>& buffer, int fenceFd,

                                        nsecs_t timestamp,

                                        IGraphicBufferProducer::QueueBufferInput* out) {

    bool isAutoTimestamp = false;



    if (timestamp == NATIVE_WINDOW_TIMESTAMP_AUTO) {
@@ -1179,7 +1185,7 @@ void Surface::getQueueBufferInputLocked(android_native_buffer_t* buffer, int fen 
}



void Surface::applyGrallocMetadataLocked(

        android_native_buffer_t* buffer,

        const sp<GraphicBuffer>& buffer,

        const IGraphicBufferProducer::QueueBufferInput& queueBufferInput) {

    ATRACE_CALL();

    auto& mapper = GraphicBufferMapper::get();
@@ -1240,7 +1246,7 @@ void Surface::onBufferQueuedLocked(int slot, sp<Fence> fence, 
    }

}



int Surface::queueBuffer(android_native_buffer_t* buffer, int fenceFd,

int Surface::queueBuffer(sp<GraphicBuffer>&& buffer, int fenceFd,

                         SurfaceQueueBufferOutput* surfaceOutput) {

    ATRACE_CALL();

    ALOGV("Surface::queueBuffer");
@@ -1319,7 +1325,8 @@ int Surface::queueBuffers(const std::vector<BatchQueuedBuffer>& buffers, 
        }



        for (size_t batchIdx = 0; batchIdx < numBuffers; batchIdx++) {

            int i = getSlotFromBufferLocked(buffers[batchIdx].buffer);

            sp<GraphicBuffer> buffer = GraphicBuffer::from(buffers[batchIdx].buffer);

            int i = getSlotFromBufferLocked(buffer);

            if (i < 0) {

                if (buffers[batchIdx].fenceFd >= 0) {

                    close(buffers[batchIdx].fenceFd);
@@ -1329,7 +1336,7 @@ int Surface::queueBuffers(const std::vector<BatchQueuedBuffer>& buffers, 
            bufferSlots[batchIdx] = i;



            IGraphicBufferProducer::QueueBufferInput input;

            getQueueBufferInputLocked(buffers[batchIdx].buffer, buffers[batchIdx].fenceFd,

            getQueueBufferInputLocked(buffer, buffers[batchIdx].fenceFd,

                                      buffers[batchIdx].timestamp, &input);

            input.slot = i;

            bufferFences[batchIdx] = input.fence;
@@ -2183,7 +2190,7 @@ int Surface::isBufferOwned(const sp<GraphicBuffer>& buffer, bool* outIsOwned) co 


    Mutex::Autolock lock(mMutex);



    int slot = this->getSlotFromBufferLocked(buffer->getNativeBuffer());

    int slot = this->getSlotFromBufferLocked(buffer);

    if (slot == BAD_VALUE) {

        ALOGV("%s: Buffer %" PRIu64 " is not owned", __FUNCTION__, buffer->getId());

        *outIsOwned = false;
@@ -2667,12 +2674,11 @@ status_t Surface::lock( 
    setUsage(GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN |

             (mIsForCursor ? GRALLOC_USAGE_CURSOR : 0));



    ANativeWindowBuffer* out;

    sp<GraphicBuffer> backBuffer;

    int fenceFd = -1;

    status_t err = dequeueBuffer(&out, &fenceFd);

    status_t err = dequeueBuffer(&backBuffer, &fenceFd);

    ALOGE_IF(err, "dequeueBuffer failed (%s)", strerror(-err));

    if (err == NO_ERROR) {

        sp<GraphicBuffer> backBuffer(GraphicBuffer::getSelf(out));

        const Rect bounds(backBuffer->width, backBuffer->height);



        Region newDirtyRegion;
@@ -2714,7 +2720,7 @@ status_t Surface::lock( 


        { // scope for the lock

            Mutex::Autolock lock(mMutex);

            int backBufferSlot(getSlotFromBufferLocked(backBuffer.get()));

            int backBufferSlot(getSlotFromBufferLocked(backBuffer));

            if (backBufferSlot >= 0) {

                Region& dirtyRegion(mSlots[backBufferSlot].dirtyRegion);

                mDirtyRegion.subtract(dirtyRegion);

libs/gui/include/gui/Surface.h

+8 −8
Original line number Diff line number Diff line
@@ -370,14 +370,14 @@ private: 
    const char* getDebugName();



protected:

    virtual int dequeueBuffer(ANativeWindowBuffer** buffer, int* fenceFd);

    virtual int cancelBuffer(ANativeWindowBuffer* buffer, int fenceFd);

    virtual int queueBuffer(ANativeWindowBuffer* buffer, int fenceFd,

    virtual int dequeueBuffer(sp<GraphicBuffer>* buffer, int* fenceFd);

    virtual int cancelBuffer(sp<GraphicBuffer>&& buffer, int fenceFd);

    virtual int queueBuffer(sp<GraphicBuffer>&& buffer, int fenceFd,

                            SurfaceQueueBufferOutput* surfaceOutput = nullptr);

    virtual int perform(int operation, va_list args);

    virtual int setSwapInterval(int interval);



    virtual int lockBuffer_DEPRECATED(ANativeWindowBuffer* buffer);

    virtual int lockBuffer_DEPRECATED(const sp<GraphicBuffer>& buffer);



    virtual int connect(int api);

    virtual int setBufferCount(int bufferCount);
@@ -509,18 +509,18 @@ protected: 
    void querySupportedTimestampsLocked() const;



    void freeAllBuffers();

    int getSlotFromBufferLocked(android_native_buffer_t* buffer) const;

    int getSlotFromBufferLocked(const sp<GraphicBuffer>& buffer) const;



    void getDequeueBufferInputLocked(IGraphicBufferProducer::DequeueBufferInput* dequeueInput);



    void getQueueBufferInputLocked(android_native_buffer_t* buffer, int fenceFd, nsecs_t timestamp,

    void getQueueBufferInputLocked(const sp<GraphicBuffer>& buffer, int fenceFd, nsecs_t timestamp,

                                   IGraphicBufferProducer::QueueBufferInput* out);



    // For easing in adoption of gralloc4 metadata by vendor components, as well as for supporting

    // the public ANativeWindow api, allow setting relevant metadata when queueing a buffer through

    // a native window

    void applyGrallocMetadataLocked(

            android_native_buffer_t* buffer,

            const sp<GraphicBuffer>& buffer,

            const IGraphicBufferProducer::QueueBufferInput& queueBufferInput);



    void onBufferQueuedLocked(int slot, sp<Fence> fence,

libs/gui/tests/Surface_test.cpp

+0 −2
Original line number Diff line number Diff line
@@ -2298,9 +2298,7 @@ TEST_F(SurfaceTest, PlatformBufferMethods) { 
    // Verify nullptrs are handled safely:

    //



    EXPECT_EQ(BAD_VALUE, surface->dequeueBuffer((sp<GraphicBuffer>*)nullptr, nullptr));

    EXPECT_EQ(BAD_VALUE, surface->dequeueBuffer((sp<GraphicBuffer>*)nullptr, &fence));

    EXPECT_EQ(BAD_VALUE, surface->dequeueBuffer(&buffer, nullptr));

    EXPECT_EQ(BAD_VALUE, surface->queueBuffer(nullptr, nullptr));

    EXPECT_EQ(BAD_VALUE, surface->detachBuffer(nullptr));

libs/ui/GraphicBuffer.cpp

+1 −1
Original line number Diff line number Diff line
@@ -75,7 +75,7 @@ static void resolveLegacyByteLayoutFromPlaneLayout(const std::vector<ui::PlaneLa 
}



sp<GraphicBuffer> GraphicBuffer::from(ANativeWindowBuffer* anwb) {

    return static_cast<GraphicBuffer *>(anwb);

    return sp<GraphicBuffer>::fromExisting(static_cast<GraphicBuffer*>(anwb));

}



GraphicBuffer* GraphicBuffer::fromAHardwareBuffer(AHardwareBuffer* buffer) {