audio: Add checks to HIDL -> effect_param_t conversion
By convention, the size of the resulting effect_param_t
can not exceed EFFECT_PARAM_SIZE_MAX. This checks needs
to be enforced when converting from HIDL arguments
into legacy C API structures.
Bug: 237291425
Test: atest VtsHalAudioEffectV7_0TargetTest
Change-Id: Ie92f62b002dc622fa8246139c3d956909670fdb6
diff --git a/audio/effect/all-versions/default/Effect.cpp b/audio/effect/all-versions/default/Effect.cpp
index 3baafc9..def3a3f 100644
--- a/audio/effect/all-versions/default/Effect.cpp
+++ b/audio/effect/all-versions/default/Effect.cpp
@@ -238,12 +238,27 @@
}
// static
-std::vector<uint8_t> Effect::parameterToHal(uint32_t paramSize, const void* paramData,
- uint32_t valueSize, const void** valueData) {
+bool Effect::parameterToHal(uint32_t paramSize, const void* paramData, uint32_t valueSize,
+ const void** valueData, std::vector<uint8_t>* halParamBuffer) {
+ constexpr size_t kMaxSize = EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t);
+ if (paramSize > kMaxSize) {
+ ALOGE("%s: Parameter size is too big: %" PRIu32, __func__, paramSize);
+ return false;
+ }
size_t valueOffsetFromData = alignedSizeIn<uint32_t>(paramSize) * sizeof(uint32_t);
+ if (valueOffsetFromData > kMaxSize) {
+ ALOGE("%s: Aligned parameter size is too big: %zu", __func__, valueOffsetFromData);
+ return false;
+ }
+ if (valueSize > kMaxSize - valueOffsetFromData) {
+ ALOGE("%s: Value size is too big: %" PRIu32 ", max size is %zu", __func__, valueSize,
+ kMaxSize - valueOffsetFromData);
+ android_errorWriteLog(0x534e4554, "237291425");
+ return false;
+ }
size_t halParamBufferSize = sizeof(effect_param_t) + valueOffsetFromData + valueSize;
- std::vector<uint8_t> halParamBuffer(halParamBufferSize, 0);
- effect_param_t* halParam = reinterpret_cast<effect_param_t*>(&halParamBuffer[0]);
+ halParamBuffer->resize(halParamBufferSize, 0);
+ effect_param_t* halParam = reinterpret_cast<effect_param_t*>(halParamBuffer->data());
halParam->psize = paramSize;
halParam->vsize = valueSize;
memcpy(halParam->data, paramData, paramSize);
@@ -256,7 +271,7 @@
*valueData = halParam->data + valueOffsetFromData;
}
}
- return halParamBuffer;
+ return true;
}
Result Effect::analyzeCommandStatus(const char* commandName, const char* context, status_t status) {
@@ -314,11 +329,15 @@
GetParameterSuccessCallback onSuccess) {
// As it is unknown what method HAL uses for copying the provided parameter data,
// it is safer to make sure that input and output buffers do not overlap.
- std::vector<uint8_t> halCmdBuffer =
- parameterToHal(paramSize, paramData, requestValueSize, nullptr);
+ std::vector<uint8_t> halCmdBuffer;
+ if (!parameterToHal(paramSize, paramData, requestValueSize, nullptr, &halCmdBuffer)) {
+ return Result::INVALID_ARGUMENTS;
+ }
const void* valueData = nullptr;
- std::vector<uint8_t> halParamBuffer =
- parameterToHal(paramSize, paramData, replyValueSize, &valueData);
+ std::vector<uint8_t> halParamBuffer;
+ if (!parameterToHal(paramSize, paramData, replyValueSize, &valueData, &halParamBuffer)) {
+ return Result::INVALID_ARGUMENTS;
+ }
uint32_t halParamBufferSize = halParamBuffer.size();
return sendCommandReturningStatusAndData(
@@ -472,8 +491,10 @@
Result Effect::setParameterImpl(uint32_t paramSize, const void* paramData, uint32_t valueSize,
const void* valueData) {
- std::vector<uint8_t> halParamBuffer =
- parameterToHal(paramSize, paramData, valueSize, &valueData);
+ std::vector<uint8_t> halParamBuffer;
+ if (!parameterToHal(paramSize, paramData, valueSize, &valueData, &halParamBuffer)) {
+ return Result::INVALID_ARGUMENTS;
+ }
return sendCommandReturningStatus(EFFECT_CMD_SET_PARAM, "SET_PARAM", halParamBuffer.size(),
&halParamBuffer[0]);
}
diff --git a/audio/effect/all-versions/default/Effect.h b/audio/effect/all-versions/default/Effect.h
index 011544d..f0b65df 100644
--- a/audio/effect/all-versions/default/Effect.h
+++ b/audio/effect/all-versions/default/Effect.h
@@ -211,8 +211,8 @@
channel_config_t* halConfig);
static void effectOffloadParamToHal(const EffectOffloadParameter& offload,
effect_offload_param_t* halOffload);
- static std::vector<uint8_t> parameterToHal(uint32_t paramSize, const void* paramData,
- uint32_t valueSize, const void** valueData);
+ static bool parameterToHal(uint32_t paramSize, const void* paramData, uint32_t valueSize,
+ const void** valueData, std::vector<uint8_t>* halParamBuffer);
Result analyzeCommandStatus(const char* commandName, const char* context, status_t status);
void getConfigImpl(int commandCode, const char* commandName, GetConfigCallback cb);
diff --git a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
index e59423f..ffa4c56 100644
--- a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
+++ b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp
@@ -623,6 +623,23 @@
EXPECT_TRUE(ret.isOk());
}
+TEST_P(AudioEffectHidlTest, GetParameterInvalidMaxReplySize) {
+ description("Verify that GetParameter caps the maximum reply size");
+ // Use a non-empty parameter to avoid being rejected by any earlier checks.
+ hidl_vec<uint8_t> parameter;
+ parameter.resize(16);
+ // Use very large size to ensure that the service does not crash. Since parameters
+ // are specific to each effect, and some effects may not have parameters at all,
+ // simply checking the return value would not reveal an issue of using an uncapped value.
+ const uint32_t veryLargeReplySize = std::numeric_limits<uint32_t>::max() - 100;
+ Result retval = Result::OK;
+ Return<void> ret =
+ effect->getParameter(parameter, veryLargeReplySize,
+ [&](Result r, const hidl_vec<uint8_t>&) { retval = r; });
+ EXPECT_TRUE(ret.isOk());
+ EXPECT_EQ(Result::INVALID_ARGUMENTS, retval);
+}
+
TEST_P(AudioEffectHidlTest, GetSupportedConfigsForFeature) {
description("Verify that GetSupportedConfigsForFeature does not crash");
Return<void> ret = effect->getSupportedConfigsForFeature(