Fix mismatch between isSupported & allocate

isSupported failed to validate usage bits like allocate does,
meaning that isSupported can return true if a vendor is unaware
of a usage bit but then allocate will (correctly!) fail to allocate.

Fix this so that all buffer descriptions go through the same
validation path.

Bug: 216478685
Test: atest HardwareBufferTest#testInvalidUsage
Change-Id: Idc9a0837f555beceb604f27a06e4833f41e97261

2 files changed, 49 insertions, 36 deletions

diff --git a/libs/ui/Gralloc4.cpp b/libs/ui/Gralloc4.cpp
index 1f8a2f058f..e02632460f 100644
--- a/libs/ui/Gralloc4.cpp
+++ b/libs/ui/Gralloc4.cpp
@@ -92,17 +92,6 @@ static inline IMapper::Rect sGralloc4Rect(const Rect& rect) {
     outRect.height = rect.height();
     return outRect;
 }
-static inline void sBufferDescriptorInfo(std::string name, uint32_t width, uint32_t height,
-                                         PixelFormat format, uint32_t layerCount, uint64_t usage,
-                                         IMapper::BufferDescriptorInfo* outDescriptorInfo) {
-    outDescriptorInfo->name = name;
-    outDescriptorInfo->width = width;
-    outDescriptorInfo->height = height;
-    outDescriptorInfo->layerCount = layerCount;
-    outDescriptorInfo->format = static_cast<hardware::graphics::common::V1_2::PixelFormat>(format);
-    outDescriptorInfo->usage = usage;
-    outDescriptorInfo->reservedSize = 0;
-}
 
 // See if gralloc "4.1" is available.
 static bool hasIAllocatorAidl() {
@@ -119,6 +108,36 @@ static bool hasIAllocatorAidl() {
     return sHasIAllocatorAidl;
 }
 
+// Determines whether the passed info is compatible with the mapper.
+static status_t validateBufferDescriptorInfo(IMapper::BufferDescriptorInfo* descriptorInfo) {
+    uint64_t validUsageBits = getValidUsageBits();
+    if (hasIAllocatorAidl()) {
+        validUsageBits |= getValidUsageBits41();
+    }
+
+    if (descriptorInfo->usage & ~validUsageBits) {
+        ALOGE("buffer descriptor contains invalid usage bits 0x%" PRIx64,
+              descriptorInfo->usage & ~validUsageBits);
+        return BAD_VALUE;
+    }
+    return NO_ERROR;
+}
+
+static inline status_t sBufferDescriptorInfo(std::string name, uint32_t width, uint32_t height,
+                                             PixelFormat format, uint32_t layerCount,
+                                             uint64_t usage,
+                                             IMapper::BufferDescriptorInfo* outDescriptorInfo) {
+    outDescriptorInfo->name = name;
+    outDescriptorInfo->width = width;
+    outDescriptorInfo->height = height;
+    outDescriptorInfo->layerCount = layerCount;
+    outDescriptorInfo->format = static_cast<hardware::graphics::common::V1_2::PixelFormat>(format);
+    outDescriptorInfo->usage = usage;
+    outDescriptorInfo->reservedSize = 0;
+
+    return validateBufferDescriptorInfo(outDescriptorInfo);
+}
+
 } // anonymous namespace
 
 void Gralloc4Mapper::preload() {
@@ -140,21 +159,6 @@ bool Gralloc4Mapper::isLoaded() const {
     return mMapper != nullptr;
 }
 
-status_t Gralloc4Mapper::validateBufferDescriptorInfo(
-        IMapper::BufferDescriptorInfo* descriptorInfo) const {
-    uint64_t validUsageBits = getValidUsageBits();
-    if (hasIAllocatorAidl()) {
-        validUsageBits |= getValidUsageBits41();
-    }
-
-    if (descriptorInfo->usage & ~validUsageBits) {
-        ALOGE("buffer descriptor contains invalid usage bits 0x%" PRIx64,
-              descriptorInfo->usage & ~validUsageBits);
-        return BAD_VALUE;
-    }
-    return NO_ERROR;
-}
-
 status_t Gralloc4Mapper::createDescriptor(void* bufferDescriptorInfo,
                                           void* outBufferDescriptor) const {
     IMapper::BufferDescriptorInfo* descriptorInfo =
@@ -207,8 +211,10 @@ status_t Gralloc4Mapper::validateBufferSize(buffer_handle_t bufferHandle, uint32
                                             uint32_t layerCount, uint64_t usage,
                                             uint32_t stride) const {
     IMapper::BufferDescriptorInfo descriptorInfo;
-    sBufferDescriptorInfo("validateBufferSize", width, height, format, layerCount, usage,
-                          &descriptorInfo);
+    if (auto error = sBufferDescriptorInfo("validateBufferSize", width, height, format, layerCount,
+                                           usage, &descriptorInfo) != OK) {
+        return error;
+    }
 
     auto buffer = const_cast<native_handle_t*>(bufferHandle);
     auto ret = mMapper->validateBufferSize(buffer, descriptorInfo, stride);
@@ -427,7 +433,7 @@ int Gralloc4Mapper::unlock(buffer_handle_t bufferHandle) const {
             if (fd >= 0) {
                 releaseFence = fd;
             } else {
-                ALOGD("failed to dup unlock release fence");
+                ALOGW("failed to dup unlock release fence");
                 sync_wait(fenceHandle->data[0], -1);
             }
         }
@@ -448,7 +454,12 @@ status_t Gralloc4Mapper::isSupported(uint32_t width, uint32_t height, PixelForma
                                      uint32_t layerCount, uint64_t usage,
                                      bool* outSupported) const {
     IMapper::BufferDescriptorInfo descriptorInfo;
-    sBufferDescriptorInfo("isSupported", width, height, format, layerCount, usage, &descriptorInfo);
+    if (auto error = sBufferDescriptorInfo("isSupported", width, height, format, layerCount, usage,
+                                           &descriptorInfo) != OK) {
+        // Usage isn't known to the HAL or otherwise failed validation.
+        *outSupported = false;
+        return OK;
+    }
 
     Error error;
     auto ret = mMapper->isSupported(descriptorInfo,
@@ -691,7 +702,10 @@ status_t Gralloc4Mapper::getDefault(uint32_t width, uint32_t height, PixelFormat
     }
 
     IMapper::BufferDescriptorInfo descriptorInfo;
-    sBufferDescriptorInfo("getDefault", width, height, format, layerCount, usage, &descriptorInfo);
+    if (auto error = sBufferDescriptorInfo("getDefault", width, height, format, layerCount, usage,
+                                           &descriptorInfo) != OK) {
+        return error;
+    }
 
     hidl_vec<uint8_t> vec;
     Error error;
@@ -1133,7 +1147,10 @@ status_t Gralloc4Allocator::allocate(std::string requestorName, uint32_t width,
                                      uint64_t usage, uint32_t bufferCount, uint32_t* outStride,
                                      buffer_handle_t* outBufferHandles, bool importBuffers) const {
     IMapper::BufferDescriptorInfo descriptorInfo;
-    sBufferDescriptorInfo(requestorName, width, height, format, layerCount, usage, &descriptorInfo);
+    if (auto error = sBufferDescriptorInfo(requestorName, width, height, format, layerCount, usage,
+                                           &descriptorInfo) != OK) {
+        return error;
+    }
 
     BufferDescriptor descriptor;
     status_t error = mMapper.createDescriptor(static_cast<void*>(&descriptorInfo),
diff --git a/libs/ui/include/ui/Gralloc4.h b/libs/ui/include/ui/Gralloc4.h
index 6bafcd6c81..fe387090cc 100644
--- a/libs/ui/include/ui/Gralloc4.h
+++ b/libs/ui/include/ui/Gralloc4.h
@@ -155,10 +155,6 @@ public:
 private:
     friend class GraphicBufferAllocator;
 
-    // Determines whether the passed info is compatible with the mapper.
-    status_t validateBufferDescriptorInfo(
-            hardware::graphics::mapper::V4_0::IMapper::BufferDescriptorInfo* descriptorInfo) const;
-
     template <class T>
     using DecodeFunction = status_t (*)(const hardware::hidl_vec<uint8_t>& input, T* output);