summaryrefslogtreecommitdiff
path: root/services/surfaceflinger/SurfaceFlinger.cpp
diff options
context:
space:
mode:
authorKevin F. Haggerty <haggertk@lineageos.org>2020-03-02 18:52:38 -0700
committermosimchah <mosimchah@gmail.com>2020-03-10 05:07:00 -0400
commit7f043c3c3ba765ca801dad7f2c49a1a6fda5c9a6 (patch)
tree85652b3e017dc96bedae5d919322dedaec58921b /services/surfaceflinger/SurfaceFlinger.cpp
parentc5f14bad81722c4eca8a81bffaca44167374f69e (diff)
Restrict Automerge: Fix reinterpret_cast security bugp9.0
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) Change-Id: Iabf5fb6f1b90c4d4d178ae2570b27e9417a4e10a
Diffstat (limited to 'services/surfaceflinger/SurfaceFlinger.cpp')
-rw-r--r--services/surfaceflinger/SurfaceFlinger.cpp125
1 files changed, 87 insertions, 38 deletions
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 5469ae7ba9..2c559775d3 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3201,20 +3201,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 {
@@ -3248,6 +3262,24 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
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()) {
@@ -3684,13 +3716,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));
@@ -3733,7 +3764,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;
}
@@ -5017,34 +5048,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;
+ }
+
+ if (sourceCrop.height() <= 0) {
+ crop.top = 0;
+ crop.bottom = parent->getCurrentState().active.h;
+ }
- int32_t reqWidth = crop.width() * frameScale;
- int32_t reqHeight = crop.height() * frameScale;
+ reqWidth = crop.width() * frameScale;
+ reqHeight = crop.height() * frameScale;
+ } // mStateLock
// really small crop or frameScale
if (reqWidth <= 0) {
@@ -5297,8 +5334,20 @@ void SurfaceFlinger::traverseLayersInDisplay(const sp<const DisplayDevice>& hw,
}
}
-}; // 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"