Restrict Automerge: Fix reinterpret_cast security bugandroid-9.0.0_r59android-9.0.0_r58android-9.0.0_r57android-9.0.0_r56android-9.0.0_r55android-9.0.0_r54security-pi-release

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
(cherry-picked from commit c4638082469e906c025c8c8a8614de65c59afc90)

9 files changed, 243 insertions, 106 deletions

diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h
index bd987dd638..38de29915b 100644
--- a/libs/gui/include/gui/SurfaceControl.h
+++ b/libs/gui/include/gui/SurfaceControl.h
@@ -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;
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index 077469b5e1..7563aca15c 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -106,19 +106,18 @@ void Client::detachLayer(const Layer* layer)
         }
     }
 }
-sp<Layer> Client::getLayerUser(const sp<IBinder>& handle) const
+
+bool Client::isAttached(const sp<IBinder>& handle) const
 {
     Mutex::Autolock _l(mLock);
     sp<Layer> lbc;
     wp<Layer> layer(mLayers.valueFor(handle));
     if (layer != 0) {
-        lbc = layer.promote();
-        ALOGE_IF(lbc==0, "getLayerUser(name=%p) is dead", handle.get());
+        return true;
     }
-    return lbc;
+    return false;
 }
 
-
 status_t Client::onTransact(
     uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags)
 {
@@ -144,7 +143,6 @@ 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,
@@ -152,22 +150,11 @@ status_t Client::createSurface(
         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) {
-        bool parentDied;
-        parent = getParentLayer(&parentDied);
-        // If we had a parent, but it died, we've lost all
-        // our capabilities.
-        if (parentDied) {
-            return NAME_NOT_FOUND;
-        }
+    bool parentDied;
+    sp<Layer> parentLayer;
+    if (!parentHandle) parentLayer = getParentLayer(&parentDied);
+    if (parentHandle == nullptr && parentDied) {
+        return NAME_NOT_FOUND;
     }
 
     /*
@@ -184,7 +171,8 @@ status_t Client::createSurface(
         uint32_t w, h;
         PixelFormat format;
         uint32_t flags;
-        sp<Layer>* parent;
+        const sp<IBinder>& parentHandle;
+        const sp<Layer>& parentLayer;
         int32_t windowType;
         int32_t ownerUid;
     public:
@@ -193,23 +181,25 @@ status_t Client::createSurface(
                 uint32_t w, uint32_t h, PixelFormat format, uint32_t flags,
                 sp<IBinder>* handle, int32_t windowType, int32_t ownerUid,
                 sp<IGraphicBufferProducer>* gbp,
-                sp<Layer>* parent)
+                const sp<IBinder>& parentHandle,
+                const sp<Layer>& parentLayer)
             : flinger(flinger), client(client),
               handle(handle), gbp(gbp), result(NO_ERROR),
               name(name), w(w), h(h), format(format), flags(flags),
-              parent(parent), windowType(windowType), ownerUid(ownerUid) {
+              parentHandle(parentHandle), parentLayer(parentLayer),
+              windowType(windowType), ownerUid(ownerUid) {
         }
         status_t getResult() const { return result; }
         virtual bool handler() {
             result = flinger->createLayer(name, client, w, h, format, flags,
-                    windowType, ownerUid, handle, gbp, parent);
+                    windowType, ownerUid, handle, gbp, parentHandle, parentLayer);
             return true;
         }
     };
 
     sp<MessageBase> msg = new MessageCreateLayer(mFlinger.get(),
             name, this, w, h, format, flags, handle,
-            windowType, ownerUid, gbp, &parent);
+            windowType, ownerUid, gbp, parentHandle, parentLayer);
     mFlinger->postMessageSync(msg);
     return static_cast<MessageCreateLayer*>( msg.get() )->getResult();
 }
@@ -219,21 +209,11 @@ status_t Client::destroySurface(const sp<IBinder>& handle) {
 }
 
 status_t Client::clearLayerFrameStats(const sp<IBinder>& handle) const {
-    sp<Layer> layer = getLayerUser(handle);
-    if (layer == nullptr) {
-        return NAME_NOT_FOUND;
-    }
-    layer->clearFrameStats();
-    return NO_ERROR;
+    return mFlinger->clearLayerFrameStats(this, handle);
 }
 
 status_t Client::getLayerFrameStats(const sp<IBinder>& handle, FrameStats* outStats) const {
-    sp<Layer> layer = getLayerUser(handle);
-    if (layer == nullptr) {
-        return NAME_NOT_FOUND;
-    }
-    layer->getFrameStats(outStats);
-    return NO_ERROR;
+    return mFlinger->getLayerFrameStats(this, handle, outStats);
 }
 
 // ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h
index 49437ed7fd..c8da5283c0 100644
--- a/services/surfaceflinger/Client.h
+++ b/services/surfaceflinger/Client.h
@@ -49,7 +49,7 @@ public:
 
     void detachLayer(const Layer* layer);
 
-    sp<Layer> getLayerUser(const sp<IBinder>& handle) const;
+    bool isAttached (const sp<IBinder>& handle) const;
 
     void updateParent(const sp<Layer>& parentLayer);
 
@@ -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);
 
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 2595ec1a05..220da1d40e 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -170,7 +170,6 @@ void Layer::onLayerDisplayed(const sp<Fence>& /*releaseFence*/) {}
 
 void Layer::onRemovedFromCurrentState() {
     // the layer is removed from SF mCurrentState to mLayersPendingRemoval
-
     mPendingRemoval = true;
 
     if (mCurrentState.zOrderRelativeOf != nullptr) {
@@ -212,6 +211,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);
 }
 
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 34811fb152..3dc166aebf 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -699,6 +699,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() {}
@@ -803,6 +805,7 @@ private:
                                        const LayerVector::Visitor& visitor);
     LayerVector makeChildrenTraversalList(LayerVector::StateSet stateSet,
                                           const std::vector<Layer*>& layersInTree);
+    bool mGetHandleCalled = false;
 };
 
 // ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9d0531ef6b..7360306d12 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3051,20 +3051,34 @@ void SurfaceFlinger::drawWormhole(const sp<const DisplayDevice>& displayDevice,
     engine.fillRegionWithColor(region, height, 0, 0, 0, 0);
 }
 
-status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
-        const sp<IBinder>& handle,
-        const sp<IGraphicBufferProducer>& gbc,
-        const sp<Layer>& lbc,
-        const sp<Layer>& parent)
-{
+status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,
+                                        const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,
+                                        const sp<IBinder>& parentHandle,
+                                        const sp<Layer>& parentLayer) {
     // add this layer to the current state list
     {
         Mutex::Autolock _l(mStateLock);
+        sp<Layer> parent;
+
+        if (parentHandle != nullptr) {
+            parent = fromHandle(parentHandle);
+            if (parent == nullptr) {
+                return NAME_NOT_FOUND;
+            }
+        } else {
+            parent = parentLayer;
+        }
+
         if (mNumLayers >= MAX_LAYERS) {
             ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers,
                   MAX_LAYERS);
             return NO_MEMORY;
         }
+
+        auto [itr, inserted] = mLayersByLocalBinderToken.emplace(handle->localBinder(), lbc);
+        if (!inserted) {
+          ALOGE("-----------ERROR REGISTERING HANDLE, layer pair: handle is already a key");
+        }
         if (parent == nullptr) {
             mCurrentState.layersSortedByZ.add(lbc);
         } else {
@@ -3093,6 +3107,23 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
     return NO_ERROR;
 }
 
+status_t SurfaceFlinger::removeLayerFromMap(const wp<Layer>& layer) {
+    auto it = mLayersByLocalBinderToken.begin();
+    while (it != mLayersByLocalBinderToken.end()) {
+        if (it->second == layer) {
+            it = mLayersByLocalBinderToken.erase(it);
+            break;
+        } else {
+            it++;
+        }
+    }
+    if (it == mLayersByLocalBinderToken.end()) {
+        ALOGE("Failed to remove layer from mapping - could not find matching layer");
+        return BAD_VALUE;
+    }
+    return NO_ERROR;
+}
+
 status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) {
     Mutex::Autolock _l(mStateLock);
     return removeLayerLocked(mStateLock, layer, topLevelOnly);
@@ -3331,8 +3362,8 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState
     const layer_state_t& s = composerState.state;
     sp<Client> client(static_cast<Client*>(composerState.client.get()));
 
-    sp<Layer> layer(client->getLayerUser(s.surface));
-    if (layer == nullptr) {
+    sp<Layer> layer = fromHandle(s.surface);
+    if (layer == nullptr || !(client->isAttached(s.surface))) {
         return 0;
     }
 
@@ -3492,8 +3523,8 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) {
     const layer_state_t& state = composerState.state;
     sp<Client> client(static_cast<Client*>(composerState.client.get()));
 
-    sp<Layer> layer(client->getLayerUser(state.surface));
-    if (layer == nullptr) {
+    sp<Layer> layer = fromHandle(state.surface);
+    if (layer == nullptr || !(client->isAttached(state.surface))) {
         return;
     }
 
@@ -3507,13 +3538,12 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) {
     }
 }
 
-status_t SurfaceFlinger::createLayer(
-        const String8& name,
-        const sp<Client>& client,
-        uint32_t w, uint32_t h, PixelFormat format, uint32_t flags,
-        int32_t windowType, int32_t ownerUid, sp<IBinder>* handle,
-        sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent)
-{
+status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& client, uint32_t w,
+                                     uint32_t h, PixelFormat format, uint32_t flags,
+                                     int32_t windowType, int32_t ownerUid, sp<IBinder>* handle,
+                                     sp<IGraphicBufferProducer>* gbp,
+                                     const sp<IBinder>& parentHandle,
+                                     const sp<Layer>& parentLayer) {
     if (int32_t(w|h) < 0) {
         ALOGE("createLayer() failed, w or h is negative (w=%d, h=%d)",
                 int(w), int(h));
@@ -3556,7 +3586,7 @@ status_t SurfaceFlinger::createLayer(
 
     layer->setInfo(windowType, ownerUid);
 
-    result = addClientLayer(client, *handle, *gbp, layer, *parent);
+    result = addClientLayer(client, *handle, *gbp, layer, parentHandle, parentLayer);
     if (result != NO_ERROR) {
         return result;
     }
@@ -3626,14 +3656,35 @@ status_t SurfaceFlinger::createColorLayer(const sp<Client>& client,
     return NO_ERROR;
 }
 
+status_t SurfaceFlinger::clearLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle) {
+    Mutex::Autolock _l(mStateLock);
+    sp<Layer> layer = fromHandle(handle);
+    if (layer == nullptr || !(client->isAttached(handle))) {
+        return NAME_NOT_FOUND;
+    }
+    layer->clearFrameStats();
+    return NO_ERROR;
+}
+
+status_t SurfaceFlinger::getLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle, FrameStats* outStats) {
+    Mutex::Autolock _l(mStateLock);
+    sp<Layer> layer = fromHandle(handle);
+    if (layer == nullptr || !(client->isAttached(handle))) {
+        return NAME_NOT_FOUND;
+    }
+    layer->getFrameStats(outStats);
+    return NO_ERROR;
+}
+
 status_t SurfaceFlinger::onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle)
 {
+    Mutex::Autolock _l(mStateLock);
     // called by a client when it wants to remove a Layer
     status_t err = NO_ERROR;
-    sp<Layer> l(client->getLayerUser(handle));
-    if (l != nullptr) {
+    sp<Layer> l = fromHandle(handle);
+    if (l != nullptr || client->isAttached(handle)) {
         mInterceptor->saveSurfaceDeletion(l);
-        err = removeLayer(l);
+        err = removeLayerLocked(mStateLock, l);
         ALOGE_IF(err<0 && err != NAME_NOT_FOUND,
                 "error removing layer=%p (%s)", l.get(), strerror(-err));
     }
@@ -3642,15 +3693,18 @@ status_t SurfaceFlinger::onLayerRemoved(const sp<Client>& client, const sp<IBind
 
 status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer)
 {
+    Mutex::Autolock _l(mStateLock);
     // called by ~LayerCleaner() when all references to the IBinder (handle)
     // are gone
     sp<Layer> l = layer.promote();
     if (l == nullptr) {
+        removeLayerFromMap(layer);
         // The layer has already been removed, carry on
         return NO_ERROR;
     }
+    removeLayerFromMap(layer);
     // If we have a parent, then we can continue to live as long as it does.
-    return removeLayer(l, true);
+    return removeLayerLocked(mStateLock, l, true);
 }
 
 // ---------------------------------------------------------------------------
@@ -4808,34 +4862,40 @@ status_t SurfaceFlinger::captureLayers(const sp<IBinder>& layerHandleBinder,
         const bool mChildrenOnly;
     };
 
-    auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
-    auto parent = layerHandle->owner.promote();
+    int reqWidth = 0;
+    int reqHeight = 0;
+    sp<Layer> parent;
+    Rect crop(sourceCrop);
 
-    if (parent == nullptr || parent->isPendingRemoval()) {
-        ALOGE("captureLayers called with a removed parent");
-        return NAME_NOT_FOUND;
-    }
+    {
+        Mutex::Autolock _l(mStateLock);
 
-    const int uid = IPCThreadState::self()->getCallingUid();
-    const bool forSystem = uid == AID_GRAPHICS || uid == AID_SYSTEM;
-    if (!forSystem && parent->getCurrentState().flags & layer_state_t::eLayerSecure) {
-        ALOGW("Attempting to capture secure layer: PERMISSION_DENIED");
-        return PERMISSION_DENIED;
-    }
+        parent = fromHandle(layerHandleBinder);
+        if (parent == nullptr || parent->isPendingRemoval()) {
+            ALOGE("captureLayers called with an invalid or removed parent");
+            return NAME_NOT_FOUND;
+        }
 
-    Rect crop(sourceCrop);
-    if (sourceCrop.width() <= 0) {
-        crop.left = 0;
-        crop.right = parent->getCurrentState().active.w;
-    }
+        const int uid = IPCThreadState::self()->getCallingUid();
+        const bool forSystem = uid == AID_GRAPHICS || uid == AID_SYSTEM;
+        if (!forSystem && parent->getCurrentState().flags & layer_state_t::eLayerSecure) {
+            ALOGW("Attempting to capture secure layer: PERMISSION_DENIED");
+            return PERMISSION_DENIED;
+        }
 
-    if (sourceCrop.height() <= 0) {
-        crop.top = 0;
-        crop.bottom = parent->getCurrentState().active.h;
-    }
+        if (sourceCrop.width() <= 0) {
+            crop.left = 0;
+            crop.right = parent->getCurrentState().active.w;
+        }
 
-    int32_t reqWidth = crop.width() * frameScale;
-    int32_t reqHeight = crop.height() * frameScale;
+        if (sourceCrop.height() <= 0) {
+            crop.top = 0;
+            crop.bottom = parent->getCurrentState().active.h;
+        }
+
+        reqWidth = crop.width() * frameScale;
+        reqHeight = crop.height() * frameScale;
+    } // mStateLock
 
     LayerRenderArea renderArea(this, parent, crop, reqWidth, reqHeight, childrenOnly);
 
@@ -5160,8 +5220,22 @@ void SurfaceFlinger::traverseLayersInDisplay(const sp<const DisplayDevice>& hw,
     }
 }
 
-}; // namespace android
+sp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) {
+    if (!handle) return nullptr;
+
+    BBinder *b = handle->localBinder();
+    if (b == nullptr) {
+        return nullptr;
+    }
+    auto it = mLayersByLocalBinderToken.find(b);
+    if (it != mLayersByLocalBinderToken.end()) {
+        auto ret = it->second.promote();
+        return ret;
+    }
+    return nullptr;
+}
 
+} // namespace android
 
 #if defined(__gl_h_)
 #error "don't include gl/gl.h in this file"
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index b123defa6f..c7555c31d2 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -343,6 +343,12 @@ public:
 
     int getPrimaryDisplayOrientation() const { return mPrimaryDisplayOrientation; }
 
+    status_t clearLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle);
+
+    status_t getLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle, FrameStats* outStats);
+
+    sp<Layer> fromHandle(const sp<IBinder>& handle) REQUIRES(mStateLock);
+
 private:
     friend class Client;
     friend class DisplayEventConnection;
@@ -516,17 +522,17 @@ private:
     uint32_t setTransactionFlags(uint32_t flags, VSyncModulator::TransactionStart transactionStart);
     void commitTransaction();
     bool containsAnyInvalidClientState(const Vector<ComposerState>& states);
-    uint32_t setClientStateLocked(const ComposerState& composerState);
+    uint32_t setClientStateLocked(const ComposerState& composerState) REQUIRES(mStateLock);
     uint32_t setDisplayStateLocked(const DisplayState& s);
-    void setDestroyStateLocked(const ComposerState& composerState);
+    void setDestroyStateLocked(const ComposerState& composerState) REQUIRES(mStateLock);
 
     /* ------------------------------------------------------------------------
      * Layer management
      */
-    status_t createLayer(const String8& name, const sp<Client>& client,
-            uint32_t w, uint32_t h, PixelFormat format, uint32_t flags,
-            int32_t windowType, int32_t ownerUid, sp<IBinder>* handle,
-            sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent);
+    status_t createLayer(const String8& name, const sp<Client>& client, uint32_t w, uint32_t h,
+                         PixelFormat format, uint32_t flags, int32_t windowType, int32_t ownerUid,
+                         sp<IBinder>* handle, sp<IGraphicBufferProducer>* gbp,
+                         const sp<IBinder>& parentHandle, const sp<Layer>& parentLayer = nullptr);
 
     status_t createBufferLayer(const sp<Client>& client, const String8& name,
             uint32_t w, uint32_t h, uint32_t flags, PixelFormat& format,
@@ -552,12 +558,13 @@ private:
     status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
     status_t removeLayerLocked(const Mutex&, const sp<Layer>& layer, bool topLevelOnly = false);
 
+    // remove layer from mapping
+    status_t removeLayerFromMap(const wp<Layer>& layer);
+
     // add a layer to SurfaceFlinger
-    status_t addClientLayer(const sp<Client>& client,
-            const sp<IBinder>& handle,
-            const sp<IGraphicBufferProducer>& gbc,
-            const sp<Layer>& lbc,
-            const sp<Layer>& parent);
+    status_t addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,
+                            const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,
+                            const sp<IBinder>& parentHandle, const sp<Layer>& parentLayer);
 
     /* ------------------------------------------------------------------------
      * Boot animation, on/off animations and screen capture
@@ -820,6 +827,9 @@ private:
     // it may be read from other threads with mStateLock held
     DefaultKeyedVector< wp<IBinder>, sp<DisplayDevice> > mDisplays;
 
+    // protected by mStateLock
+    std::unordered_map<BBinder*, wp<Layer>> mLayersByLocalBinderToken;
+
     // don't use a lock for these, we don't care
     int mDebugRegion;
     int mDebugDDMS;
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index 322e8a0fea..2331dfd13d 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -18,7 +18,8 @@ cc_test {
     tags: ["test"],
     test_suites: ["device-tests"],
     srcs: [
-        "Stress_test.cpp",
+        "InvalidHandles_test.cpp",
+	"Stress_test.cpp",
         "SurfaceInterceptor_test.cpp",
         "Transaction_test.cpp",
     ],
diff --git a/services/surfaceflinger/tests/InvalidHandles_test.cpp b/services/surfaceflinger/tests/InvalidHandles_test.cpp
new file mode 100644
index 0000000000..42d1f5aa6c
--- /dev/null
+++ b/services/surfaceflinger/tests/InvalidHandles_test.cpp
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <binder/Binder.h>
+
+#include <gtest/gtest.h>
+
+#include <gui/ISurfaceComposer.h>
+#include <gui/SurfaceComposerClient.h>
+#include <private/gui/ComposerService.h>
+#include <ui/Rect.h>
+
+namespace android {
+namespace {
+
+class NotALayer : public BBinder {};
+
+/**
+ * For all of these tests we make a SurfaceControl with an invalid layer handle
+ * and verify we aren't able to trick SurfaceFlinger.
+ */
+class InvalidHandleTest : public ::testing::Test {
+protected:
+    sp<SurfaceComposerClient> mScc;
+    sp<SurfaceControl> mNotSc;
+    void SetUp() override {
+        mScc = new SurfaceComposerClient;
+        ASSERT_EQ(NO_ERROR, mScc->initCheck());
+        mNotSc = makeNotSurfaceControl();
+    }
+
+    sp<SurfaceControl> makeNotSurfaceControl() {
+        return new SurfaceControl(mScc, new NotALayer(), nullptr, true);
+    }
+};
+
+TEST_F(InvalidHandleTest, createSurfaceInvalidHandle) {
+    auto notSc = makeNotSurfaceControl();
+    ASSERT_EQ(nullptr,
+              mScc->createSurface(String8("lolcats"), 19, 47, PIXEL_FORMAT_RGBA_8888, 0,
+                                  notSc.get())
+                      .get());
+}
+
+TEST_F(InvalidHandleTest, captureLayersInvalidHandle) {
+    sp<ISurfaceComposer> sf(ComposerService::getComposerService());
+    sp<GraphicBuffer> outBuffer;
+
+    ASSERT_EQ(NAME_NOT_FOUND,
+              sf->captureLayers(mNotSc->getHandle(), &outBuffer, Rect::EMPTY_RECT, 1.0f));
+}
+
+} // namespace
+} // namespace android