From 0574c56e88e96d33c923a8f54364ac0bf3dc5a91 Mon Sep 17 00:00:00 2001 From: rago Date: Tue, 22 Nov 2016 18:02:48 -0800 Subject: Fix security vulnerability: potential OOB write in audioserver Bug: 32705438 Bug: 32703959 Test: cts security test Change-Id: I8900c92fa55b56c4c2c9d721efdbabe6bfc8a4a4 (cherry picked from commit e275907e576601a3579747c3a842790bacf111e2) (cherry picked from commit b0bcddb44d992e74140a3f5eedc7177977ea8e34) --- .../libeffects/lvm/wrapper/Bundle/EffectBundle.cpp | 27 +++++++++++++++++----- media/libmedia/IEffect.cpp | 12 ++++++++++ services/audioflinger/Effects.cpp | 16 +++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp index 5e975b0..2588140 100644 --- a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp +++ b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp @@ -3124,10 +3124,6 @@ int Effect_command(effect_handle_t self, //ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM start"); effect_param_t *p = (effect_param_t *)pCmdData; - if (SIZE_MAX - sizeof(effect_param_t) < (size_t)p->psize) { - android_errorWriteLog(0x534e4554, "26347509"); - return -EINVAL; - } if (pCmdData == NULL || cmdSize < sizeof(effect_param_t) || cmdSize < (sizeof(effect_param_t) + p->psize) || pReplyData == NULL || replySize == NULL || @@ -3135,13 +3131,32 @@ int Effect_command(effect_handle_t self, ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: ERROR"); return -EINVAL; } + if (EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) < (size_t)p->psize) { + android_errorWriteLog(0x534e4554, "26347509"); + ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: psize too big"); + return -EINVAL; + } + uint32_t paddedParamSize = ((p->psize + sizeof(int32_t) - 1) / sizeof(int32_t)) * + sizeof(int32_t); + if ((EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) < paddedParamSize) || + (EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) - paddedParamSize < + p->vsize)) { + ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: padded_psize or vsize too big"); + return -EINVAL; + } + uint32_t expectedReplySize = sizeof(effect_param_t) + paddedParamSize + p->vsize; + if (*replySize < expectedReplySize) { + ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: min. replySize %u, got %u bytes", + expectedReplySize, *replySize); + android_errorWriteLog(0x534e4554, "32705438"); + return -EINVAL; + } memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize); p = (effect_param_t *)pReplyData; - int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t); - + uint32_t voffset = paddedParamSize; if(pContext->EffectType == LVM_BASS_BOOST){ p->status = android::BassBoost_getParameter(pContext, p->data, diff --git a/media/libmedia/IEffect.cpp b/media/libmedia/IEffect.cpp index faf5795..af6d8de 100644 --- a/media/libmedia/IEffect.cpp +++ b/media/libmedia/IEffect.cpp @@ -25,6 +25,9 @@ namespace android { +// Maximum command/reply size expected +#define EFFECT_PARAM_SIZE_MAX 65536 + enum { ENABLE = IBinder::FIRST_CALL_TRANSACTION, DISABLE, @@ -156,6 +159,10 @@ status_t BnEffect::onTransact( uint32_t cmdSize = data.readInt32(); char *cmd = NULL; if (cmdSize) { + if (cmdSize > EFFECT_PARAM_SIZE_MAX) { + reply->writeInt32(NO_MEMORY); + return NO_ERROR; + } cmd = (char *)calloc(cmdSize, 1); if (cmd == NULL) { reply->writeInt32(NO_MEMORY); @@ -167,6 +174,11 @@ status_t BnEffect::onTransact( uint32_t replySz = replySize; char *resp = NULL; if (replySize) { + if (replySize > EFFECT_PARAM_SIZE_MAX) { + free(cmd); + reply->writeInt32(NO_MEMORY); + return NO_ERROR; + } resp = (char *)calloc(replySize, 1); if (resp == NULL) { free(cmd); diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index 27dfa05..b9fe741 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -578,6 +578,22 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode, android_errorWriteLog(0x534e4554, "32438594"); return -EINVAL; } + if (cmdCode == EFFECT_CMD_GET_PARAM && + (sizeof(effect_param_t) > *replySize + || ((effect_param_t *)pCmdData)->psize > *replySize + - sizeof(effect_param_t) + || ((effect_param_t *)pCmdData)->vsize > *replySize + - sizeof(effect_param_t) + - ((effect_param_t *)pCmdData)->psize + || roundUpDelta(((effect_param_t *)pCmdData)->psize, (uint32_t)sizeof(int)) > + *replySize + - sizeof(effect_param_t) + - ((effect_param_t *)pCmdData)->psize + - ((effect_param_t *)pCmdData)->vsize)) { + ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: reply size inconsistent"); + android_errorWriteLog(0x534e4554, "32705438"); + return -EINVAL; + } if ((cmdCode == EFFECT_CMD_SET_PARAM || cmdCode == EFFECT_CMD_SET_PARAM_DEFERRED) && // DEFERRED not generally used (sizeof(effect_param_t) > cmdSize -- cgit v1.1