From 8e3480edfe9933306f82c1656deb8e6b7090273c Mon Sep 17 00:00:00 2001
From: Mikhail Naganov <mnaganov@google.com>
Date: Thu, 1 Sep 2022 00:31:43 +0000
Subject: [PATCH] audio: Add checks to effects feature configs retrieval

The size of the feature config needs to be limited
by the Binder transaction size. This check is enforced
before calling into legacy C API.

Also, fixed invalid calculation of buffer size
in Effect::getSupportedConfigsImpl.

Bug: 240266798
Test: atest VtsHalAudioEffectV7_0TargetTest
Change-Id: I1a1f7931a07e28642967fa68d9a358429138db29
---
 audio/effect/all-versions/default/Effect.cpp  | 11 ++++++-
 audio/effect/all-versions/default/Effect.h    |  3 ++
 .../VtsHalAudioEffectTargetTest.cpp           | 31 +++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/audio/effect/all-versions/default/Effect.cpp b/audio/effect/all-versions/default/Effect.cpp
index def3a3f3fb..b57dc63368 100644
--- a/audio/effect/all-versions/default/Effect.cpp
+++ b/audio/effect/all-versions/default/Effect.cpp
@@ -316,6 +316,11 @@ void Effect::getConfigImpl(int commandCode, const char* commandName, GetConfigCa
 
 Result Effect::getCurrentConfigImpl(uint32_t featureId, uint32_t configSize,
                                     GetCurrentConfigSuccessCallback onSuccess) {
+    if (configSize > kMaxDataSize - sizeof(uint32_t)) {
+        ALOGE("%s: Config size is too big: %" PRIu32, __func__, configSize);
+        android_errorWriteLog(0x534e4554, "240266798");
+        return Result::INVALID_ARGUMENTS;
+    }
     uint32_t halCmd = featureId;
     std::vector<uint32_t> halResult(alignedSizeIn<uint32_t>(sizeof(uint32_t) + configSize), 0);
     uint32_t halResultSize = 0;
@@ -350,8 +355,12 @@ Result Effect::getParameterImpl(uint32_t paramSize, const void* paramData,
 
 Result Effect::getSupportedConfigsImpl(uint32_t featureId, uint32_t maxConfigs, uint32_t configSize,
                                        GetSupportedConfigsSuccessCallback onSuccess) {
+    if (maxConfigs != 0 && configSize > (kMaxDataSize - 2 * sizeof(uint32_t)) / maxConfigs) {
+        ALOGE("%s: Config size is too big: %" PRIu32, __func__, configSize);
+        return Result::INVALID_ARGUMENTS;
+    }
     uint32_t halCmd[2] = {featureId, maxConfigs};
-    uint32_t halResultSize = 2 * sizeof(uint32_t) + maxConfigs * sizeof(configSize);
+    uint32_t halResultSize = 2 * sizeof(uint32_t) + maxConfigs * configSize;
     std::vector<uint8_t> halResult(static_cast<size_t>(halResultSize), 0);
     return sendCommandReturningStatusAndData(
         EFFECT_CMD_GET_FEATURE_SUPPORTED_CONFIGS, "GET_FEATURE_SUPPORTED_CONFIGS", sizeof(halCmd),
diff --git a/audio/effect/all-versions/default/Effect.h b/audio/effect/all-versions/default/Effect.h
index f0b65df0b9..5d8dcccba6 100644
--- a/audio/effect/all-versions/default/Effect.h
+++ b/audio/effect/all-versions/default/Effect.h
@@ -184,6 +184,9 @@ struct Effect : public IEffect {
     using GetSupportedConfigsSuccessCallback =
         std::function<void(uint32_t supportedConfigs, void* configsData)>;
 
+    // Sets the limit on the maximum size of vendor-provided data structures.
+    static constexpr size_t kMaxDataSize = 1 << 20;
+
     static const char* sContextResultOfCommand;
     static const char* sContextCallToCommand;
     static const char* sContextCallFunction;
diff --git a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
index c808ff6cc2..d95bb06c3a 100644
--- a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
+++ b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
@@ -665,6 +665,37 @@ TEST_P(AudioEffectHidlTest, SetCurrentConfigForFeature) {
     EXPECT_TRUE(ret.isOk());
 }
 
+TEST_P(AudioEffectHidlTest, GetSupportedConfigsForFeatureInvalidConfigSize) {
+    description("Verify that GetSupportedConfigsForFeature caps the maximum config size");
+    const bool isNewDeviceLaunchingOnTPlus = property_get_int32("ro.vendor.api_level", 0) >= 33;
+    if (!isNewDeviceLaunchingOnTPlus) {
+        GTEST_SKIP() << "The test only applies to devices launching on T or later";
+    }
+    // Use very large size to ensure that the service does not crash.
+    const uint32_t veryLargeConfigSize = std::numeric_limits<uint32_t>::max() - 100;
+    Result retval = Result::OK;
+    Return<void> ret = effect->getSupportedConfigsForFeature(
+            0, 1, veryLargeConfigSize,
+            [&](Result r, uint32_t, const hidl_vec<uint8_t>&) { retval = r; });
+    EXPECT_TRUE(ret.isOk());
+    EXPECT_EQ(Result::INVALID_ARGUMENTS, retval);
+}
+
+TEST_P(AudioEffectHidlTest, GetCurrentConfigForFeatureInvalidConfigSize) {
+    description("Verify that GetCurrentConfigForFeature caps the maximum config size");
+    const bool isNewDeviceLaunchingOnTPlus = property_get_int32("ro.vendor.api_level", 0) >= 33;
+    if (!isNewDeviceLaunchingOnTPlus) {
+        GTEST_SKIP() << "The test only applies to devices launching on T or later";
+    }
+    // Use very large size to ensure that the service does not crash.
+    const uint32_t veryLargeConfigSize = std::numeric_limits<uint32_t>::max() - 100;
+    Result retval = Result::OK;
+    Return<void> ret = effect->getCurrentConfigForFeature(
+            0, veryLargeConfigSize, [&](Result r, const hidl_vec<uint8_t>&) { retval = r; });
+    EXPECT_TRUE(ret.isOk());
+    EXPECT_EQ(Result::INVALID_ARGUMENTS, retval);
+}
+
 // The main test class for Equalizer Audio Effect HIDL HAL.
 class EqualizerAudioEffectHidlTest : public AudioEffectHidlTest {
   public: