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

Commit c38310ff authored by Valerie Hau's avatar Valerie Hau
Browse files

Restrict Automerge: Fix reinterpret_cast security bug 

This patch fixes a security bug in SurfaceFlinger.  Bug is due to a
reinterpret_cast used when obtaining a sp<Layer> from an sp<IBinder> passed
from a client.  Without a checking mechanism, client could pass a
malicious data packet.  This is a modified cherry-pick of a patch by Rob Carr
that utilizes a map to identify the appropriate layer based on
the incoming IBinder token.

Original patch commit:

"Author: Robert Carr <racarr@google.com>
Date:   Thu Apr 11 13:18:21 2019 -0700

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: 137284057
Test: build, boot, manual, SurfaceFlinger_test
Change-Id: I9b5f298db2da9cd974c423eb52f261a90f0d17dc
parent 32615136

libs/gui/include/gui/SurfaceControl.h

+4 −6
Original line number Diff line number Diff line
@@ -76,6 +76,10 @@ public: 


    sp<SurfaceComposerClient> getClient() const;



    SurfaceControl(const sp<SurfaceComposerClient>& client,

                   const sp<IBinder>& handle,

                   const sp<IGraphicBufferProducer>& gbp,

                   bool owned);

private:

    // can't be copied

    SurfaceControl& operator = (SurfaceControl& rhs);
@@ -84,12 +88,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

+6 −22
Original line number Diff line number Diff line
@@ -118,7 +118,6 @@ sp<Layer> Client::getLayerUser(const sp<IBinder>& handle) const 
    return lbc;

}





status_t Client::onTransact(

    uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags)

{
@@ -144,34 +143,19 @@ status_t Client::onTransact( 
     return BnSurfaceComposerClient::onTransact(code, data, reply, flags);

}





status_t Client::createSurface(

        const String8& name,

        uint32_t w, uint32_t h, PixelFormat format, uint32_t flags,

        const sp<IBinder>& parentHandle, int32_t windowType, int32_t ownerUid,

        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;

        }

    }

    if (parent == nullptr) {

        sp<IGraphicBufferProducer>* gbp) {

    bool parentDied;

        parent = getParentLayer(&parentDied);

        // If we had a parent, but it died, we've lost all

        // our capabilities.

        if (parentDied) {

    sp<Layer> parentLayer = getParentLayer(&parentDied);

    if (parentHandle == nullptr && parentDied) {

        return NAME_NOT_FOUND;

    }

    }



    return mFlinger->createLayer(name, this, w, h, format, flags, windowType,

                                 ownerUid, handle, gbp, &parent);

                                 ownerUid, handle, gbp, parentHandle, parentLayer);

}



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

services/surfaceflinger/Client.h

+1 −1
Original line number Diff line number Diff line
@@ -58,7 +58,7 @@ private: 
    virtual status_t createSurface(

            const String8& name,

            uint32_t w, uint32_t h,PixelFormat format, uint32_t flags,

            const sp<IBinder>& parent, int32_t windowType, int32_t ownerUid,

            const sp<IBinder>& parentHandle, int32_t windowType, int32_t ownerUid,

            sp<IBinder>* handle,

            sp<IGraphicBufferProducer>* gbp);

services/surfaceflinger/Layer.cpp

+11 −2
Original line number Diff line number Diff line
@@ -178,10 +178,14 @@ Layer::~Layer() { 
void Layer::onLayerDisplayed(const sp<Fence>& /*releaseFence*/) {}



void Layer::onRemovedFromCurrentState() {

    if (!mPendingRemoval) {

        // the layer is removed from SF mCurrentState to mLayersPendingRemoval



        mPendingRemoval = true;



        // remove from sf mapping

        mFlinger->removeLayerFromMap(this);

    }



    if (mCurrentState.zOrderRelativeOf != nullptr) {

        sp<Layer> strongRelative = mCurrentState.zOrderRelativeOf.promote();

        if (strongRelative != nullptr) {
@@ -221,6 +225,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

+3 −0
Original line number Diff line number Diff line
@@ -704,6 +704,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() {}
@@ -808,6 +810,7 @@ private: 
                                       const LayerVector::Visitor& visitor);

    LayerVector makeChildrenTraversalList(LayerVector::StateSet stateSet,

                                          const std::vector<Layer*>& layersInTree);

    bool mGetHandleCalled = false;

};



// ---------------------------------------------------------------------------