commit 2569796daba6d47cb78746dccde14241b3a19729
author Kevin Tang <zhikait@codeaurora.org> Tue May 05 14:42:17 2020 +0530
committer Harrison Lingren <hlingren@google.com> Sat Jun 13 16:05:30 2020 -0700
tree 42f4200fc9154a8aaca4f50e390d1656b5f1e60d
parent c7e46349d26a75824457ef9f5e9e67f2a63a7a8c

fixing the location api level race condition on callbacks

At the layer right under HIDL impl, where the callback objs
are received from HIDL and used by HAL layer, there is race
condition volnerability which could yield using a incompletely
copied sp obj. Added mutex protection.

Change-Id: I611db590d1fadbe43c74db71a1ea906dbe067c6d
CRs-Fixed: 2144976
Bug: 129282808

Signed-off-by: Harrison Lingren <hlingren@google.com>
Change-Id: I15ead5e2502b8b6455441a07e00293591f0f2fd4

msm8998/android/location_api/GnssAPIClient.cpp

diff --git a/msm8998/android/location_api/GnssAPIClient.cpp b/msm8998/android/location_api/GnssAPIClient.cpp
index efcc1c6..400051e 100644
--- a/msm8998/android/location_api/GnssAPIClient.cpp
+++ b/msm8998/android/location_api/GnssAPIClient.cpp
@@ -74,10 +74,10 @@
     const sp<IGnssNiCallback>& niCb)
 {
     LOC_LOGD("%s]: (%p %p)", __FUNCTION__, &gpsCb, &niCb);
-
+    mMutex.lock();
     mGnssCbIface = gpsCb;
     mGnssNiCbIface = niCb;
-
+    mMutex.unlock();
     LocationCallbacks locationCallbacks;
     locationCallbacks.size = sizeof(LocationCallbacks);
 
@@ -234,7 +234,10 @@
     LOC_LOGD("%s]: (%02x)", __FUNCTION__, capabilitiesMask);
     mLocationCapabilitiesMask = capabilitiesMask;
     mLocationCapabilitiesCached = true;
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+   
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (gnssCbIface != nullptr) {
         uint32_t data = 0;
@@ -275,7 +278,9 @@
 void GnssAPIClient::onTrackingCb(Location location)
 {
     LOC_LOGD("%s]: (flags: %02x)", __FUNCTION__, location.flags);
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (gnssCbIface != nullptr) {
         GnssLocation gnssLocation;
@@ -291,7 +296,9 @@
 void GnssAPIClient::onGnssNiCb(uint32_t id, GnssNiNotification gnssNiNotification)
 {
     LOC_LOGD("%s]: (id: %d)", __FUNCTION__, id);
-    sp<IGnssNiCallback> gnssNiCbIface = mGnssNiCbIface;
+    mMutex.lock();
+    auto gnssNiCbIface(mGnssNiCbIface);
+    mMutex.unlock();
 
     if (gnssNiCbIface == nullptr) {
         LOC_LOGE("%s]: mGnssNiCbIface is nullptr", __FUNCTION__);
@@ -364,7 +371,9 @@
 void GnssAPIClient::onGnssSvCb(GnssSvNotification gnssSvNotification)
 {
     LOC_LOGD("%s]: (count: %zu)", __FUNCTION__, gnssSvNotification.count);
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (gnssCbIface != nullptr) {
         IGnssCallback::GnssSvStatus svStatus;
@@ -379,7 +388,9 @@
 
 void GnssAPIClient::onGnssNmeaCb(GnssNmeaNotification gnssNmeaNotification)
 {
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (gnssCbIface != nullptr) {
         android::hardware::hidl_string nmeaString;
@@ -396,7 +407,9 @@
 void GnssAPIClient::onStartTrackingCb(LocationError error)
 {
     LOC_LOGD("%s]: (%d)", __FUNCTION__, error);
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (error == LOCATION_ERROR_SUCCESS && gnssCbIface != nullptr) {
         auto r = gnssCbIface->gnssStatusCb(IGnssCallback::GnssStatusValue::ENGINE_ON);
@@ -415,7 +428,9 @@
 void GnssAPIClient::onStopTrackingCb(LocationError error)
 {
     LOC_LOGD("%s]: (%d)", __FUNCTION__, error);
-    sp<IGnssCallback> gnssCbIface = mGnssCbIface;
+    mMutex.lock();
+    auto gnssCbIface(mGnssCbIface);
+    mMutex.unlock();
 
     if (error == LOCATION_ERROR_SUCCESS && gnssCbIface != nullptr) {
         auto r = gnssCbIface->gnssStatusCb(IGnssCallback::GnssStatusValue::SESSION_END);

msm8998/android/location_api/GnssAPIClient.h

diff --git a/msm8998/android/location_api/GnssAPIClient.h b/msm8998/android/location_api/GnssAPIClient.h
index d447157..904ee25 100644
--- a/msm8998/android/location_api/GnssAPIClient.h
+++ b/msm8998/android/location_api/GnssAPIClient.h
@@ -30,7 +30,7 @@
 #ifndef GNSS_API_CLINET_H
 #define GNSS_API_CLINET_H
 
-
+#include <mutex>
 #include <android/hardware/gnss/1.0/IGnss.h>
 #include <android/hardware/gnss/1.0/IGnssCallback.h>
 #include <android/hardware/gnss/1.0/IGnssNiCallback.h>
@@ -89,7 +89,7 @@
 private:
     sp<IGnssCallback> mGnssCbIface;
     sp<IGnssNiCallback> mGnssNiCbIface;
-
+    std::mutex mMutex;
     LocationCapabilitiesMask mLocationCapabilitiesMask;
     bool mLocationCapabilitiesCached;