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/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_)