From 09052cc6655ab2756c4c484b958cdaf29325e652 Mon Sep 17 00:00:00 2001
From: Arthur Ishiguro <arthuri@google.com>
Date: Tue, 22 Sep 2020 13:05:15 -0700
Subject: Prevent mEventCache UAF in SensorEventConnection

Since there is no check to see if SensorEventConnection has been
destroyed, the mEventCache pointer can still be used even after it
was freed.

Bug: 168211968
Test: Run test code that attempts to enable a sensor after destroying
the SensorEventConnection, and verify no system_server crash occurs.

Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580
Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580
---
 services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++---------
 services/sensorservice/SensorEventConnection.h   |  5 +++--
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp
index 0a05dd1b18..15ca7ac3cb 100644
--- a/services/sensorservice/SensorEventConnection.cpp
+++ b/services/sensorservice/SensorEventConnection.cpp
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include <log/log.h>
 #include <sys/socket.h>
 #include <utils/threads.h>
 
@@ -44,20 +45,13 @@ SensorService::SensorEventConnection::SensorEventConnection(
 SensorService::SensorEventConnection::~SensorEventConnection() {
     ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this);
     destroy();
-}
-
-void SensorService::SensorEventConnection::destroy() {
-    Mutex::Autolock _l(mDestroyLock);
-
-    // destroy once only
-    if (mDestroyed) {
-        return;
-    }
-
     mService->cleanupConnection(this);
     if (mEventCache != NULL) {
         delete mEventCache;
     }
+}
+
+void SensorService::SensorEventConnection::destroy() {
     mDestroyed = true;
 }
 
@@ -525,6 +519,11 @@ status_t SensorService::SensorEventConnection::enableDisable(
         int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs,
         int reservedFlags)
 {
+    if (mDestroyed) {
+        android_errorWriteLog(0x534e4554, "168211968");
+        return DEAD_OBJECT;
+    }
+
     status_t err;
     if (enabled) {
         err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs,
@@ -539,10 +538,19 @@ status_t SensorService::SensorEventConnection::enableDisable(
 status_t SensorService::SensorEventConnection::setEventRate(
         int handle, nsecs_t samplingPeriodNs)
 {
+    if (mDestroyed) {
+        android_errorWriteLog(0x534e4554, "168211968");
+        return DEAD_OBJECT;
+    }
+
     return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName);
 }
 
 status_t  SensorService::SensorEventConnection::flush() {
+    if (mDestroyed) {
+        return DEAD_OBJECT;
+    }
+
     return  mService->flushSensor(this, mOpPackageName);
 }
 
diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h
index 6f282cdc60..bd1b3bbef7 100644
--- a/services/sensorservice/SensorEventConnection.h
+++ b/services/sensorservice/SensorEventConnection.h
@@ -17,6 +17,7 @@
 #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H
 #define ANDROID_SENSOR_EVENT_CONNECTION_H
 
+#include <stdatomic.h>
 #include <stdint.h>
 #include <sys/types.h>
 
@@ -165,8 +166,8 @@ private:
     int mTotalAcksNeeded, mTotalAcksReceived;
 #endif
 
-    mutable Mutex mDestroyLock;
-    bool mDestroyed;
+    // Used to track if this object was inappropriately used after destroy().
+    std::atomic_bool mDestroyed;
 };
 
 } // namepsace android
-- 
cgit v1.2.3-59-g8ed1b