From 82016b05946bd41ecbaf6872c00b0195ea80c094 Mon Sep 17 00:00:00 2001 From: Sam Mortimer Date: Fri, 9 Dec 2016 13:36:25 -0800 Subject: soundtrigger: fix memory corruption Fixes hotword on angler. Change-Id: Ic15a617c0f79f03785feaddd2dfa6deb90842a06 (cherry picked from commit 5f72b2213b9dc96ce91871398b539ad6aa653142) --- services/soundtrigger/SoundTriggerHwService.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/soundtrigger/SoundTriggerHwService.cpp b/services/soundtrigger/SoundTriggerHwService.cpp index a1cc6ff..a45d5f6 100644 --- a/services/soundtrigger/SoundTriggerHwService.cpp +++ b/services/soundtrigger/SoundTriggerHwService.cpp @@ -270,12 +270,12 @@ void SoundTriggerHwService::sendRecognitionEvent(struct sound_trigger_recognitio if (module == NULL) { return; } + struct sound_trigger_phrase_recognition_event newEvent; if (event-> type == SOUND_MODEL_TYPE_KEYPHRASE && event->data_size != 0 && event->data_offset != sizeof(struct sound_trigger_phrase_recognition_event)) { // set some defaults for the phrase if the recognition event won't be parsed properly // TODO: read defaults from the config - struct sound_trigger_phrase_recognition_event newEvent; memset(&newEvent, 0, sizeof(struct sound_trigger_phrase_recognition_event)); sp model = module->getModel(event->model); -- cgit v1.1 From 621ca73010f3954566b27c6554ce992cc6069670 Mon Sep 17 00:00:00 2001 From: rago Date: Mon, 14 Nov 2016 14:58:34 -0800 Subject: Fix security vulnerability: Effect command might allow negative indexes Bug: 32448258 Bug: 32095626 Test: Use POC bug or cts security test Change-Id: I69f24eac5866f8d9090fc4c0ebe58c2c297b63df (cherry picked from commit 01183402d757f0c28bfd5e3b127b3809dfd67459) (cherry picked from commit 321ea5257e37c8edb26e66fe4ee78cca4cd915fe) Fix security vulnerability: Equalizer command might allow negative indexes Bug: 32247948 Bug: 32438598 Bug: 32436341 Test: use POC on bug or cts security test Change-Id: I91bd6aadb6c7410163e03101f365db767f4cd2a3 (cherry picked from commit 0872b65cff9129633471945431b9a5a28418049c) (cherry picked from commit e981cca9fff3608af22bdf8fc1acef5470e25663) (cherry picked from commit c66c43ad571ed2590dcd55a762c73c90d9744bac) --- .../libeffects/lvm/wrapper/Bundle/EffectBundle.cpp | 32 ++++++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp index f0afd39..5e975b0 100644 --- a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp +++ b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp @@ -2357,8 +2357,12 @@ int Equalizer_getParameter(EffectContext *pContext, case EQ_PARAM_BAND_LEVEL: param2 = *pParamTemp; - if (param2 >= FIVEBAND_NUMBANDS) { + if (param2 < 0 || param2 >= FIVEBAND_NUMBANDS) { status = -EINVAL; + if (param2 < 0) { + android_errorWriteLog(0x534e4554, "32438598"); + ALOGW("\tERROR Equalizer_getParameter() EQ_PARAM_BAND_LEVEL band %d", param2); + } break; } *(int16_t *)pValue = (int16_t)EqualizerGetBandLevel(pContext, param2); @@ -2368,8 +2372,12 @@ int Equalizer_getParameter(EffectContext *pContext, case EQ_PARAM_CENTER_FREQ: param2 = *pParamTemp; - if (param2 >= FIVEBAND_NUMBANDS) { + if (param2 < 0 || param2 >= FIVEBAND_NUMBANDS) { status = -EINVAL; + if (param2 < 0) { + android_errorWriteLog(0x534e4554, "32436341"); + ALOGW("\tERROR Equalizer_getParameter() EQ_PARAM_CENTER_FREQ band %d", param2); + } break; } *(int32_t *)pValue = EqualizerGetCentreFrequency(pContext, param2); @@ -2379,8 +2387,12 @@ int Equalizer_getParameter(EffectContext *pContext, case EQ_PARAM_BAND_FREQ_RANGE: param2 = *pParamTemp; - if (param2 >= FIVEBAND_NUMBANDS) { + if (param2 < 0 || param2 >= FIVEBAND_NUMBANDS) { status = -EINVAL; + if (param2 < 0) { + android_errorWriteLog(0x534e4554, "32247948"); + ALOGW("\tERROR Equalizer_getParameter() EQ_PARAM_BAND_FREQ_RANGE band %d", param2); + } break; } EqualizerGetBandFreqRange(pContext, param2, (uint32_t *)pValue, ((uint32_t *)pValue + 1)); @@ -2407,9 +2419,13 @@ int Equalizer_getParameter(EffectContext *pContext, case EQ_PARAM_GET_PRESET_NAME: param2 = *pParamTemp; - if (param2 >= EqualizerGetNumPresets()) { - //if (param2 >= 20) { // AGO FIX + if ((param2 < 0 && param2 != PRESET_CUSTOM) || param2 >= EqualizerGetNumPresets()) { status = -EINVAL; + if (param2 < 0) { + android_errorWriteLog(0x534e4554, "32448258"); + ALOGE("\tERROR Equalizer_getParameter() EQ_PARAM_GET_PRESET_NAME preset %d", + param2); + } break; } name = (char *)pValue; @@ -2479,8 +2495,12 @@ int Equalizer_setParameter (EffectContext *pContext, void *pParam, void *pValue) band = *pParamTemp; level = (int32_t)(*(int16_t *)pValue); //ALOGV("\tEqualizer_setParameter() EQ_PARAM_BAND_LEVEL band %d, level %d", band, level); - if (band >= FIVEBAND_NUMBANDS) { + if (band < 0 || band >= FIVEBAND_NUMBANDS) { status = -EINVAL; + if (band < 0) { + android_errorWriteLog(0x534e4554, "32095626"); + ALOGE("\tERROR Equalizer_setParameter() EQ_PARAM_BAND_LEVEL band %d", band); + } break; } EqualizerSetBandLevel(pContext, band, level); -- cgit v1.1 From 8dea6a22058328109dc1fcb7450ca5553f35b4df Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Wed, 2 Nov 2016 14:17:57 -0700 Subject: DO NOT MERGE: defensive parsing of mp3 album art information several points in stagefrights mp3 album art code used strlen() to parse user-supplied strings that may be unterminated, resulting in reading beyond the end of a buffer. This changes the code to use strnlen() for 8-bit encodings and strengthens the parsing of 16-bit encodings similarly. It also reworks how we watch for the end-of-buffer to avoid all over-reads. Bug: 32377688 Test: crafted mp3's w/ good/bad cover art. See what showed in play music Change-Id: Ia9f526d71b21ef6a61acacf616b573753cd21df6 (cherry picked from commit fa0806b594e98f1aed3ebcfc6a801b4c0056f9eb) (cherry picked from commit 7a3246b870ddd11861eda2ab458b11d723c7f62c) --- media/libstagefright/id3/ID3.cpp | 56 ++++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/media/libstagefright/id3/ID3.cpp b/media/libstagefright/id3/ID3.cpp index d1fd0d9..8944d83 100644 --- a/media/libstagefright/id3/ID3.cpp +++ b/media/libstagefright/id3/ID3.cpp @@ -837,20 +837,21 @@ void ID3::Iterator::findFrame() { } } -static size_t StringSize(const uint8_t *start, uint8_t encoding) { +// return includes terminator; if unterminated, returns > limit +static size_t StringSize(const uint8_t *start, size_t limit, uint8_t encoding) { + if (encoding == 0x00 || encoding == 0x03) { // ISO 8859-1 or UTF-8 - return strlen((const char *)start) + 1; + return strnlen((const char *)start, limit) + 1; } // UCS-2 size_t n = 0; - while (start[n] != '\0' || start[n + 1] != '\0') { + while ((n+1 < limit) && (start[n] != '\0' || start[n + 1] != '\0')) { n += 2; } - - // Add size of null termination. - return n + 2; + n += 2; + return n; } const void * @@ -871,11 +872,19 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { if (mVersion == ID3_V2_3 || mVersion == ID3_V2_4) { uint8_t encoding = data[0]; - mime->setTo((const char *)&data[1]); - size_t mimeLen = strlen((const char *)&data[1]) + 1; + size_t consumed = 1; + + // *always* in an 8-bit encoding + size_t mimeLen = StringSize(&data[consumed], size - consumed, 0x00); + if (mimeLen > size - consumed) { + ALOGW("bogus album art size: mime"); + return NULL; + } + mime->setTo((const char *)&data[consumed]); + consumed += mimeLen; #if 0 - uint8_t picType = data[1 + mimeLen]; + uint8_t picType = data[consumed]; if (picType != 0x03) { // Front Cover Art it.next(); @@ -883,20 +892,30 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { } #endif - size_t descLen = StringSize(&data[2 + mimeLen], encoding); + consumed++; + if (consumed >= size) { + ALOGW("bogus album art size: pic type"); + return NULL; + } + + size_t descLen = StringSize(&data[consumed], size - consumed, encoding); + consumed += descLen; - if (size < 2 || - size - 2 < mimeLen || - size - 2 - mimeLen < descLen) { - ALOGW("bogus album art sizes"); + if (consumed >= size) { + ALOGW("bogus album art size: description"); return NULL; } - *length = size - 2 - mimeLen - descLen; - return &data[2 + mimeLen + descLen]; + *length = size - consumed; + + return &data[consumed]; } else { uint8_t encoding = data[0]; + if (size <= 5) { + return NULL; + } + if (!memcmp(&data[1], "PNG", 3)) { mime->setTo("image/png"); } else if (!memcmp(&data[1], "JPG", 3)) { @@ -916,7 +935,10 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { } #endif - size_t descLen = StringSize(&data[5], encoding); + size_t descLen = StringSize(&data[5], size - 5, encoding); + if (descLen > size - 5) { + return NULL; + } *length = size - 5 - descLen; -- cgit v1.1 From f5abeb809738e8b1f094d0601e882eab786d18de Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Fri, 11 Nov 2016 09:20:00 -0800 Subject: Make VBRISeeker more robust Bug: 32577290 Change-Id: I9bcc9422ae7dd3ae4a38df330c9dcd7ac4941ec8 (cherry picked from commit 7fdd36418e945cf6a500018632dfb0ed8cb1a343) (cherry picked from commit 453b351ac5bd2b6619925dc966da60adf6b3126c) --- media/libstagefright/VBRISeeker.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/media/libstagefright/VBRISeeker.cpp b/media/libstagefright/VBRISeeker.cpp index 8a0fcac..5067ddc 100644 --- a/media/libstagefright/VBRISeeker.cpp +++ b/media/libstagefright/VBRISeeker.cpp @@ -83,8 +83,23 @@ sp VBRISeeker::CreateFromSource( scale, entrySize); + if (entrySize > 4) { + ALOGE("invalid VBRI entry size: %zu", entrySize); + return NULL; + } + + sp seeker = new (std::nothrow) VBRISeeker; + if (seeker == NULL) { + ALOGW("Couldn't allocate VBRISeeker"); + return NULL; + } + size_t totalEntrySize = numEntries * entrySize; - uint8_t *buffer = new uint8_t[totalEntrySize]; + uint8_t *buffer = new (std::nothrow) uint8_t[totalEntrySize]; + if (!buffer) { + ALOGW("Couldn't allocate %zu bytes", totalEntrySize); + return NULL; + } n = source->readAt(pos + sizeof(vbriHeader), buffer, totalEntrySize); if (n < (ssize_t)totalEntrySize) { @@ -94,7 +109,6 @@ sp VBRISeeker::CreateFromSource( return NULL; } - sp seeker = new VBRISeeker; seeker->mBasePos = post_id3_pos + frameSize; // only update mDurationUs if the calculated duration is valid (non zero) // otherwise, leave duration at -1 so that getDuration() and getOffsetForTime() -- cgit v1.1 From b4a6b14b4bbd839fff29d176a0e318d2720a79ec Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Tue, 18 Oct 2016 17:13:09 -0700 Subject: Visualizer: Check capture size and latency parameters Bug: 31781965 Change-Id: I1c439a0d0f6aa0057b3c651499f28426e1e1f5e4 (cherry picked from commit 9a2732ba0a8d609ab040d2c1ddee28577ead9772) (cherry picked from commit 557bd7bfe6c4895faee09e46fc9b5304a956c8b7) --- media/libeffects/visualizer/EffectVisualizer.cpp | 43 ++++++++++++++++++------ 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/media/libeffects/visualizer/EffectVisualizer.cpp b/media/libeffects/visualizer/EffectVisualizer.cpp index 21fddb1..b7d27d6 100644 --- a/media/libeffects/visualizer/EffectVisualizer.cpp +++ b/media/libeffects/visualizer/EffectVisualizer.cpp @@ -59,6 +59,8 @@ enum visualizer_state_e { #define DISCARD_MEASUREMENTS_TIME_MS 2000 // discard measurements older than this number of ms +#define MAX_LATENCY_MS 3000 // 3 seconds of latency for audio pipeline + // maximum number of buffers for which we keep track of the measurements #define MEASUREMENT_WINDOW_MAX_SIZE_IN_BUFFERS 25 // note: buffer index is stored in uint8_t @@ -521,18 +523,29 @@ int Visualizer_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize, break; } switch (*(uint32_t *)p->data) { - case VISUALIZER_PARAM_CAPTURE_SIZE: - pContext->mCaptureSize = *((uint32_t *)p->data + 1); - ALOGV("set mCaptureSize = %" PRIu32, pContext->mCaptureSize); - break; + case VISUALIZER_PARAM_CAPTURE_SIZE: { + const uint32_t captureSize = *((uint32_t *)p->data + 1); + if (captureSize > VISUALIZER_CAPTURE_SIZE_MAX) { + android_errorWriteLog(0x534e4554, "31781965"); + *(int32_t *)pReplyData = -EINVAL; + ALOGW("set mCaptureSize = %u > %u", captureSize, VISUALIZER_CAPTURE_SIZE_MAX); + } else { + pContext->mCaptureSize = captureSize; + ALOGV("set mCaptureSize = %u", captureSize); + } + } break; case VISUALIZER_PARAM_SCALING_MODE: pContext->mScalingMode = *((uint32_t *)p->data + 1); ALOGV("set mScalingMode = %" PRIu32, pContext->mScalingMode); break; - case VISUALIZER_PARAM_LATENCY: - pContext->mLatency = *((uint32_t *)p->data + 1); - ALOGV("set mLatency = %" PRIu32, pContext->mLatency); - break; + case VISUALIZER_PARAM_LATENCY: { + uint32_t latency = *((uint32_t *)p->data + 1); + if (latency > MAX_LATENCY_MS) { + latency = MAX_LATENCY_MS; // clamp latency b/31781965 + } + pContext->mLatency = latency; + ALOGV("set mLatency = %u", latency); + } break; case VISUALIZER_PARAM_MEASUREMENT_MODE: pContext->mMeasurementMode = *((uint32_t *)p->data + 1); ALOGV("set mMeasurementMode = %" PRIu32, pContext->mMeasurementMode); @@ -571,10 +584,18 @@ int Visualizer_command(effect_handle_t self, uint32_t cmdCode, uint32_t cmdSize, if (latencyMs < 0) { latencyMs = 0; } - const uint32_t deltaSmpl = - pContext->mConfig.inputCfg.samplingRate * latencyMs / 1000; - int32_t capturePoint = pContext->mCaptureIdx - captureSize - deltaSmpl; + uint32_t deltaSmpl = captureSize + + pContext->mConfig.inputCfg.samplingRate * latencyMs / 1000; + + // large sample rate, latency, or capture size, could cause overflow. + // do not offset more than the size of buffer. + if (deltaSmpl > CAPTURE_BUF_SIZE) { + android_errorWriteLog(0x534e4554, "31781965"); + deltaSmpl = CAPTURE_BUF_SIZE; + } + int32_t capturePoint = pContext->mCaptureIdx - deltaSmpl; + // a negative capturePoint means we wrap the buffer. if (capturePoint < 0) { uint32_t size = -capturePoint; if (size > captureSize) { -- cgit v1.1 From 178e1e1e6a4fd7c3cc284858c6f56ddf7e2697c3 Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Fri, 4 Nov 2016 19:40:53 -0700 Subject: Effects: Check get parameter command size Test: Custom test. Bug: 32438594 Bug: 32624850 Bug: 32635664 Change-Id: I9b1315e2c02f11bea395bfdcf5c1ccddccbad8a6 (cherry picked from commit 3d34cc76e315dfa8c3b1edf78835b0dab4980505) (cherry picked from commit 26965db50a617f69bdefca0d7533796c80374f2c) --- services/audioflinger/Effects.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index 5505d2e..d46c10e 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -571,6 +571,13 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode, android_errorWriteLog(0x534e4554, "29251553"); return -EINVAL; } + if (cmdCode == EFFECT_CMD_GET_PARAM && + (sizeof(effect_param_t) > cmdSize || + ((effect_param_t *)pCmdData)->psize > cmdSize + - sizeof(effect_param_t))) { + android_errorWriteLog(0x534e4554, "32438594"); + 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 From ffa026e98f239bbb17b7978cf8f10b7977ab563e Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Wed, 26 Oct 2016 17:41:56 -0700 Subject: stagefright: remove allottedSize equality check in IOMX::useBuffer This was meant for buffers shared cross-process, but we are not gaining anything from this check even if it was at the correct place. Bug: 32436178 Change-Id: I6919e8ac6e35092273e171f49f6711ba577ba2e6 (cherry picked from commit 58388aa7be1c6963eb4b8464d46938ba9b0a04b0) --- media/libstagefright/omx/OMXNodeInstance.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index c09064f..afb7382 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -784,13 +784,6 @@ status_t OMXNodeInstance::useBuffer( } memset(data, 0, allottedSize); - // if we are not connecting the buffers, the sizes must match - if (allottedSize != params->size()) { - CLOG_ERROR(useBuffer, BAD_VALUE, SIMPLE_BUFFER(portIndex, (size_t)allottedSize, data)); - delete[] data; - return BAD_VALUE; - } - buffer_meta = new BufferMeta( params, portIndex, false /* copyToOmx */, false /* copyFromOmx */, data); } else { -- cgit v1.1 From ad990eb12c7aff3c4bcdd50cae90b2b7c20041e6 Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Wed, 2 Nov 2016 14:59:03 -0700 Subject: IOMX: convert ANWB to Gralloc meta if using useBuffer in the same process This was disabled by a previous commit. Bug: 32436178 Change-Id: I9f9c6a372a039226d61f3651be3af207fed63e60 (cherry picked from commit 4fb1e42a16e77d7abf1d84bedbc20f901af26524) --- media/libstagefright/omx/OMXNodeInstance.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index afb7382..0c30e44 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -170,8 +170,10 @@ struct BufferMeta { return buf; } - bool copyToOmx() const { - return mCopyToOmx; + bool copyingOrSharingToOmx(const OMX_BUFFERHEADERTYPE *header) const { + return mCopyToOmx + // sharing buffer with client + || (mMem != NULL && mMem->pointer() == header->pBuffer); } void setGraphicBuffer(const sp &graphicBuffer) { @@ -1276,7 +1278,7 @@ status_t OMXNodeInstance::emptyBuffer( // convert incoming ANW meta buffers if component is configured for gralloc metadata mode // ignore rangeOffset in this case - if (buffer_meta->copyToOmx() + if (buffer_meta->copyingOrSharingToOmx(header) && mMetadataType[kPortIndexInput] == kMetadataBufferTypeGrallocSource && backup->capacity() >= sizeof(VideoNativeMetadata) && codec->capacity() >= sizeof(VideoGrallocMetadata) -- cgit v1.1