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
diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h
index 55efcbf..23bfc02 100644
--- a/libs/gui/include/gui/SurfaceControl.h
+++ b/libs/gui/include/gui/SurfaceControl.h
@@ -84,6 +84,9 @@
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 @@
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 e54b460..482c723 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -76,18 +76,9 @@
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 @@
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 {
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 1527c34..60f6fe1 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -130,7 +130,6 @@
}
mFrameTracker.logAndResetStats(mName);
-
mFlinger->onLayerDestroyed();
}
@@ -205,6 +204,11 @@
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 f4545e0..373d159 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -797,6 +797,8 @@
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 @@
FloatRect mScreenBounds;
void setZOrderRelativeOf(const wp<Layer>& relativeOf);
+
+ bool mGetHandleCalled = false;
};
} // namespace android
diff --git a/services/surfaceflinger/RefreshRateOverlay.cpp b/services/surfaceflinger/RefreshRateOverlay.cpp
index e70bfe4..240c84e 100644
--- a/services/surfaceflinger/RefreshRateOverlay.cpp
+++ b/services/surfaceflinger/RefreshRateOverlay.cpp
@@ -31,9 +31,9 @@
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;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 6063555..63522eb 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3396,21 +3396,31 @@
engine.fillRegionWithColor(region, 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,
- bool addToCurrentState)
-{
+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, bool addToCurrentState) {
// 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;
}
+
+ mLayersByLocalBinderToken.emplace(handle->localBinder(), lbc);
+
if (parent == nullptr && addToCurrentState) {
mCurrentState.layersSortedByZ.add(lbc);
} else if (parent == nullptr) {
@@ -4017,13 +4027,19 @@
status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& client, uint32_t w,
uint32_t h, PixelFormat format, uint32_t flags,
LayerMetadata metadata, sp<IBinder>* handle,
- sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent) {
+ 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));
return BAD_VALUE;
}
+ ALOG_ASSERT(parentLayer == nullptr || parentHandle == nullptr,
+ "Expected only one of parentLayer or parentHandle to be non-null. "
+ "Programmer error?");
+
status_t result = NO_ERROR;
sp<Layer> layer;
@@ -4087,8 +4103,8 @@
}
bool addToCurrentState = callingThreadHasUnscopedSurfaceFlingerAccess();
- result = addClientLayer(client, *handle, *gbp, layer, *parent,
- addToCurrentState);
+ result = addClientLayer(client, *handle, *gbp, layer, parentHandle, parentLayer,
+ addToCurrentState);
if (result != NO_ERROR) {
return result;
}
@@ -4205,6 +4221,16 @@
mCurrentState.layersSortedByZ.remove(layer);
}
markLayerPendingRemovalLocked(layer);
+
+ auto it = mLayersByLocalBinderToken.begin();
+ while (it != mLayersByLocalBinderToken.end()) {
+ if (it->second == layer) {
+ it = mLayersByLocalBinderToken.erase(it);
+ } else {
+ it++;
+ }
+ }
+
layer.clear();
}
@@ -5485,34 +5511,50 @@
const bool mChildrenOnly;
};
- auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
- auto parent = layerHandle->owner.promote();
-
- if (parent == nullptr || parent->isRemovedFromCurrentState()) {
- ALOGE("captureLayers called with a removed parent");
- return NAME_NOT_FOUND;
- }
-
- 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;
- }
-
+ int reqWidth = 0;
+ int reqHeight = 0;
+ sp<Layer> parent;
Rect crop(sourceCrop);
- if (sourceCrop.width() <= 0) {
- crop.left = 0;
- crop.right = parent->getBufferSize(parent->getCurrentState()).getWidth();
- }
+ std::unordered_set<sp<Layer>, ISurfaceComposer::SpHash<Layer>> excludeLayers;
- if (sourceCrop.height() <= 0) {
- crop.top = 0;
- crop.bottom = parent->getBufferSize(parent->getCurrentState()).getHeight();
- }
+ {
+ Mutex::Autolock _l(mStateLock);
- int32_t reqWidth = crop.width() * frameScale;
- int32_t reqHeight = crop.height() * frameScale;
+ parent = fromHandle(layerHandleBinder);
+ if (parent == nullptr || parent->isRemovedFromCurrentState()) {
+ ALOGE("captureLayers called with an invalid or removed parent");
+ return NAME_NOT_FOUND;
+ }
+
+ 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.width() <= 0) {
+ crop.left = 0;
+ crop.right = parent->getBufferSize(parent->getCurrentState()).getWidth();
+ }
+
+ if (sourceCrop.height() <= 0) {
+ crop.top = 0;
+ crop.bottom = parent->getBufferSize(parent->getCurrentState()).getHeight();
+ }
+ reqWidth = crop.width() * frameScale;
+ reqHeight = crop.height() * frameScale;
+
+ for (const auto& handle : excludeHandles) {
+ sp<Layer> excludeLayer = fromHandle(handle);
+ if (excludeLayer != nullptr) {
+ excludeLayers.emplace(excludeLayer);
+ } else {
+ ALOGW("Invalid layer handle passed as excludeLayer to captureLayers");
+ return NAME_NOT_FOUND;
+ }
+ }
+ } // mStateLock
// really small crop or frameScale
if (reqWidth <= 0) {
@@ -5522,18 +5564,6 @@
reqHeight = 1;
}
- std::unordered_set<sp<Layer>, ISurfaceComposer::SpHash<Layer>> excludeLayers;
- for (const auto& handle : excludeHandles) {
- BBinder* local = handle->localBinder();
- if (local != nullptr) {
- auto layerHandle = reinterpret_cast<Layer::Handle*>(local);
- excludeLayers.emplace(layerHandle->owner.promote());
- } else {
- ALOGW("Invalid layer handle passed as excludeLayer to captureLayers");
- return NAME_NOT_FOUND;
- }
- }
-
LayerRenderArea renderArea(this, parent, crop, reqWidth, reqHeight, reqDataspace, childrenOnly);
auto traverseLayers = [parent, childrenOnly,
&excludeLayers](const LayerVector::Visitor& visitor) {
@@ -5885,6 +5915,18 @@
mFlinger->setInputWindowsFinished();
}
+sp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) {
+ BBinder *b = handle->localBinder();
+ if (b == nullptr) {
+ return nullptr;
+ }
+ auto it = mLayersByLocalBinderToken.find(b);
+ if (it != mLayersByLocalBinderToken.end()) {
+ return it->second.promote();
+ }
+ return nullptr;
+}
+
} // namespace android
#if defined(__gl_h_)
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 7a7ad33..626140f 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -313,6 +313,8 @@
return mTransactionCompletedThread;
}
+ sp<Layer> fromHandle(const sp<IBinder>& handle) REQUIRES(mStateLock);
+
private:
friend class BufferLayer;
friend class BufferQueueLayer;
@@ -575,7 +577,8 @@
*/
status_t createLayer(const String8& name, const sp<Client>& client, uint32_t w, uint32_t h,
PixelFormat format, uint32_t flags, LayerMetadata metadata,
- sp<IBinder>* handle, sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent);
+ sp<IBinder>* handle, sp<IGraphicBufferProducer>* gbp,
+ const sp<IBinder>& parentHandle, const sp<Layer>& parentLayer = nullptr);
status_t createBufferQueueLayer(const sp<Client>& client, const String8& name, uint32_t w,
uint32_t h, uint32_t flags, LayerMetadata metadata,
@@ -603,12 +606,10 @@
void markLayerPendingRemovalLocked(const sp<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,
- bool addToCurrentState);
+ 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,
+ bool addToCurrentState);
// Traverse through all the layers and compute and cache its bounds.
void computeLayerBounds();
@@ -981,6 +982,9 @@
std::map<wp<IBinder>, sp<DisplayDevice>> mDisplays;
std::unordered_map<DisplayId, sp<IBinder>> mPhysicalDisplayTokens;
+ // protected by mStateLock
+ std::unordered_map<BBinder*, wp<Layer>> mLayersByLocalBinderToken;
+
// don't use a lock for these, we don't care
int mDebugRegion = 0;
bool mDebugDisableHWC = false;
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index 003ae7f..53a3611 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -19,6 +19,7 @@
srcs: [
"BufferGenerator.cpp",
"Credentials_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 0000000..42d1f5a
--- /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