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

Commit c0df312c authored by Robert Carr's avatar Robert Carr
Browse files

SurfaceFlinger: Validate layers before casting. 

Reinterpret casting random IBinder = no-fun. I first attempted
to use inheritance of "getInterfaceDescriptor" in Layer::Handle but
departing from "standard-layout" (e.g. using virtual methods) means that
downcasting with static/reinterpret_cast is no longer valid. Instead I opted
for the pattern the system-server uses of maintaing a map.

Now that we look up the handle in a map rather than casting IBinder
to Layer::Handle we need to make sure we have unique instances of the
handle. In general this is true but we weren't doing this in the
createWithSurfaceParent where we had an extra call to getHandle. Here
we both refactor createWithSurfaceParent so it works with the new
changes and also add protection for getHandle. We also fix an error
where the handle map was populated outside of lock.

Bug: 129768960
Test: InvalidHandles_test.cpp ASurfaceControlTest SurfaceControlTest
Change-Id: I869bf6164c8d8203af7486ed1b12a763d5a56662
parent 27e6f2b5

libs/gui/include/gui/SurfaceControl.h

+3 −6
Original line number Diff line number Diff line
@@ -84,6 +84,9 @@ public: 
    

    explicit SurfaceControl(const sp<SurfaceControl>& other);



    SurfaceControl(const sp<SurfaceComposerClient>& client, const sp<IBinder>& handle,

                   const sp<IGraphicBufferProducer>& gbp, bool owned);



private:

    // can't be copied

    SurfaceControl& operator = (SurfaceControl& rhs);
@@ -92,12 +95,6 @@ private: 
    friend class SurfaceComposerClient;

    friend class Surface;



    SurfaceControl(

            const sp<SurfaceComposerClient>& client,

            const sp<IBinder>& handle,

            const sp<IGraphicBufferProducer>& gbp,

            bool owned);



    ~SurfaceControl();



    sp<Surface> generateSurfaceLocked() const;

services/surfaceflinger/Client.cpp

+3 −13
Original line number Diff line number Diff line
@@ -76,18 +76,9 @@ status_t Client::createSurface(const String8& name, uint32_t w, uint32_t h, Pixe 
                               uint32_t flags, const sp<IBinder>& parentHandle,

                               LayerMetadata metadata, sp<IBinder>* handle,

                               sp<IGraphicBufferProducer>* gbp) {

    sp<Layer> parent = nullptr;

    if (parentHandle != nullptr) {

        auto layerHandle = reinterpret_cast<Layer::Handle*>(parentHandle.get());

        parent = layerHandle->owner.promote();

        if (parent == nullptr) {

            return NAME_NOT_FOUND;

        }

    }



    // We rely on createLayer to check permissions.

    return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp,

                                 &parent);

                                 parentHandle);

}



status_t Client::createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h,
@@ -104,9 +95,8 @@ status_t Client::createWithSurfaceParent(const String8& name, uint32_t w, uint32 
        return BAD_VALUE;

    }



    sp<IBinder> parentHandle = layer->getHandle();



    return createSurface(name, w, h, format, flags, parentHandle, std::move(metadata), handle, gbp);

    return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp,

                                 nullptr, layer);

}



status_t Client::clearLayerFrameStats(const sp<IBinder>& handle) const {

services/surfaceflinger/Layer.cpp

+5 −1
Original line number Diff line number Diff line
@@ -130,7 +130,6 @@ Layer::~Layer() { 
    }



    mFrameTracker.logAndResetStats(mName);



    mFlinger->onLayerDestroyed();

}
@@ -205,6 +204,11 @@ bool Layer::getPremultipledAlpha() const { 


sp<IBinder> Layer::getHandle() {

    Mutex::Autolock _l(mLock);

    if (mGetHandleCalled) {

        ALOGE("Get handle called twice" );

        return nullptr;

    }

    mGetHandleCalled = true;

    return new Handle(mFlinger, this);

}

services/surfaceflinger/Layer.h

+4 −0
Original line number Diff line number Diff line
@@ -797,6 +797,8 @@ public: 
        wp<Layer> owner;

    };



    // Creates a new handle each time, so we only expect

    // this to be called once.

    sp<IBinder> getHandle();

    const String8& getName() const;

    virtual void notifyAvailableFrames() {}
@@ -930,6 +932,8 @@ private: 
    FloatRect mScreenBounds;



    void setZOrderRelativeOf(const wp<Layer>& relativeOf);



    bool mGetHandleCalled = false;

};



} // namespace android

services/surfaceflinger/RefreshRateOverlay.cpp

+2 −2
Original line number Diff line number Diff line
@@ -31,9 +31,9 @@ bool RefreshRateOverlay::createLayer() { 
    const status_t ret =

            mFlinger.createLayer(String8("RefreshRateOverlay"), mClient, 0, 0,

                                 PIXEL_FORMAT_RGBA_8888, ISurfaceComposerClient::eFXSurfaceColor,

                                 LayerMetadata(), &mIBinder, &mGbp, &mLayer);

                                 LayerMetadata(), &mIBinder, &mGbp, nullptr);

    if (ret) {

        ALOGE("failed to color layer");

        ALOGE("failed to create color layer");

        return false;

    }