graphics: fix use-after-free in mapper 2.0 passthrough

The mapper 2.0 gralloc passthrough keeps a set of the native_handle_t
pointers of all imported buffers. This change ensures that the
underlying HAL wrapper freeing the native_handle_t is performed
atomically with respect to accesses to the passthrough's native_handle_t
pointer set.

This fixes a race where a native_handle_t could be reallocated between
being freed by the HAL wrapper and removed from the passthrough's set,
which could then cause the import which happened to reallocate that
native_handle_t to spuriously fail.

Bug: 170798776
Test: Manually very no more spurious import failures in ARCVM's decoder.
Change-Id: I654a44e94adb319f54fb462f4484db414ca1b154

3 files changed, 16 insertions, 19 deletions

diff --git a/graphics/mapper/2.0/utils/hal/include/mapper-hal/2.0/Mapper.h b/graphics/mapper/2.0/utils/hal/include/mapper-hal/2.0/Mapper.h
index 81341742c7..0067105991 100644
--- a/graphics/mapper/2.0/utils/hal/include/mapper-hal/2.0/Mapper.h
+++ b/graphics/mapper/2.0/utils/hal/include/mapper-hal/2.0/Mapper.h
@@ -85,11 +85,7 @@ class MapperImpl : public Interface {
             return Error::BAD_BUFFER;
         }
 
-        Error error = mHal->freeBuffer(bufferHandle);
-        if (error == Error::NONE) {
-            removeImportedBuffer(buffer);
-        }
-        return error;
+        return freeImportedBuffer(bufferHandle);
     }
 
     Return<void> lock(void* buffer, uint64_t cpuUsage, const V2_0::IMapper::Rect& accessRegion,
@@ -160,8 +156,8 @@ class MapperImpl : public Interface {
         return static_cast<void*>(bufferHandle);
     }
 
-    virtual native_handle_t* removeImportedBuffer(void* buffer) {
-        return static_cast<native_handle_t*>(buffer);
+    virtual Error freeImportedBuffer(native_handle_t* bufferHandle) {
+        return mHal->freeBuffer(bufferHandle);
     }
 
     virtual native_handle_t* getImportedBuffer(void* buffer) const {
diff --git a/graphics/mapper/2.0/utils/passthrough/include/mapper-passthrough/2.0/GrallocLoader.h b/graphics/mapper/2.0/utils/passthrough/include/mapper-passthrough/2.0/GrallocLoader.h
index 85a91c3ada..f2e0064357 100644
--- a/graphics/mapper/2.0/utils/passthrough/include/mapper-passthrough/2.0/GrallocLoader.h
+++ b/graphics/mapper/2.0/utils/passthrough/include/mapper-passthrough/2.0/GrallocLoader.h
@@ -56,17 +56,14 @@ class GrallocImportedBufferPool {
         return *singleton;
     }
 
+    std::mutex* getMutex() { return &mMutex; }
+
     void* add(native_handle_t* bufferHandle) {
         std::lock_guard<std::mutex> lock(mMutex);
         return mBufferHandles.insert(bufferHandle).second ? bufferHandle : nullptr;
     }
 
-    native_handle_t* remove(void* buffer) {
-        auto bufferHandle = static_cast<native_handle_t*>(buffer);
-
-        std::lock_guard<std::mutex> lock(mMutex);
-        return mBufferHandles.erase(bufferHandle) == 1 ? bufferHandle : nullptr;
-    }
+    void removeLocked(native_handle* bufferHandle) { mBufferHandles.erase(bufferHandle); }
 
     native_handle_t* get(void* buffer) {
         auto bufferHandle = static_cast<native_handle_t*>(buffer);
@@ -95,8 +92,13 @@ class GrallocMapper : public T {
         return GrallocImportedBufferPool::getInstance().add(bufferHandle);
     }
 
-    native_handle_t* removeImportedBuffer(void* buffer) override {
-        return GrallocImportedBufferPool::getInstance().remove(buffer);
+    Error freeImportedBuffer(native_handle_t* bufferHandle) override {
+        std::lock_guard<std::mutex> lock(*GrallocImportedBufferPool::getInstance().getMutex());
+        Error error = this->mHal->freeBuffer(bufferHandle);
+        if (error == Error::NONE) {
+            GrallocImportedBufferPool::getInstance().removeLocked(bufferHandle);
+        }
+        return error;
     }
 
     native_handle_t* getImportedBuffer(void* buffer) const override {
diff --git a/graphics/mapper/2.1/utils/hal/include/mapper-hal/2.1/Mapper.h b/graphics/mapper/2.1/utils/hal/include/mapper-hal/2.1/Mapper.h
index 038f57264c..b4a2bedc03 100644
--- a/graphics/mapper/2.1/utils/hal/include/mapper-hal/2.1/Mapper.h
+++ b/graphics/mapper/2.1/utils/hal/include/mapper-hal/2.1/Mapper.h
@@ -46,7 +46,7 @@ class MapperImpl : public V2_0::hal::detail::MapperImpl<Interface, Hal> {
             return Error::BAD_BUFFER;
         }
 
-        return mHal->validateBufferSize(bufferHandle, descriptorInfo, stride);
+        return this->mHal->validateBufferSize(bufferHandle, descriptorInfo, stride);
     }
 
     Return<void> getTransportSize(void* buffer, IMapper::getTransportSize_cb hidl_cb) {
@@ -58,7 +58,7 @@ class MapperImpl : public V2_0::hal::detail::MapperImpl<Interface, Hal> {
 
         uint32_t numFds = 0;
         uint32_t numInts = 0;
-        Error error = mHal->getTransportSize(bufferHandle, &numFds, &numInts);
+        Error error = this->mHal->getTransportSize(bufferHandle, &numFds, &numInts);
         hidl_cb(error, numFds, numInts);
         return Void();
     }
@@ -66,7 +66,7 @@ class MapperImpl : public V2_0::hal::detail::MapperImpl<Interface, Hal> {
     Return<void> createDescriptor_2_1(const IMapper::BufferDescriptorInfo& descriptorInfo,
                                       IMapper::createDescriptor_2_1_cb hidl_cb) override {
         BufferDescriptor descriptor;
-        Error error = mHal->createDescriptor_2_1(descriptorInfo, &descriptor);
+        Error error = this->mHal->createDescriptor_2_1(descriptorInfo, &descriptor);
         hidl_cb(error, descriptor);
         return Void();
     }
@@ -74,7 +74,6 @@ class MapperImpl : public V2_0::hal::detail::MapperImpl<Interface, Hal> {
    private:
     using BaseType2_0 = V2_0::hal::detail::MapperImpl<Interface, Hal>;
     using BaseType2_0::getImportedBuffer;
-    using BaseType2_0::mHal;
 };
 
 }  // namespace detail