Skip to content

Commit 21f648f

Browse files
Eric Laurentandi34
Eric Laurent
authored and
andi34
committed
DO NOT MERGE - audio effects: fix heap overflow
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
1 parent 1060675 commit 21f648f

File tree

6 files changed

+103
-154
lines changed

6 files changed

+103
-154
lines changed

‎media/libeffects/downmix/EffectDownmix.c

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,8 @@ void Downmix_testIndexComputation(uint32_t mask) {
160160
/*--- Effect Library Interface Implementation ---*/
161161

162162
int32_t DownmixLib_Create(const effect_uuid_t *uuid,
163-
int32_t sessionId,
164-
int32_t ioId,
163+
int32_t sessionId __unused,
164+
int32_t ioId __unused,
165165
effect_handle_t *pHandle) {
166166
int ret;
167167
int i;
@@ -384,15 +384,15 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
384384

385385
switch (cmdCode) {
386386
case EFFECT_CMD_INIT:
387-
if (pReplyData == NULL || *replySize != sizeof(int)) {
387+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
388388
return -EINVAL;
389389
}
390390
*(int *) pReplyData = Downmix_Init(pDwmModule);
391391
break;
392392

393393
case EFFECT_CMD_SET_CONFIG:
394394
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t)
395-
|| pReplyData == NULL || *replySize != sizeof(int)) {
395+
|| pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
396396
return -EINVAL;
397397
}
398398
*(int *) pReplyData = Downmix_Configure(pDwmModule,
@@ -407,7 +407,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
407407
ALOGV("Downmix_Command EFFECT_CMD_GET_PARAM pCmdData %p, *replySize %d, pReplyData: %p",
408408
pCmdData, *replySize, pReplyData);
409409
if (pCmdData == NULL || cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
410-
pReplyData == NULL ||
410+
pReplyData == NULL || replySize == NULL ||
411411
*replySize < (int) sizeof(effect_param_t) + 2 * sizeof(int32_t)) {
412412
return -EINVAL;
413413
}
@@ -424,7 +424,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
424424
ALOGV("Downmix_Command EFFECT_CMD_SET_PARAM cmdSize %d pCmdData %p, *replySize %d, " \
425425
"pReplyData %p", cmdSize, pCmdData, *replySize, pReplyData);
426426
if (pCmdData == NULL || (cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)))
427-
|| pReplyData == NULL || *replySize != (int)sizeof(int32_t)) {
427+
|| pReplyData == NULL || replySize == NULL || *replySize != (int)sizeof(int32_t)) {
428428
return -EINVAL;
429429
}
430430
effect_param_t *cmd = (effect_param_t *) pCmdData;
@@ -443,7 +443,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
443443
break;
444444

445445
case EFFECT_CMD_ENABLE:
446-
if (pReplyData == NULL || *replySize != sizeof(int)) {
446+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
447447
return -EINVAL;
448448
}
449449
if (pDownmixer->state != DOWNMIX_STATE_INITIALIZED) {
@@ -455,7 +455,7 @@ static int Downmix_Command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdS
455455
break;
456456

457457
case EFFECT_CMD_DISABLE:
458-
if (pReplyData == NULL || *replySize != sizeof(int)) {
458+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
459459
return -EINVAL;
460460
}
461461
if (pDownmixer->state != DOWNMIX_STATE_ACTIVE) {
@@ -670,7 +670,7 @@ int Downmix_Configure(downmix_module_t *pDwmModule, effect_config_t *pConfig, bo
670670
*----------------------------------------------------------------------------
671671
*/
672672

673-
int Downmix_Reset(downmix_object_t *pDownmixer, bool init) {
673+
int Downmix_Reset(downmix_object_t *pDownmixer __unused, bool init __unused) {
674674
// nothing to do here
675675
return 0;
676676
}

‎media/libeffects/loudness/EffectLoudnessEnhancer.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,8 @@ int LE_init(LoudnessEnhancerContext *pContext)
189189
//
190190

191191
int LELib_Create(const effect_uuid_t *uuid,
192-
int32_t sessionId,
193-
int32_t ioId,
192+
int32_t sessionId __unused,
193+
int32_t ioId __unused,
194194
effect_handle_t *pHandle) {
195195
ALOGV("LELib_Create()");
196196
int ret;
@@ -327,7 +327,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
327327
break;
328328
case EFFECT_CMD_SET_CONFIG:
329329
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t)
330-
|| pReplyData == NULL || *replySize != sizeof(int)) {
330+
|| pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
331331
return -EINVAL;
332332
}
333333
*(int *) pReplyData = LE_setConfig(pContext,
@@ -344,7 +344,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
344344
LE_reset(pContext);
345345
break;
346346
case EFFECT_CMD_ENABLE:
347-
if (pReplyData == NULL || *replySize != sizeof(int)) {
347+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
348348
return -EINVAL;
349349
}
350350
if (pContext->mState != LOUDNESS_ENHANCER_STATE_INITIALIZED) {
@@ -368,7 +368,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
368368
case EFFECT_CMD_GET_PARAM: {
369369
if (pCmdData == NULL ||
370370
cmdSize != (int)(sizeof(effect_param_t) + sizeof(uint32_t)) ||
371-
pReplyData == NULL ||
371+
pReplyData == NULL || replySize == NULL ||
372372
*replySize < (int)(sizeof(effect_param_t) + sizeof(uint32_t) + sizeof(uint32_t))) {
373373
return -EINVAL;
374374
}
@@ -394,7 +394,7 @@ int LE_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize,
394394
case EFFECT_CMD_SET_PARAM: {
395395
if (pCmdData == NULL ||
396396
cmdSize != (int)(sizeof(effect_param_t) + sizeof(uint32_t) + sizeof(uint32_t)) ||
397-
pReplyData == NULL || *replySize != sizeof(int32_t)) {
397+
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
398398
return -EINVAL;
399399
}
400400
*(int32_t *)pReplyData = 0;

‎media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp

Lines changed: 42 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -2752,7 +2752,7 @@ int Effect_command(effect_handle_t self,
27522752

27532753
switch (cmdCode){
27542754
case EFFECT_CMD_INIT:
2755-
if (pReplyData == NULL || *replySize != sizeof(int)){
2755+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)){
27562756
ALOGV("\tLVM_ERROR, EFFECT_CMD_INIT: ERROR for effect type %d",
27572757
pContext->EffectType);
27582758
return -EINVAL;
@@ -2779,10 +2779,8 @@ int Effect_command(effect_handle_t self,
27792779

27802780
case EFFECT_CMD_SET_CONFIG:
27812781
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_SET_CONFIG start");
2782-
if (pCmdData == NULL||
2783-
cmdSize != sizeof(effect_config_t)||
2784-
pReplyData == NULL||
2785-
*replySize != sizeof(int)){
2782+
if (pCmdData == NULL || cmdSize != sizeof(effect_config_t) ||
2783+
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
27862784
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
27872785
"EFFECT_CMD_SET_CONFIG: ERROR");
27882786
return -EINVAL;
@@ -2792,8 +2790,7 @@ int Effect_command(effect_handle_t self,
27922790
break;
27932791

27942792
case EFFECT_CMD_GET_CONFIG:
2795-
if (pReplyData == NULL ||
2796-
*replySize != sizeof(effect_config_t)) {
2793+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(effect_config_t)) {
27972794
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
27982795
"EFFECT_CMD_GET_CONFIG: ERROR");
27992796
return -EINVAL;
@@ -2811,30 +2808,27 @@ int Effect_command(effect_handle_t self,
28112808
case EFFECT_CMD_GET_PARAM:{
28122809
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM start");
28132810

2814-
if(pContext->EffectType == LVM_BASS_BOOST){
2815-
if (pCmdData == NULL ||
2816-
cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
2817-
pReplyData == NULL ||
2818-
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))){
2819-
ALOGV("\tLVM_ERROR : BassBoost_command cmdCode Case: "
2820-
"EFFECT_CMD_GET_PARAM: ERROR");
2821-
return -EINVAL;
2822-
}
2823-
effect_param_t *p = (effect_param_t *)pCmdData;
2811+
effect_param_t *p = (effect_param_t *)pCmdData;
2812+
2813+
if (pCmdData == NULL || cmdSize < sizeof(effect_param_t) ||
2814+
cmdSize < (sizeof(effect_param_t) + p->psize) ||
2815+
pReplyData == NULL || replySize == NULL ||
2816+
*replySize < (sizeof(effect_param_t) + p->psize)) {
2817+
ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: ERROR");
2818+
return -EINVAL;
2819+
}
28242820

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

2827-
p = (effect_param_t *)pReplyData;
2823+
p = (effect_param_t *)pReplyData;
28282824

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

2827+
if(pContext->EffectType == LVM_BASS_BOOST){
28312828
p->status = android::BassBoost_getParameter(pContext,
28322829
p->data,
28332830
(size_t *)&p->vsize,
28342831
p->data + voffset);
2835-
2836-
*replySize = sizeof(effect_param_t) + voffset + p->vsize;
2837-
28382832
//ALOGV("\tBassBoost_command EFFECT_CMD_GET_PARAM "
28392833
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
28402834
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)),
@@ -2843,29 +2837,11 @@ int Effect_command(effect_handle_t self,
28432837
}
28442838

28452839
if(pContext->EffectType == LVM_VIRTUALIZER){
2846-
if (pCmdData == NULL ||
2847-
cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
2848-
pReplyData == NULL ||
2849-
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))){
2850-
ALOGV("\tLVM_ERROR : Virtualizer_command cmdCode Case: "
2851-
"EFFECT_CMD_GET_PARAM: ERROR");
2852-
return -EINVAL;
2853-
}
2854-
effect_param_t *p = (effect_param_t *)pCmdData;
2855-
2856-
memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);
2857-
2858-
p = (effect_param_t *)pReplyData;
2859-
2860-
int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);
2861-
28622840
p->status = android::Virtualizer_getParameter(pContext,
28632841
(void *)p->data,
28642842
(size_t *)&p->vsize,
28652843
p->data + voffset);
28662844

2867-
*replySize = sizeof(effect_param_t) + voffset + p->vsize;
2868-
28692845
//ALOGV("\tVirtualizer_command EFFECT_CMD_GET_PARAM "
28702846
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
28712847
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)),
@@ -2875,29 +2851,11 @@ int Effect_command(effect_handle_t self,
28752851
if(pContext->EffectType == LVM_EQUALIZER){
28762852
//ALOGV("\tEqualizer_command cmdCode Case: "
28772853
// "EFFECT_CMD_GET_PARAM start");
2878-
if (pCmdData == NULL ||
2879-
cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
2880-
pReplyData == NULL ||
2881-
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))) {
2882-
ALOGV("\tLVM_ERROR : Equalizer_command cmdCode Case: "
2883-
"EFFECT_CMD_GET_PARAM");
2884-
return -EINVAL;
2885-
}
2886-
effect_param_t *p = (effect_param_t *)pCmdData;
2887-
2888-
memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);
2889-
2890-
p = (effect_param_t *)pReplyData;
2891-
2892-
int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);
2893-
28942854
p->status = android::Equalizer_getParameter(pContext,
28952855
p->data,
28962856
&p->vsize,
28972857
p->data + voffset);
28982858

2899-
*replySize = sizeof(effect_param_t) + voffset + p->vsize;
2900-
29012859
//ALOGV("\tEqualizer_command EFFECT_CMD_GET_PARAM *pCmdData %d, *replySize %d, "
29022860
// "*pReplyData %08x %08x",
29032861
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)), *replySize,
@@ -2907,35 +2865,19 @@ int Effect_command(effect_handle_t self,
29072865
}
29082866
if(pContext->EffectType == LVM_VOLUME){
29092867
//ALOGV("\tVolume_command cmdCode Case: EFFECT_CMD_GET_PARAM start");
2910-
if (pCmdData == NULL ||
2911-
cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
2912-
pReplyData == NULL ||
2913-
*replySize < (int) (sizeof(effect_param_t) + sizeof(int32_t))){
2914-
ALOGV("\tLVM_ERROR : Volume_command cmdCode Case: "
2915-
"EFFECT_CMD_GET_PARAM: ERROR");
2916-
return -EINVAL;
2917-
}
2918-
effect_param_t *p = (effect_param_t *)pCmdData;
2919-
2920-
memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);
2921-
2922-
p = (effect_param_t *)pReplyData;
2923-
2924-
int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);
2925-
29262868
p->status = android::Volume_getParameter(pContext,
29272869
(void *)p->data,
29282870
(size_t *)&p->vsize,
29292871
p->data + voffset);
29302872

2931-
*replySize = sizeof(effect_param_t) + voffset + p->vsize;
2932-
29332873
//ALOGV("\tVolume_command EFFECT_CMD_GET_PARAM "
29342874
// "*pCmdData %d, *replySize %d, *pReplyData %d ",
29352875
// *(int32_t *)((char *)pCmdData + sizeof(effect_param_t)),
29362876
// *replySize,
29372877
// *(int16_t *)((char *)pReplyData + sizeof(effect_param_t) + voffset));
29382878
}
2879+
*replySize = sizeof(effect_param_t) + voffset + p->vsize;
2880+
29392881
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM end");
29402882
} break;
29412883
case EFFECT_CMD_SET_PARAM:{
@@ -2946,10 +2888,9 @@ int Effect_command(effect_handle_t self,
29462888
// *replySize,
29472889
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));
29482890

2949-
if (pCmdData == NULL||
2950-
cmdSize != (int)(sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t))||
2951-
pReplyData == NULL||
2952-
*replySize != sizeof(int32_t)){
2891+
if (pCmdData == NULL ||
2892+
cmdSize != (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t)) ||
2893+
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
29532894
ALOGV("\tLVM_ERROR : BassBoost_command cmdCode Case: "
29542895
"EFFECT_CMD_SET_PARAM: ERROR");
29552896
return -EINVAL;
@@ -2979,10 +2920,11 @@ int Effect_command(effect_handle_t self,
29792920
// *replySize,
29802921
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));
29812922

2982-
if (pCmdData == NULL||
2983-
cmdSize != (int)(sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t))||
2984-
pReplyData == NULL||
2985-
*replySize != sizeof(int32_t)){
2923+
if (pCmdData == NULL ||
2924+
// legal parameters are int16_t or int32_t
2925+
cmdSize > (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int32_t)) ||
2926+
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t) +sizeof(int16_t)) ||
2927+
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
29862928
ALOGV("\tLVM_ERROR : Virtualizer_command cmdCode Case: "
29872929
"EFFECT_CMD_SET_PARAM: ERROR");
29882930
return -EINVAL;
@@ -3014,8 +2956,8 @@ int Effect_command(effect_handle_t self,
30142956
// *replySize,
30152957
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) + sizeof(int32_t)));
30162958

3017-
if (pCmdData == NULL || cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t)) ||
3018-
pReplyData == NULL || *replySize != sizeof(int32_t)) {
2959+
if (pCmdData == NULL || cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
2960+
pReplyData == NULL || replySize == NULL || *replySize != sizeof(int32_t)) {
30192961
ALOGV("\tLVM_ERROR : Equalizer_command cmdCode Case: "
30202962
"EFFECT_CMD_SET_PARAM: ERROR");
30212963
return -EINVAL;
@@ -3033,10 +2975,10 @@ int Effect_command(effect_handle_t self,
30332975
// *replySize,
30342976
// *(int16_t *)((char *)pCmdData + sizeof(effect_param_t) +sizeof(int32_t)));
30352977

3036-
if ( pCmdData == NULL||
3037-
cmdSize < (int)(sizeof(effect_param_t) + sizeof(int32_t))||
3038-
pReplyData == NULL||
3039-
*replySize != sizeof(int32_t)){
2978+
if (pCmdData == NULL ||
2979+
cmdSize < (sizeof(effect_param_t) + sizeof(int32_t)) ||
2980+
pReplyData == NULL || replySize == NULL ||
2981+
*replySize != sizeof(int32_t)) {
30402982
ALOGV("\tLVM_ERROR : Volume_command cmdCode Case: "
30412983
"EFFECT_CMD_SET_PARAM: ERROR");
30422984
return -EINVAL;
@@ -3052,7 +2994,7 @@ int Effect_command(effect_handle_t self,
30522994

30532995
case EFFECT_CMD_ENABLE:
30542996
ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_ENABLE start");
3055-
if (pReplyData == NULL || *replySize != sizeof(int)){
2997+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
30562998
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_ENABLE: ERROR");
30572999
return -EINVAL;
30583000
}
@@ -3062,7 +3004,7 @@ int Effect_command(effect_handle_t self,
30623004

30633005
case EFFECT_CMD_DISABLE:
30643006
//ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_DISABLE start");
3065-
if (pReplyData == NULL || *replySize != sizeof(int)){
3007+
if (pReplyData == NULL || replySize == NULL || *replySize != sizeof(int)) {
30663008
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_DISABLE: ERROR");
30673009
return -EINVAL;
30683010
}
@@ -3072,6 +3014,11 @@ int Effect_command(effect_handle_t self,
30723014
case EFFECT_CMD_SET_DEVICE:
30733015
{
30743016
ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_SET_DEVICE start");
3017+
if (pCmdData == NULL){
3018+
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: EFFECT_CMD_SET_DEVICE: ERROR");
3019+
return -EINVAL;
3020+
}
3021+
30753022
uint32_t device = *(uint32_t *)pCmdData;
30763023

30773024
if (pContext->EffectType == LVM_BASS_BOOST) {
@@ -3158,8 +3105,8 @@ int Effect_command(effect_handle_t self,
31583105
break;
31593106
}
31603107

3161-
if (pCmdData == NULL ||
3162-
cmdSize != 2 * sizeof(uint32_t)) {
3108+
if (pCmdData == NULL || cmdSize != 2 * sizeof(uint32_t) || pReplyData == NULL ||
3109+
replySize == NULL || *replySize < 2*sizeof(int32_t)) {
31633110
ALOGV("\tLVM_ERROR : Effect_command cmdCode Case: "
31643111
"EFFECT_CMD_SET_VOLUME: ERROR");
31653112
return -EINVAL;

0 commit comments

Comments
 (0)