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
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 25cb589..6ce2cb8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3131,20 +3131,34 @@
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 {
@@ -3178,6 +3192,24 @@
return removeLayerLocked(mStateLock, layer, topLevelOnly);
}
+status_t SurfaceFlinger::removeLayerFromMap(Layer* layer) {
+ auto it = mLayersByLocalBinderToken.begin();
+ while (it != mLayersByLocalBinderToken.end()) {
+ auto strongRef = it->second.promote();
+ if (strongRef != nullptr && strongRef.get() == 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::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
bool topLevelOnly) {
if (layer->isPendingRemoval()) {
@@ -3614,13 +3646,12 @@
}
}
-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));
@@ -3663,7 +3694,7 @@
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;
}
@@ -4935,34 +4966,40 @@
const bool mChildrenOnly;
};
- auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
- auto parent = layerHandle->owner.promote();
-
- if (parent == nullptr || parent->isPendingRemoval()) {
- 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->getCurrentState().active.w;
- }
- if (sourceCrop.height() <= 0) {
- crop.top = 0;
- crop.bottom = parent->getCurrentState().active.h;
- }
+ {
+ Mutex::Autolock _l(mStateLock);
- int32_t reqWidth = crop.width() * frameScale;
- int32_t reqHeight = crop.height() * frameScale;
+ parent = fromHandle(layerHandleBinder);
+ if (parent == nullptr || parent->isPendingRemoval()) {
+ 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->getCurrentState().active.w;
+ }
+
+ 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);
@@ -5287,8 +5324,20 @@
}
}
-}; // namespace android
+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()) {
+ 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"