Navigation Menu

Skip to content

Commit

Permalink
audio effects: fix heap overflow
Browse files Browse the repository at this point in the history 
Check consistency of effect command reply sizes before
copying to reply address.

Also add null pointer check on reply size.
Also remove unused parameter warning.

Bug: 21953516.
Change-Id: I4cf00c12eaed696af28f3b7613f7e36f47a160c4
(cherry picked from commit 0f714a4)
  • Loading branch information
@ciwrl
Eric Laurent authored and ciwrl committed Aug 18, 2015
1 parent 89115df commit 1328bce
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 149 deletions.
18 changes: 9 additions & 9 deletions media/libeffects/downmix/EffectDownmix.c
View file
Expand Up @@ -149,8 +149,8 @@ void Downmix_testIndexComputation(uint32_t mask) {
/*--- Effect Library Interface Implementation ---*/

int32_t DownmixLib_Create(const effect_uuid_t *uuid,
int32_t sessionId,
int32_t ioId,
int32_t sessionId __unused,
int32_t ioId __unused,
effect_handle_t *pHandle) {
int ret;
int i;
Expand Down Expand Up @@ -370,15 +370,15 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS

switch (cmdCode) {
case EFFECT_CMD_INIT:
if (pReplyData == NULL || *replySize != sizeof(int)) {
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
*(int *) pReplyData = Downmix_Init(pDwmModule);
break;

case EFFECT_CMD_SET_CONFIG:
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t)
|| pReplyData == NULL || *replySize != sizeof(int)) {
|| pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
*(int *) pReplyData = Downmix_Configure(pDwmModule,
Expand All @@ -393,7 +393,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
ALOGV("Downmix_Command EFFECT_CMD_GET_PARAM pCmdData %p, *replySize %" PRIu32 ", pReplyData: %p",
pCmdData, *replySize, pReplyData);
if (pCmdData == NULL || cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL ||
pReplyData == NULL || replySize == NULL ||
*replySize < (int) sizeof(effect_param_t) + 2 * sizeof(int32_t)) {
return -EINVAL;
}
Expand All @@ -410,7 +410,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
ALOGV("Downmix_Command EFFECT_CMD_SET_PARAM cmdSize %d pCmdData %p, *replySize %" PRIu32
", pReplyData %p", cmdSize, pCmdData, *replySize, pReplyData);
if (pCmdData == NULL || (cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)))
|| pReplyData == NULL || *replySize != (int)sizeof(int32_t)) {
|| pReplyData == NULL || replySize == NULL || *replySize != (int)sizeof(int32_t)) {
return -EINVAL;
}
effect_param_t *cmd = (effect_param_t *) pCmdData;
Expand All @@ -429,7 +429,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
break;

case EFFECT_CMD_ENABLE:
if (pReplyData == NULL || *replySize != sizeof(int)) {
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
if (pDownmixer->state != DOWNMIX_STATE_INITIALIZED) {
Expand All @@ -441,7 +441,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
break;

case EFFECT_CMD_DISABLE:
if (pReplyData == NULL || *replySize != sizeof(int)) {
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
if (pDownmixer->state != DOWNMIX_STATE_ACTIVE) {
Expand Down Expand Up @@ -659,7 +659,7 @@ int Downmix_Configure(downmix_module_t *pDwmModule, effect_config_t *pConfig, bo
*----------------------------------------------------------------------------
*/

int Downmix_Reset(downmix_object_t *pDownmixer, bool init) {
int Downmix_Reset(downmix_object_t *pDownmixer __unused, bool init __unused) {
// nothing to do here
return 0;
}
Expand Down
12 changes: 6 additions & 6 deletions media/libeffects/loudness/EffectLoudnessEnhancer.cpp
View file
Expand Up @@ -189,8 +189,8 @@ int LE_init(LoudnessEnhancerContext *pContext)
//

int LELib_Create(const effect_uuid_t *uuid,
int32_t sessionId,
int32_t ioId,
int32_t sessionId __unused,
int32_t ioId __unused,
effect_handle_t *pHandle) {
ALOGV("LELib_Create()");
int ret;
Expand Down Expand Up @@ -327,7 +327,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
break;
case EFFECT_CMD_SET_CONFIG:
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t)
|| pReplyData == NULL || *replySize != sizeof(int)) {
|| pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
*(int *) pReplyData = LE_setConfig(pContext,
Expand All @@ -344,7 +344,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
LE_reset(pContext);
break;
case EFFECT_CMD_ENABLE:
if (pReplyData == NULL || *replySize != sizeof(int)) {
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
return -EINVAL;
}
if (pContext->mState != LOUDNESS_ENHANCER_STATE_INITIALIZED) {
Expand All @@ -368,7 +368,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
case EFFECT_CMD_GET_PARAM: {
if (pCmdData == NULL ||
cmdSize != (int)(sizeof(effect_param_t) + sizeof(uint32_t)) ||
pReplyData == NULL ||
pReplyData == NULL || replySize == NULL ||
*replySize < (int)(sizeof(effect_param_t) + sizeof(uint32_t) + sizeof(uint32_t))) {
return -EINVAL;
}
Expand All @@ -394,7 +394,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
case EFFECT_CMD_SET_PARAM: {
if (pCmdData == NULL ||
cmdSize != (int)(sizeof(effect_param_t) + sizeof(uint32_t) + sizeof(uint32_t)) ||
pReplyData == NULL || *replySize != sizeof(int32_t)) {
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
return -EINVAL;
}
*(int32_t *)pReplyData = 0;
Expand Down
134 changes: 40 additions & 94 deletions media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp
View file
Expand Up @@ -2979,7 +2979,7 @@ int Effect_command(effect_handle_t self,

switch (cmdCode){
case EFFECT_CMD_INIT:
if (pReplyData == NULL || *replySize != sizeof(int)){
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)){
ALOGV("\tLVM_ERROR, EFFECT_CMD_INIT: ERROR for effect type %d",
pContext->EffectType);
return -EINVAL;
Expand All @@ -3006,10 +3006,8 @@ int Effect_command(effect_handle_t self,

case EFFECT_CMD_SET_CONFIG:
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_SET_CONFIG start");
if (pCmdData == NULL||
cmdSize != sizeof(effect_config_t)||
pReplyData == NULL||
*replySize != sizeof(int)){
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t) ||
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
"EFFECT_CMD_SET_CONFIG: ERROR");
return -EINVAL;
Expand All @@ -3019,8 +3017,7 @@ int Effect_command(effect_handle_t self,
break;

case EFFECT_CMD_GET_CONFIG:
if (pReplyData == NULL ||
*replySize != sizeof(effect_config_t)) {
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(effect_config_t)) {
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
"EFFECT_CMD_GET_CONFIG: ERROR");
return -EINVAL;
Expand All @@ -3038,30 +3035,27 @@ int Effect_command(effect_handle_t self,
case EFFECT_CMD_GET_PARAM:{
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM start");

if(pContext->EffectType == LVM_BASS_BOOST){
if (pCmdData == NULL ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL ||
*replySize < (sizeof(effect_param_t) + sizeof(int32_t))){
ALOGV("\tLVM_ERROR : BassBoost_command cmdCode Case: "
"EFFECT_CMD_GET_PARAM: ERROR");
return -EINVAL;
}
effect_param_t *p = (effect_param_t *)pCmdData;
effect_param_t *p = (effect_param_t *)pCmdData;

if (pCmdData == NULL || cmdSize < sizeof(effect_param_t) ||
cmdSize < (sizeof(effect_param_t) + p->psize) ||
pReplyData == NULL || replySize == NULL ||
*replySize < (sizeof(effect_param_t) + p->psize)) {
ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: ERROR");
return -EINVAL;
}

memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);
memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);

p = (effect_param_t *)pReplyData;
p = (effect_param_t *)pReplyData;

int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);
int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);

if(pContext->EffectType == LVM_BASS_BOOST){
p->status = android::BassBoost_getParameter(pContext,
p->data,
&p->vsize,
p->data + voffset);

*replySize = sizeof(effect_param_t) + voffset + p->vsize;

//ALOGV("\tBassBoost_command EFFECT_CMD_GET_PARAM "
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)),
Expand All @@ -3070,27 +3064,10 @@ int Effect_command(effect_handle_t self,
}

if(pContext->EffectType == LVM_VIRTUALIZER){
if (pCmdData == NULL ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL ||
*replySize < (sizeof(effect_param_t) + sizeof(int32_t))){
ALOGV("\tLVM_ERROR : Virtualizer_command cmdCode Case: "
"EFFECT_CMD_GET_PARAM: ERROR");
return -EINVAL;
}
effect_param_t *p = (effect_param_t *)pCmdData;

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);

p->status = android::Virtualizer_getParameter(pContext,
(void *)p->data,
&p->vsize,
p->data + voffset);
*replySize = sizeof(effect_param_t) + voffset + p->vsize;

//ALOGV("\tVirtualizer_command EFFECT_CMD_GET_PARAM "
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
Expand All @@ -3101,29 +3078,11 @@ int Effect_command(effect_handle_t self,
if(pContext->EffectType == LVM_EQUALIZER){
//ALOGV("\tEqualizer_command cmdCode Case: "
// "EFFECT_CMD_GET_PARAM start");
if (pCmdData == NULL ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL ||
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))) {
ALOGV("\tLVM_ERROR : Equalizer_command cmdCode Case: "
"EFFECT_CMD_GET_PARAM");
return -EINVAL;
}
effect_param_t *p = (effect_param_t *)pCmdData;

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);

p->status = android::Equalizer_getParameter(pContext,
p->data,
&p->vsize,
p->data + voffset);

*replySize = sizeof(effect_param_t) + voffset + p->vsize;

//ALOGV("\tEqualizer_command EFFECT_CMD_GET_PARAM *pCmdData %d, *replySize %d, "
// "*pReplyData %08x %08x",
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)), *replySize,
Expand All @@ -3133,35 +3092,19 @@ int Effect_command(effect_handle_t self,
}
if(pContext->EffectType == LVM_VOLUME){
//ALOGV("\tVolume_command cmdCode Case: EFFECT_CMD_GET_PARAM start");
if (pCmdData == NULL ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL ||
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))){
ALOGV("\tLVM_ERROR : Volume_command cmdCode Case: "
"EFFECT_CMD_GET_PARAM: ERROR");
return -EINVAL;
}
effect_param_t *p = (effect_param_t *)pCmdData;

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);

p->status = android::Volume_getParameter(pContext,
(void *)p->data,
&p->vsize,
p->data + voffset);

*replySize = sizeof(effect_param_t) + voffset + p->vsize;

//ALOGV("\tVolume_command EFFECT_CMD_GET_PARAM "
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)),
// *replySize,
// *(int16_t *)((char *)pReplyData + sizeof(effect_param_t) + voffset));
}
*replySize = sizeof(effect_param_t) + voffset + p->vsize;

//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM end");
} break;
case EFFECT_CMD_SET_PARAM:{
Expand All @@ -3172,10 +3115,9 @@ int Effect_command(effect_handle_t self,
// *replySize,
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));

if (pCmdData == NULL||
cmdSize != (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t))||
pReplyData == NULL||
*replySize != sizeof(int32_t)){
if (pCmdData == NULL ||
cmdSize != (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t)) ||
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
ALOGV("\tLVM_ERROR : BassBoost_command cmdCode Case: "
"EFFECT_CMD_SET_PARAM: ERROR");
return -EINVAL;
Expand Down Expand Up @@ -3207,11 +3149,10 @@ int Effect_command(effect_handle_t self,
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));

if (pCmdData == NULL ||
// legal parameters are int16_t or int32_t
cmdSize > (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int32_t)) ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t)) ||
pReplyData == NULL ||
*replySize != sizeof(int32_t)){
// legal parameters are int16_t or int32_t
cmdSize > (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int32_t)) ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t)) ||
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
ALOGV("\tLVM_ERROR : Virtualizer_command cmdCode Case: "
"EFFECT_CMD_SET_PARAM: ERROR");
return -EINVAL;
Expand Down Expand Up @@ -3244,7 +3185,7 @@ int Effect_command(effect_handle_t self,
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));

if (pCmdData == NULL || cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL || *replySize != sizeof(int32_t)) {
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
ALOGV("\tLVM_ERROR : Equalizer_command cmdCode Case: "
"EFFECT_CMD_SET_PARAM: ERROR");
return -EINVAL;
Expand All @@ -3262,10 +3203,10 @@ int Effect_command(effect_handle_t self,
// *replySize,
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) +sizeof(int32_t)));

if ( pCmdData == NULL||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t))||
pReplyData == NULL||
*replySize != sizeof(int32_t)){
if (pCmdData == NULL ||
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
pReplyData == NULL || replySize == NULL ||
*replySize != sizeof(int32_t)) {
ALOGV("\tLVM_ERROR : Volume_command cmdCode Case: "
"EFFECT_CMD_SET_PARAM: ERROR");
return -EINVAL;
Expand All @@ -3281,7 +3222,7 @@ int Effect_command(effect_handle_t self,

case EFFECT_CMD_ENABLE:
ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_ENABLE start");
if (pReplyData == NULL || *replySize != sizeof(int)){
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_ENABLE: ERROR");
return -EINVAL;
}
Expand All @@ -3291,7 +3232,7 @@ int Effect_command(effect_handle_t self,

case EFFECT_CMD_DISABLE:
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_DISABLE start");
if (pReplyData == NULL || *replySize != sizeof(int)){
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_DISABLE: ERROR");
return -EINVAL;
}
Expand All @@ -3301,6 +3242,11 @@ int Effect_command(effect_handle_t self,
case EFFECT_CMD_SET_DEVICE:
{
ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_SET_DEVICE start");
if (pCmdData == NULL){
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_SET_DEVICE: ERROR");
return -EINVAL;
}

uint32_t device = *(uint32_t *)pCmdData;
pContext->pBundledContext->nOutputDevice = (audio_devices_t) device;

Expand Down Expand Up @@ -3389,8 +3335,8 @@ int Effect_command(effect_handle_t self,
break;
}

if (pCmdData == NULL ||
cmdSize != 2 * sizeof(uint32_t)) {
if (pCmdData == NULL || cmdSize != 2 * sizeof(uint32_t) || pReplyData == NULL ||
replySize == NULL || *replySize < 2*sizeof(int32_t)) {
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
"EFFECT_CMD_SET_VOLUME: ERROR");
return -EINVAL;
Expand Down

2 comments on commit 1328bce

@taltum
Copy link

@taltum taltum commented on 1328bce Aug 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: 21953516.
Change-Id: I4cf00c12eaed696af28f3b7613f7e36f47a160c4
(cherry picked from commit 0f714a4)

@taltum
Copy link

@taltum taltum commented on 1328bce Aug 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

media/libeffects/downmix/EffectDownmix.c

Please sign in to comment.