commit 9a02eda8d5f95d754aa908e66089750183355511
author Alec Mouri <alecmouri@google.com> Tue Apr 21 17:39:34 2020 -0700
committer Alec Mouri <alecmouri@google.com> Wed Apr 22 11:24:30 2020 -0700
tree 97ebeea93991e0132e090c5fddd45fae92967989
parent 9bb4cfb009f1317605e8ff090e0ce930e15d4d40

Lookup layer handle when registering region sampling listener

We must do this in order to prevent clients from providing a bogus
handle when registering a region sampling listener. Fortunately, this
particular path required a permissions check so it cannot be accessed
from arbitrary apps on unrooted devices. But, we should not allow this
type of memory corruption to be reachable by the system.

Bug: 153467444
Test: libgui_test
Test: Repro steps in the bug no longer reproduce
Change-Id: I883506798574dfd0688371fdb6305cfad9d153fc

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index ddf0775..54b7ef3 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1449,7 +1449,9 @@
     if (!listener || samplingArea == Rect::INVALID_RECT) {
         return BAD_VALUE;
     }
-    mRegionSamplingThread->addListener(samplingArea, stopLayerHandle, listener);
+
+    const wp<Layer> stopLayer = fromHandle(stopLayerHandle);
+    mRegionSamplingThread->addListener(samplingArea, stopLayer, listener);
     return NO_ERROR;
 }
 
@@ -3173,7 +3175,7 @@
         Mutex::Autolock _l(mStateLock);
         sp<Layer> parent;
         if (parentHandle != nullptr) {
-            parent = fromHandle(parentHandle);
+            parent = fromHandleLocked(parentHandle).promote();
             if (parent == nullptr) {
                 return NAME_NOT_FOUND;
             }
@@ -3548,7 +3550,7 @@
 
     sp<Layer> layer = nullptr;
     if (s.surface) {
-        layer = fromHandle(s.surface);
+        layer = fromHandleLocked(s.surface).promote();
     } else {
         // The client may provide us a null handle. Treat it as if the layer was removed.
         ALOGW("Attempt to set client state with a null layer handle");
@@ -3864,7 +3866,7 @@
 
     {
         Mutex::Autolock _l(mStateLock);
-        mirrorFrom = fromHandle(mirrorFromHandle);
+        mirrorFrom = fromHandleLocked(mirrorFromHandle).promote();
         if (!mirrorFrom) {
             return NAME_NOT_FOUND;
         }
@@ -5566,7 +5568,7 @@
     {
         Mutex::Autolock lock(mStateLock);
 
-        parent = fromHandle(layerHandleBinder);
+        parent = fromHandleLocked(layerHandleBinder).promote();
         if (parent == nullptr || parent->isRemovedFromCurrentState()) {
             ALOGE("captureLayers called with an invalid or removed parent");
             return NAME_NOT_FOUND;
@@ -5599,7 +5601,7 @@
         reqHeight = crop.height() * frameScale;
 
         for (const auto& handle : excludeHandles) {
-            sp<Layer> excludeLayer = fromHandle(handle);
+            sp<Layer> excludeLayer = fromHandleLocked(handle).promote();
             if (excludeLayer != nullptr) {
                 excludeLayers.emplace(excludeLayer);
             } else {
@@ -6062,7 +6064,12 @@
     mFlinger->setInputWindowsFinished();
 }
 
-sp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) {
+wp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) {
+    Mutex::Autolock _l(mStateLock);
+    return fromHandleLocked(handle);
+}
+
+wp<Layer> SurfaceFlinger::fromHandleLocked(const sp<IBinder>& handle) {
     BBinder* b = nullptr;
     if (handle) {
         b = handle->localBinder();
@@ -6072,7 +6079,7 @@
     }
     auto it = mLayersByLocalBinderToken.find(b);
     if (it != mLayersByLocalBinderToken.end()) {
-        return it->second.promote();
+        return it->second;
     }
     return nullptr;
 }