From 7900d8611ea22ce04c1697a8f391b83ed48c904d Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Tue, 15 Nov 2016 17:19:58 -0800 Subject: Effect: Use local cached data for Effect commit Test: POC, Cts Effect, BassBoost, EnvReverb, Equalizer, Test: LoudnessEnhancer, PresetReverb, Virtualizer, Visualizer Bug: 32220769 Change-Id: Iea96ba0daf71691ee8954cca4ba1c10fe827626e (cherry picked from commit dd79ccda92c1e9b982b2d0f8877d98e5258fbb73) (cherry picked from commit a155de4d70e0b9ac8fc02b2bdcbb2e8e6cca46ff) --- services/audioflinger/Effects.cpp | 57 ++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index d46c10e..27dfa05 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -1264,36 +1264,54 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, // particular client process: no risk to block the whole media server process or mixer // threads if we are stuck here Mutex::Autolock _l(mCblk->lock); - if (mCblk->clientIndex > EFFECT_PARAM_BUFFER_SIZE || - mCblk->serverIndex > EFFECT_PARAM_BUFFER_SIZE) { + + // keep local copy of index in case of client corruption b/32220769 + const uint32_t clientIndex = mCblk->clientIndex; + const uint32_t serverIndex = mCblk->serverIndex; + if (clientIndex > EFFECT_PARAM_BUFFER_SIZE || + serverIndex > EFFECT_PARAM_BUFFER_SIZE) { mCblk->serverIndex = 0; mCblk->clientIndex = 0; return BAD_VALUE; } status_t status = NO_ERROR; - while (mCblk->serverIndex < mCblk->clientIndex) { - int reply; - uint32_t rsize = sizeof(int); - int *p = (int *)(mBuffer + mCblk->serverIndex); - int size = *p++; - if (((uint8_t *)p + size) > mBuffer + mCblk->clientIndex) { + effect_param_t *param = NULL; + for (uint32_t index = serverIndex; index < clientIndex;) { + int *p = (int *)(mBuffer + index); + const int size = *p++; + if (size < 0 + || size > EFFECT_PARAM_BUFFER_SIZE + || ((uint8_t *)p + size) > mBuffer + clientIndex) { ALOGW("command(): invalid parameter block size"); + status = BAD_VALUE; break; } - effect_param_t *param = (effect_param_t *)p; - if (param->psize == 0 || param->vsize == 0) { - ALOGW("command(): null parameter or value size"); - mCblk->serverIndex += size; - continue; + + // copy to local memory in case of client corruption b/32220769 + param = (effect_param_t *)realloc(param, size); + if (param == NULL) { + ALOGW("command(): out of memory"); + status = NO_MEMORY; + break; } - uint32_t psize = sizeof(effect_param_t) + - ((param->psize - 1) / sizeof(int) + 1) * sizeof(int) + - param->vsize; + memcpy(param, p, size); + + int reply = 0; + uint32_t rsize = sizeof(reply); status_t ret = mEffect->command(EFFECT_CMD_SET_PARAM, - psize, - p, + size, + param, &rsize, &reply); + + // verify shared memory: server index shouldn't change; client index can't go back. + if (serverIndex != mCblk->serverIndex + || clientIndex > mCblk->clientIndex) { + android_errorWriteLog(0x534e4554, "32220769"); + status = BAD_VALUE; + break; + } + // stop at first error encountered if (ret != NO_ERROR) { status = ret; @@ -1303,8 +1321,9 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, *(int *)pReplyData = reply; break; } - mCblk->serverIndex += size; + index += size; } + free(param); mCblk->serverIndex = 0; mCblk->clientIndex = 0; return status; -- cgit v1.1 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 From dac1444e4926f94d8d9ac6b6a098ac101ce4a7be Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Thu, 1 Dec 2016 15:28:29 -0800 Subject: DO NOT MERGE - improve audio effect framwework thread safety - Reorganize handle effect creation code to make sure the effect engine is created with both thread and effect chain mutex held. - Reorganize handle disconnect code to make sure the effect engine is released with both thread and effect chain mutex held. - Protect IEffect interface methods in EffectHande with a Mutex. - Only pin effect if the session was acquired first. - Do not use strong pointer to EffectModule in EffectHandles: only the EffectChain has a single strong reference to the EffectModule. Bug: 32707507 CVE-2017-0479 CVE-2017-0480 CVE-2017-0499 Change-Id: Ia1098cba2cd32cc2d1c9dfdff4adc2388dfed80e (cherry picked from commit b378b73dd7480b584340b8028802c9ca2d625123) (cherry picked from commit 22e26d8ee73488c58ba3e7928e5da155151abfd0 with backport by ) --- services/audioflinger/AudioFlinger.cpp | 23 ++- services/audioflinger/AudioFlinger.h | 1 + services/audioflinger/Effects.cpp | 224 +++++++++++++-------- services/audioflinger/Effects.h | 37 +++- services/audioflinger/Threads.cpp | 51 +++-- services/audioflinger/Threads.h | 7 +- .../service/AudioPolicyInterfaceImpl.cpp | 5 +- 7 files changed, 225 insertions(+), 123 deletions(-) diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index 23215dd..9d435e9 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -1346,7 +1346,7 @@ void AudioFlinger::removeNotificationClient(pid_t pid) ALOGV("%d died, releasing its sessions", pid); size_t num = mAudioSessionRefs.size(); bool removed = false; - for (size_t i = 0; i< num; ) { + for (size_t i = 0; i < num; ) { AudioSessionRef *ref = mAudioSessionRefs.itemAt(i); ALOGV(" pid %d @ %d", ref->mPid, i); if (ref->mPid == pid) { @@ -2343,7 +2343,7 @@ void AudioFlinger::acquireAudioSessionId(int audioSession, pid_t pid) } size_t num = mAudioSessionRefs.size(); - for (size_t i = 0; i< num; i++) { + for (size_t i = 0; i < num; i++) { AudioSessionRef *ref = mAudioSessionRefs.editItemAt(i); if (ref->mSessionid == audioSession && ref->mPid == caller) { ref->mCnt++; @@ -2364,7 +2364,7 @@ void AudioFlinger::releaseAudioSessionId(int audioSession, pid_t pid) caller = pid; } size_t num = mAudioSessionRefs.size(); - for (size_t i = 0; i< num; i++) { + for (size_t i = 0; i < num; i++) { AudioSessionRef *ref = mAudioSessionRefs.itemAt(i); if (ref->mSessionid == audioSession && ref->mPid == caller) { ref->mCnt--; @@ -2382,6 +2382,18 @@ void AudioFlinger::releaseAudioSessionId(int audioSession, pid_t pid) ALOGW_IF(caller != getpid_cached, "session id %d not found for pid %d", audioSession, caller); } +bool AudioFlinger::isSessionAcquired_l(audio_session_t audioSession) +{ + size_t num = mAudioSessionRefs.size(); + for (size_t i = 0; i < num; i++) { + AudioSessionRef *ref = mAudioSessionRefs.itemAt(i); + if (ref->mSessionid == audioSession) { + return true; + } + } + return false; +} + void AudioFlinger::purgeStaleEffects_l() { ALOGV("purging stale effects"); @@ -2723,8 +2735,9 @@ sp AudioFlinger::createEffect( sp client = registerPid(pid); // create effect on selected output thread + bool pinned = (sessionId > AUDIO_SESSION_OUTPUT_MIX) && isSessionAcquired_l((audio_session_t)sessionId); handle = thread->createEffect_l(client, effectClient, priority, sessionId, - &desc, enabled, &lStatus); + &desc, enabled, &lStatus, pinned); if (handle != 0 && id != NULL) { *id = handle->id(); } @@ -2924,7 +2937,7 @@ bool AudioFlinger::updateOrphanEffectChains(const sp ALOGV("updateOrphanEffectChains session %d index %d", session, index); if (index >= 0) { sp chain = mOrphanEffectChains.valueAt(index); - if (chain->removeEffect_l(effect) == 0) { + if (chain->removeEffect_l(effect, true) == 0) { ALOGV("updateOrphanEffectChains removing effect chain at index %d", index); mOrphanEffectChains.removeItemsAt(index); } diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h index 08fa70d..3e9a088 100644 --- a/services/audioflinger/AudioFlinger.h +++ b/services/audioflinger/AudioFlinger.h @@ -593,6 +593,7 @@ private: void removeNotificationClient(pid_t pid); bool isNonOffloadableGlobalEffectEnabled_l(); void onNonOffloadableGlobalEffectEnable(); + bool isSessionAcquired_l(audio_session_t audioSession); // Store an effect chain to mOrphanEffectChains keyed vector. // Called when a thread exits and effects are still attached to it. diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index b9fe741..fb9e157 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -59,8 +59,9 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread, const wp& chain, effect_descriptor_t *desc, int id, - int sessionId) - : mPinned(sessionId > AUDIO_SESSION_OUTPUT_MIX), + audio_session_t sessionId, + bool pinned) + : mPinned(pinned), mThread(thread), mChain(chain), mId(id), mSessionId(sessionId), mDescriptor(*desc), // mConfig is set by configure() and not used before then @@ -71,7 +72,7 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread, mSuspended(false), mAudioFlinger(thread->mAudioFlinger) { - ALOGV("Constructor %p", this); + ALOGV("Constructor %p pinned %d", this, pinned); int lStatus; // create effect engine from effect factory @@ -86,6 +87,8 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread, goto Error; } + setOffloaded(thread->type() == ThreadBase::OFFLOAD, thread->id()); + ALOGV("Constructor success name %s, Interface %p", mDescriptor.name, mEffectInterface); return; Error: @@ -98,9 +101,8 @@ AudioFlinger::EffectModule::~EffectModule() { ALOGV("Destructor %p", this); if (mEffectInterface != NULL) { - remove_effect_from_hal_l(); - // release effect engine - EffectRelease(mEffectInterface); + ALOGW("EffectModule %p destructor called with unreleased interface", this); + release_l(); } } @@ -115,7 +117,7 @@ status_t AudioFlinger::EffectModule::addHandle(EffectHandle *handle) size_t i; for (i = 0; i < size; i++) { EffectHandle *h = mHandles[i]; - if (h == NULL || h->destroyed_l()) { + if (h == NULL || h->disconnected()) { continue; } // first non destroyed handle is considered in control @@ -143,9 +145,14 @@ status_t AudioFlinger::EffectModule::addHandle(EffectHandle *handle) return status; } -size_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle) +ssize_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle) { Mutex::Autolock _l(mLock); + return removeHandle_l(handle); +} + +ssize_t AudioFlinger::EffectModule::removeHandle_l(EffectHandle *handle) +{ size_t size = mHandles.size(); size_t i; for (i = 0; i < size; i++) { @@ -154,9 +161,10 @@ size_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle) } } if (i == size) { - return size; + ALOGW("%s %p handle not found %p", __FUNCTION__, this, handle); + return BAD_VALUE; } - ALOGV("removeHandle() %p removed handle %p in position %d", this, handle, i); + ALOGV("removeHandle_l() %p removed handle %p in position %zu", this, handle, i); mHandles.removeAt(i); // if removed from first place, move effect control from this handle to next in line @@ -183,7 +191,7 @@ AudioFlinger::EffectHandle *AudioFlinger::EffectModule::controlHandle_l() // the first valid handle in the list has control over the module for (size_t i = 0; i < mHandles.size(); i++) { EffectHandle *h = mHandles[i]; - if (h != NULL && !h->destroyed_l()) { + if (h != NULL && !h->disconnected()) { return h; } } @@ -191,29 +199,22 @@ AudioFlinger::EffectHandle *AudioFlinger::EffectModule::controlHandle_l() return NULL; } -size_t AudioFlinger::EffectModule::disconnect(EffectHandle *handle, bool unpinIfLast) +// unsafe method called when the effect parent thread has been destroyed +ssize_t AudioFlinger::EffectModule::disconnectHandle(EffectHandle *handle, bool unpinIfLast) { ALOGV("disconnect() %p handle %p", this, handle); - // keep a strong reference on this EffectModule to avoid calling the - // destructor before we exit - sp keep(this); - { - if (removeHandle(handle) == 0) { - if (!isPinned() || unpinIfLast) { - sp thread = mThread.promote(); - if (thread != 0) { - Mutex::Autolock _l(thread->mLock); - thread->removeEffect_l(this); - } - sp af = mAudioFlinger.promote(); - if (af != 0) { - af->updateOrphanEffectChains(this); - } - AudioSystem::unregisterEffect(mId); - } + Mutex::Autolock _l(mLock); + ssize_t numHandles = removeHandle_l(handle); + if ((numHandles == 0) && (!mPinned || unpinIfLast)) { + AudioSystem::unregisterEffect(mId); + sp af = mAudioFlinger.promote(); + if (af != 0) { + mLock.unlock(); + af->updateOrphanEffectChains(this); + mLock.lock(); } } - return mHandles.size(); + return numHandles; } void AudioFlinger::EffectModule::updateState() { @@ -528,6 +529,17 @@ status_t AudioFlinger::EffectModule::stop_l() return status; } +// must be called with EffectChain::mLock held +void AudioFlinger::EffectModule::release_l() +{ + if (mEffectInterface != NULL) { + remove_effect_from_hal_l(); + // release effect engine + EffectRelease(mEffectInterface); + mEffectInterface = NULL; + } +} + status_t AudioFlinger::EffectModule::remove_effect_from_hal_l() { if ((mDescriptor.flags & EFFECT_FLAG_TYPE_MASK) == EFFECT_FLAG_TYPE_PRE_PROC || @@ -620,7 +632,7 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode, uint32_t size = (replySize == NULL) ? 0 : *replySize; for (size_t i = 1; i < mHandles.size(); i++) { EffectHandle *h = mHandles[i]; - if (h != NULL && !h->destroyed_l()) { + if (h != NULL && !h->disconnected()) { h->commandExecuted(cmdCode, cmdSize, pCmdData, size, pReplyData); } } @@ -673,7 +685,7 @@ status_t AudioFlinger::EffectModule::setEnabled_l(bool enabled) } for (size_t i = 1; i < mHandles.size(); i++) { EffectHandle *h = mHandles[i]; - if (h != NULL && !h->destroyed_l()) { + if (h != NULL && !h->disconnected()) { h->setEnabled(enabled); } } @@ -838,8 +850,7 @@ bool AudioFlinger::EffectModule::purgeHandles() Mutex::Autolock _l(mLock); for (size_t i = 0; i < mHandles.size(); i++) { EffectHandle *handle = mHandles[i]; - if (handle != NULL && !handle->destroyed_l()) { - handle->effect().clear(); + if (handle != NULL && !handle->disconnected()) { if (handle->hasControl()) { enabled = handle->enabled(); } @@ -1066,7 +1077,7 @@ void AudioFlinger::EffectModule::dump(int fd, const Vector& args __unu result.append("\t\t\t Pid Priority Ctrl Locked client server\n"); for (size_t i = 0; i < mHandles.size(); ++i) { EffectHandle *handle = mHandles[i]; - if (handle != NULL && !handle->destroyed_l()) { + if (handle != NULL && !handle->disconnected()) { handle->dumpToBuffer(buffer, SIZE); result.append(buffer); } @@ -1092,7 +1103,7 @@ AudioFlinger::EffectHandle::EffectHandle(const sp& effect, int32_t priority) : BnEffect(), mEffect(effect), mEffectClient(effectClient), mClient(client), mCblk(NULL), - mPriority(priority), mHasControl(false), mEnabled(false), mDestroyed(false) + mPriority(priority), mHasControl(false), mEnabled(false), mDisconnected(false) { ALOGV("constructor %p", this); @@ -1115,14 +1126,6 @@ AudioFlinger::EffectHandle::EffectHandle(const sp& effect, AudioFlinger::EffectHandle::~EffectHandle() { ALOGV("Destructor %p", this); - - if (mEffect == 0) { - mDestroyed = true; - return; - } - mEffect->lock(); - mDestroyed = true; - mEffect->unlock(); disconnect(false); } @@ -1133,13 +1136,15 @@ status_t AudioFlinger::EffectHandle::initCheck() status_t AudioFlinger::EffectHandle::enable() { + AutoMutex _l(mLock); ALOGV("enable %p", this); + sp effect = mEffect.promote(); + if (effect == 0 || mDisconnected) { + return DEAD_OBJECT; + } if (!mHasControl) { return INVALID_OPERATION; } - if (mEffect == 0) { - return DEAD_OBJECT; - } if (mEnabled) { return NO_ERROR; @@ -1147,20 +1152,20 @@ status_t AudioFlinger::EffectHandle::enable() mEnabled = true; - sp thread = mEffect->thread().promote(); + sp thread = effect->thread().promote(); if (thread != 0) { - thread->checkSuspendOnEffectEnabled(mEffect, true, mEffect->sessionId()); + thread->checkSuspendOnEffectEnabled(effect, true, effect->sessionId()); } // checkSuspendOnEffectEnabled() can suspend this same effect when enabled - if (mEffect->suspended()) { + if (effect->suspended()) { return NO_ERROR; } - status_t status = mEffect->setEnabled(true); + status_t status = effect->setEnabled(true); if (status != NO_ERROR) { if (thread != 0) { - thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId()); + thread->checkSuspendOnEffectEnabled(effect, false, effect->sessionId()); } mEnabled = false; } else { @@ -1171,13 +1176,13 @@ status_t AudioFlinger::EffectHandle::enable() Mutex::Autolock _l(t->mLock); t->broadcast_l(); } - if (!mEffect->isOffloadable()) { + if (!effect->isOffloadable()) { if (thread->type() == ThreadBase::OFFLOAD || (thread->type() == ThreadBase::DIRECT && thread->mIsDirectPcm)) { PlaybackThread *t = (PlaybackThread *)thread.get(); t->invalidateTracks(AUDIO_STREAM_MUSIC); } - if (mEffect->sessionId() == AUDIO_SESSION_OUTPUT_MIX) { + if (effect->sessionId() == AUDIO_SESSION_OUTPUT_MIX) { thread->mAudioFlinger->onNonOffloadableGlobalEffectEnable(); } } @@ -1189,27 +1194,29 @@ status_t AudioFlinger::EffectHandle::enable() status_t AudioFlinger::EffectHandle::disable() { ALOGV("disable %p", this); + AutoMutex _l(mLock); + sp effect = mEffect.promote(); + if (effect == 0 || mDisconnected) { + return DEAD_OBJECT; + } if (!mHasControl) { return INVALID_OPERATION; } - if (mEffect == 0) { - return DEAD_OBJECT; - } if (!mEnabled) { return NO_ERROR; } mEnabled = false; - if (mEffect->suspended()) { + if (effect->suspended()) { return NO_ERROR; } - status_t status = mEffect->setEnabled(false); + status_t status = effect->setEnabled(false); - sp thread = mEffect->thread().promote(); + sp thread = effect->thread().promote(); if (thread != 0) { - thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId()); + thread->checkSuspendOnEffectEnabled(effect, false, effect->sessionId()); if ((thread->type() == ThreadBase::OFFLOAD) || (thread->type() == ThreadBase::DIRECT && thread->mIsDirectPcm)){ PlaybackThread *t = (PlaybackThread *)thread.get(); @@ -1223,25 +1230,39 @@ status_t AudioFlinger::EffectHandle::disable() void AudioFlinger::EffectHandle::disconnect() { + ALOGV("%s %p", __FUNCTION__, this); disconnect(true); } void AudioFlinger::EffectHandle::disconnect(bool unpinIfLast) { - ALOGV("disconnect(%s)", unpinIfLast ? "true" : "false"); - if (mEffect == 0) { + AutoMutex _l(mLock); + ALOGV("disconnect(%s) %p", unpinIfLast ? "true" : "false", this); + if (mDisconnected) { + if (unpinIfLast) { + android_errorWriteLog(0x534e4554, "32707507"); + } return; } - // restore suspended effects if the disconnected handle was enabled and the last one. - if ((mEffect->disconnect(this, unpinIfLast) == 0) && mEnabled) { - sp thread = mEffect->thread().promote(); - if (thread != 0) { - thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId()); + mDisconnected = true; + sp thread; + { + sp effect = mEffect.promote(); + if (effect != 0) { + thread = effect->thread().promote(); + } + } + if (thread != 0) { + thread->disconnectEffectHandle(this, unpinIfLast); + } else { + ALOGW("%s Effect handle %p disconnected after thread destruction", __FUNCTION__, this); + // try to cleanup as much as we can + sp effect = mEffect.promote(); + if (effect != 0) { + effect->disconnectHandle(this, unpinIfLast); } } - // release sp on module => module destructor can be called now - mEffect.clear(); if (mClient != 0) { if (mCblk != NULL) { // unlike ~TrackBase(), mCblk is never a local new, so don't delete @@ -1261,15 +1282,17 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, void *pReplyData) { ALOGVV("command(), cmdCode: %d, mHasControl: %d, mEffect: %p", - cmdCode, mHasControl, (mEffect == 0) ? 0 : mEffect.get()); + cmdCode, mHasControl, mEffect.unsafe_get()); + AutoMutex _l(mLock); + sp effect = mEffect.promote(); + if (effect == 0 || mDisconnected) { + return DEAD_OBJECT; + } // only get parameter command is permitted for applications not controlling the effect if (!mHasControl && cmdCode != EFFECT_CMD_GET_PARAM) { return INVALID_OPERATION; } - if (mEffect == 0) { - return DEAD_OBJECT; - } if (mClient == 0) { return INVALID_OPERATION; } @@ -1314,7 +1337,7 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, int reply = 0; uint32_t rsize = sizeof(reply); - status_t ret = mEffect->command(EFFECT_CMD_SET_PARAM, + status_t ret = effect->command(EFFECT_CMD_SET_PARAM, size, param, &rsize, @@ -1351,7 +1374,7 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, return disable(); } - return mEffect->command(cmdCode, cmdSize, pCmdData, replySize, pReplyData); + return effect->command(cmdCode, cmdSize, pCmdData, replySize, pReplyData); } void AudioFlinger::EffectHandle::setControl(bool hasControl, bool signal, bool enabled) @@ -1433,7 +1456,6 @@ AudioFlinger::EffectChain::~EffectChain() if (mOwnInBuffer) { delete mInBuffer; } - } // getEffectFromDesc_l() must be called with ThreadBase::mLock held @@ -1547,13 +1569,38 @@ void AudioFlinger::EffectChain::process_l() } } -// addEffect_l() must be called with PlaybackThread::mLock held +// createEffect_l() must be called with ThreadBase::mLock held +status_t AudioFlinger::EffectChain::createEffect_l(sp& effect, + ThreadBase *thread, + effect_descriptor_t *desc, + int id, + audio_session_t sessionId, + bool pinned) +{ + Mutex::Autolock _l(mLock); + effect = new EffectModule(thread, this, desc, id, sessionId, pinned); + status_t lStatus = effect->status(); + if (lStatus == NO_ERROR) { + lStatus = addEffect_ll(effect); + } + if (lStatus != NO_ERROR) { + effect.clear(); + } + return lStatus; +} + +// addEffect_l() must be called with ThreadBase::mLock held status_t AudioFlinger::EffectChain::addEffect_l(const sp& effect) { + Mutex::Autolock _l(mLock); + return addEffect_ll(effect); +} +// addEffect_l() must be called with ThreadBase::mLock and EffectChain::mLock held +status_t AudioFlinger::EffectChain::addEffect_ll(const sp& effect) +{ effect_descriptor_t desc = effect->desc(); uint32_t insertPref = desc.flags & EFFECT_FLAG_INSERT_MASK; - Mutex::Autolock _l(mLock); effect->setChain(this); sp thread = mThread.promote(); if (thread == 0) { @@ -1663,8 +1710,9 @@ status_t AudioFlinger::EffectChain::addEffect_l(const sp& effect) return NO_ERROR; } -// removeEffect_l() must be called with PlaybackThread::mLock held -size_t AudioFlinger::EffectChain::removeEffect_l(const sp& effect) +// removeEffect_l() must be called with ThreadBase::mLock held +size_t AudioFlinger::EffectChain::removeEffect_l(const sp& effect, + bool release) { Mutex::Autolock _l(mLock); size_t size = mEffects.size(); @@ -1679,6 +1727,10 @@ size_t AudioFlinger::EffectChain::removeEffect_l(const sp& effect) mEffects[i]->state() == EffectModule::STOPPING) { mEffects[i]->stop(); } + if (release) { + mEffects[i]->release_l(); + } + if (type == EFFECT_FLAG_TYPE_AUXILIARY) { delete[] effect->inBuffer(); } else { @@ -1697,7 +1749,7 @@ size_t AudioFlinger::EffectChain::removeEffect_l(const sp& effect) return mEffects.size(); } -// setDevice_l() must be called with PlaybackThread::mLock held +// setDevice_l() must be called with ThreadBase::mLock held void AudioFlinger::EffectChain::setDevice_l(audio_devices_t device) { size_t size = mEffects.size(); @@ -1706,7 +1758,7 @@ void AudioFlinger::EffectChain::setDevice_l(audio_devices_t device) } } -// setMode_l() must be called with PlaybackThread::mLock held +// setMode_l() must be called with ThreadBase::mLock held void AudioFlinger::EffectChain::setMode_l(audio_mode_t mode) { size_t size = mEffects.size(); @@ -1715,7 +1767,7 @@ void AudioFlinger::EffectChain::setMode_l(audio_mode_t mode) } } -// setAudioSource_l() must be called with PlaybackThread::mLock held +// setAudioSource_l() must be called with ThreadBase::mLock held void AudioFlinger::EffectChain::setAudioSource_l(audio_source_t source) { size_t size = mEffects.size(); @@ -1724,7 +1776,7 @@ void AudioFlinger::EffectChain::setAudioSource_l(audio_source_t source) } } -// setVolume_l() must be called with PlaybackThread::mLock held +// setVolume_l() must be called with ThreadBase::mLock held bool AudioFlinger::EffectChain::setVolume_l(uint32_t *left, uint32_t *right) { uint32_t newLeft = *left; @@ -1876,7 +1928,7 @@ void AudioFlinger::EffectChain::setEffectSuspended_l( effect->setSuspended(false); effect->lock(); EffectHandle *handle = effect->controlHandle_l(); - if (handle != NULL && !handle->destroyed_l()) { + if (handle != NULL && !handle->disconnected()) { effect->setEnabled_l(handle->enabled()); } effect->unlock(); diff --git a/services/audioflinger/Effects.h b/services/audioflinger/Effects.h index 6f93f81..4d53c0d 100644 --- a/services/audioflinger/Effects.h +++ b/services/audioflinger/Effects.h @@ -45,7 +45,8 @@ public: const wp& chain, effect_descriptor_t *desc, int id, - int sessionId); + audio_session_t sessionId, + bool pinned); virtual ~EffectModule(); enum effect_state { @@ -93,8 +94,9 @@ public: const wp& thread() { return mThread; } status_t addHandle(EffectHandle *handle); - size_t disconnect(EffectHandle *handle, bool unpinIfLast); - size_t removeHandle(EffectHandle *handle); + ssize_t disconnectHandle(EffectHandle *handle, bool unpinIfLast); + ssize_t removeHandle(EffectHandle *handle); + ssize_t removeHandle_l(EffectHandle *handle); const effect_descriptor_t& desc() const { return mDescriptor; } wp& chain() { return mChain; } @@ -120,6 +122,7 @@ public: status_t setOffloaded(bool offloaded, audio_io_handle_t io); bool isOffloaded() const; void addEffectToHal_l(); + void release_l(); void dump(int fd, const Vector& args); @@ -204,12 +207,17 @@ public: bool enabled() const { return mEnabled; } // Getters - int id() const { return mEffect->id(); } + wp effect() const { return mEffect; } + int id() const { + sp effect = mEffect.promote(); + if (effect == 0) { + return 0; + } + return effect->id(); + } int priority() const { return mPriority; } bool hasControl() const { return mHasControl; } - sp effect() const { return mEffect; } - // destroyed_l() must be called with the associated EffectModule mLock held - bool destroyed_l() const { return mDestroyed; } + bool disconnected() const { return mDisconnected; } void dumpToBuffer(char* buffer, size_t size); @@ -218,7 +226,8 @@ protected: EffectHandle(const EffectHandle&); EffectHandle& operator =(const EffectHandle&); - sp mEffect; // pointer to controlled EffectModule + Mutex mLock; // protects IEffect method calls + wp mEffect; // pointer to controlled EffectModule sp mEffectClient; // callback interface for client notifications /*const*/ sp mClient; // client for shared memory allocation, see disconnect() sp mCblkMemory; // shared memory for control block @@ -229,8 +238,7 @@ protected: bool mHasControl; // true if this handle is controlling the effect bool mEnabled; // cached enable state: needed when the effect is // restored after being suspended - bool mDestroyed; // Set to true by destructor. Access with EffectModule - // mLock held + bool mDisconnected; // Set to true by disconnect() }; // the EffectChain class represents a group of effects associated to one audio session. @@ -263,8 +271,15 @@ public: mLock.unlock(); } + status_t createEffect_l(sp& effect, + ThreadBase *thread, + effect_descriptor_t *desc, + int id, + audio_session_t sessionId, + bool pinned); status_t addEffect_l(const sp& handle); - size_t removeEffect_l(const sp& handle); + status_t addEffect_ll(const sp& handle); + size_t removeEffect_l(const sp& handle, bool release = false); int sessionId() const { return mSessionId; } void setSessionId(int sessionId) { mSessionId = sessionId; } diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index e5e8bdb..d7c9c47 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -1159,7 +1159,8 @@ sp AudioFlinger::ThreadBase::createEffect_l( int sessionId, effect_descriptor_t *desc, int *enabled, - status_t *status) + status_t *status, + bool pinned) { sp effect; sp handle; @@ -1253,20 +1254,7 @@ sp AudioFlinger::ThreadBase::createEffect_l( } effectRegistered = true; // create a new effect module if none present in the chain - effect = new EffectModule(this, chain, desc, id, sessionId); - lStatus = effect->status(); - if (lStatus != NO_ERROR) { - goto Exit; - } - - bool setVal = false; - if (mType == OFFLOAD || (mType == DIRECT && mIsDirectPcm)) { - setVal = true; - } - - effect->setOffloaded(setVal, mId); - - lStatus = chain->addEffect_l(effect); + lStatus = chain->createEffect_l(effect, this, desc, id, (audio_session_t)sessionId, pinned); if (lStatus != NO_ERROR) { goto Exit; } @@ -1307,6 +1295,33 @@ Exit: return handle; } +void AudioFlinger::ThreadBase::disconnectEffectHandle(EffectHandle *handle, + bool unpinIfLast) +{ + bool remove = false; + sp effect; + { + Mutex::Autolock _l(mLock); + + effect = handle->effect().promote(); + if (effect == 0) { + return; + } + // restore suspended effects if the disconnected handle was enabled and the last one. + remove = (effect->removeHandle(handle) == 0) && (!effect->isPinned() || unpinIfLast); + if (remove) { + removeEffect_l(effect, true); + } + } + if (remove) { + mAudioFlinger->updateOrphanEffectChains(effect); + AudioSystem::unregisterEffect(effect->id()); + if (handle->enabled()) { + checkSuspendOnEffectEnabled(effect, false, effect->sessionId()); + } + } +} + sp AudioFlinger::ThreadBase::getEffect(int sessionId, int effectId) { Mutex::Autolock _l(mLock); @@ -1371,9 +1386,9 @@ status_t AudioFlinger::ThreadBase::addEffect_l(const sp& effect) return NO_ERROR; } -void AudioFlinger::ThreadBase::removeEffect_l(const sp& effect) { +void AudioFlinger::ThreadBase::removeEffect_l(const sp& effect, bool release) { - ALOGV("removeEffect_l() %p effect %p", this, effect.get()); + ALOGV("%s %p effect %p", __FUNCTION__, this, effect.get()); effect_descriptor_t desc = effect->desc(); if ((desc.flags & EFFECT_FLAG_TYPE_MASK) == EFFECT_FLAG_TYPE_AUXILIARY) { detachAuxEffect_l(effect->id()); @@ -1382,7 +1397,7 @@ void AudioFlinger::ThreadBase::removeEffect_l(const sp& effect) { sp chain = effect->chain().promote(); if (chain != 0) { // remove effect chain if removing last effect - if (chain->removeEffect_l(effect) == 0) { + if (chain->removeEffect_l(effect, release) == 0) { removeEffectChain_l(chain); } } else { diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h index 8fab1e4..4009c18 100644 --- a/services/audioflinger/Threads.h +++ b/services/audioflinger/Threads.h @@ -309,7 +309,8 @@ public: int sessionId, effect_descriptor_t *desc, int *enabled, - status_t *status /*non-NULL*/); + status_t *status /*non-NULL*/, + bool pinned); // return values for hasAudioSession (bit field) enum effect_state { @@ -346,7 +347,9 @@ public: status_t addEffect_l(const sp< EffectModule>& effect); // remove and effect module. Also removes the effect chain is this was the last // effect - void removeEffect_l(const sp< EffectModule>& effect); + void removeEffect_l(const sp< EffectModule>& effect, bool release = false); + // disconnect an effect handle from module and destroy module if last handle + void disconnectEffectHandle(EffectHandle *handle, bool unpinIfLast); // detach all tracks connected to an auxiliary effect virtual void detachAuxEffect_l(int effectId __unused) {} // returns either EFFECT_SESSION if effects on this audio session exist in one diff --git a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp index b23c35e..45b3bb0 100644 --- a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp +++ b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp @@ -417,7 +417,6 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input, spaudioPolicyEffects; { Mutex::Autolock _l(mLock); - mAudioPolicyManager->releaseInput(input, session); audioPolicyEffects = mAudioPolicyEffects; } if (audioPolicyEffects != 0) { @@ -427,6 +426,10 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input, ALOGW("Failed to release effects on input %d", input); } } + { + Mutex::Autolock _l(mLock); + mAudioPolicyManager->releaseInput(input, session); + } } status_t AudioPolicyService::initStreamVolume(audio_stream_type_t stream, -- cgit v1.1 From cd5482bfac57ad358b663dff6adcc3582038c51a Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Thu, 15 Dec 2016 14:46:09 -0800 Subject: DO NOT MERGE - audioflinger: fix recursive mutex lock in EffectHandle. Bug: 33661708 Bug: 32707507 Bug: 32095713 Test: run CTS AudioEffectTest#test5_0Command, Custom binder test CVE-2017-0479 CVE-2017-0480 Change-Id: I03f674f126c191143bd8bdfe236f793e975826a5 (cherry picked from commit 31a4598a1908b3ccac7ddb33c511ce66840aa911) (cherry picked from commit 8415635765380be496da9b4578d8f134a527d86b) --- services/audioflinger/Effects.cpp | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index fb9e157..8a8b05b 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -1284,6 +1284,24 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, ALOGVV("command(), cmdCode: %d, mHasControl: %d, mEffect: %p", cmdCode, mHasControl, mEffect.unsafe_get()); + if (cmdCode == EFFECT_CMD_ENABLE) { + if (*replySize < sizeof(int)) { + android_errorWriteLog(0x534e4554, "32095713"); + return BAD_VALUE; + } + *(int *)pReplyData = NO_ERROR; + *replySize = sizeof(int); + return enable(); + } else if (cmdCode == EFFECT_CMD_DISABLE) { + if (*replySize < sizeof(int)) { + android_errorWriteLog(0x534e4554, "32095713"); + return BAD_VALUE; + } + *(int *)pReplyData = NO_ERROR; + *replySize = sizeof(int); + return disable(); + } + AutoMutex _l(mLock); sp effect = mEffect.promote(); if (effect == 0 || mDisconnected) { @@ -1299,11 +1317,17 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, // handle commands that are not forwarded transparently to effect engine if (cmdCode == EFFECT_CMD_SET_PARAM_COMMIT) { + if (*replySize < sizeof(int)) { + android_errorWriteLog(0x534e4554, "32095713"); + return BAD_VALUE; + } + *(int *)pReplyData = NO_ERROR; + *replySize = sizeof(int); + // No need to trylock() here as this function is executed in the binder thread serving a // particular client process: no risk to block the whole media server process or mixer // threads if we are stuck here Mutex::Autolock _l(mCblk->lock); - // keep local copy of index in case of client corruption b/32220769 const uint32_t clientIndex = mCblk->clientIndex; const uint32_t serverIndex = mCblk->serverIndex; @@ -1366,12 +1390,6 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode, mCblk->serverIndex = 0; mCblk->clientIndex = 0; return status; - } else if (cmdCode == EFFECT_CMD_ENABLE) { - *(int *)pReplyData = NO_ERROR; - return enable(); - } else if (cmdCode == EFFECT_CMD_DISABLE) { - *(int *)pReplyData = NO_ERROR; - return disable(); } return effect->command(cmdCode, cmdSize, pCmdData, replySize, pReplyData); -- cgit v1.1 From 85473b3d8e9a5ed76a431924b21b0b10e19bc7a0 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Thu, 12 Jan 2017 15:49:04 -0800 Subject: Don't initialize sync sample parameters until the end to avoid leaving them in a partially initialized state. Bug: 33137046 Test: ran CTS tests CVE-2017-0483 Change-Id: I1f5c070233c5917d85da9e930e01a3fc51a0a0ec (cherry picked from commit a9660fe122ca382e1777e0c5d3c42ca67ffb0377) (cherry picked from commit bc62c086e9ba7530723dc8874b83159f4d77d976) --- media/libstagefright/SampleTable.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/media/libstagefright/SampleTable.cpp b/media/libstagefright/SampleTable.cpp index 8a38c24..2d7e613 100644 --- a/media/libstagefright/SampleTable.cpp +++ b/media/libstagefright/SampleTable.cpp @@ -512,8 +512,6 @@ status_t SampleTable::setSyncSampleParams(off64_t data_offset, size_t data_size) return ERROR_MALFORMED; } - mSyncSampleOffset = data_offset; - uint8_t header[8]; if (mDataSource->readAt( data_offset, header, sizeof(header)) < (ssize_t)sizeof(header)) { @@ -525,13 +523,13 @@ status_t SampleTable::setSyncSampleParams(off64_t data_offset, size_t data_size) return ERROR_MALFORMED; } - mNumSyncSamples = U32_AT(&header[4]); + uint32_t numSyncSamples = U32_AT(&header[4]); - if (mNumSyncSamples < 2) { + if (numSyncSamples < 2) { ALOGV("Table of sync samples is empty or has only a single entry!"); } - uint64_t allocSize = (uint64_t)mNumSyncSamples * sizeof(uint32_t); + uint64_t allocSize = (uint64_t)numSyncSamples * sizeof(uint32_t); if (allocSize > kMaxTotalSize) { ALOGE("Sync sample table size too large."); return ERROR_OUT_OF_RANGE; @@ -549,22 +547,27 @@ status_t SampleTable::setSyncSampleParams(off64_t data_offset, size_t data_size) return ERROR_OUT_OF_RANGE; } - mSyncSamples = new (std::nothrow) uint32_t[mNumSyncSamples]; + mSyncSamples = new (std::nothrow) uint32_t[numSyncSamples]; if (!mSyncSamples) { ALOGE("Cannot allocate sync sample table with %llu entries.", - (unsigned long long)mNumSyncSamples); + (unsigned long long)numSyncSamples); return ERROR_OUT_OF_RANGE; } - if (mDataSource->readAt(mSyncSampleOffset + 8, mSyncSamples, + if (mDataSource->readAt(data_offset + 8, mSyncSamples, (size_t)allocSize) != (ssize_t)allocSize) { + delete mSyncSamples; + mSyncSamples = NULL; return ERROR_IO; } - for (size_t i = 0; i < mNumSyncSamples; ++i) { + for (size_t i = 0; i < numSyncSamples; ++i) { mSyncSamples[i] = ntohl(mSyncSamples[i]) - 1; } + mSyncSampleOffset = data_offset; + mNumSyncSamples = numSyncSamples; + return OK; } -- cgit v1.1 From e9598179f3e286a58e8222a3654701b330cd5268 Mon Sep 17 00:00:00 2001 From: Robert Shih Date: Tue, 24 Jan 2017 18:08:59 -0800 Subject: avc_utils: skip empty NALs from malformed bistreams Avoid a CHECK and make it the decoder's repsonsibility to handle a malformed bistream gracefully. Bug: 34509901 Bug: 33137046 Test: StagefrightTest#testStagefright_bug_27855419_CVE_2016_2463 CVE-2017-0483 Change-Id: I2d94f8da63d65a86a9c711c45546e4c695e0f3b4 (cherry picked from commit 91fe76a157847825601b8f7a627efd1c9cbadcae) (cherry picked from commit 5cabe32a59f9be1e913b6a07a23d4cfa55e3fb2f) --- media/libstagefright/avc_utils.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/media/libstagefright/avc_utils.cpp b/media/libstagefright/avc_utils.cpp index 98b5c0e..bf014ba 100644 --- a/media/libstagefright/avc_utils.cpp +++ b/media/libstagefright/avc_utils.cpp @@ -454,7 +454,10 @@ bool IsAVCReferenceFrame(const sp &accessUnit) { size_t nalSize; bool bIsReferenceFrame = true; while (getNextNALUnit(&data, &size, &nalStart, &nalSize, true) == OK) { - CHECK_GT(nalSize, 0u); + if (nalSize == 0u) { + ALOGW("skipping empty nal unit from potentially malformed bitstream"); + continue; + } unsigned nalType = nalStart[0] & 0x1f; -- cgit v1.1 From c59a656f9923a0e2fc308dabc373594e88b345d1 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Wed, 1 Feb 2017 15:27:41 -0800 Subject: CameraBase: Don't return an sp<> by reference If the server dies, the binder death callback clears out the global camera service sp<>, and any current references to it will become quite unhappy. Test: Camera CTS passes Bug: 31992879 AOSP-Change-Id: I2966bed35d0319e3f26e3d4b1b8dc08006a22348 CVE-2017-0544 Change-Id: Ib7ef455366927b0471f8fcabdd5a54e38e375d41 (cherry picked from commit 4b49489c12e6862e9a320ebcb53872e809ed20ec) --- camera/CameraBase.cpp | 10 +++++----- include/camera/CameraBase.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/camera/CameraBase.cpp b/camera/CameraBase.cpp index 5d50aa8..871dcea 100644 --- a/camera/CameraBase.cpp +++ b/camera/CameraBase.cpp @@ -65,7 +65,7 @@ namespace { // establish binder interface to camera service template -const sp& CameraBase::getCameraService() +const sp CameraBase::getCameraService() { Mutex::Autolock _l(gLock); if (gCameraService.get() == 0) { @@ -98,7 +98,7 @@ sp CameraBase::connect(int cameraId, sp c = new TCam(cameraId); sp cl = c; status_t status = NO_ERROR; - const sp& cs = getCameraService(); + const sp cs = getCameraService(); if (cs != 0) { TCamConnectService fnConnectService = TCamTraits::fnConnectService; @@ -195,7 +195,7 @@ int CameraBase::getNumberOfCameras() { template status_t CameraBase::getCameraInfo(int cameraId, struct CameraInfo* cameraInfo) { - const sp& cs = getCameraService(); + const sp cs = getCameraService(); if (cs == 0) return UNKNOWN_ERROR; return cs->getCameraInfo(cameraId, cameraInfo); } @@ -203,7 +203,7 @@ status_t CameraBase::getCameraInfo(int cameraId, template status_t CameraBase::addServiceListener( const sp& listener) { - const sp& cs = getCameraService(); + const sp cs = getCameraService(); if (cs == 0) return UNKNOWN_ERROR; return cs->addListener(listener); } @@ -211,7 +211,7 @@ status_t CameraBase::addServiceListener( template status_t CameraBase::removeServiceListener( const sp& listener) { - const sp& cs = getCameraService(); + const sp cs = getCameraService(); if (cs == 0) return UNKNOWN_ERROR; return cs->removeListener(listener); } diff --git a/include/camera/CameraBase.h b/include/camera/CameraBase.h index 1b93157..4c849de 100644 --- a/include/camera/CameraBase.h +++ b/include/camera/CameraBase.h @@ -101,7 +101,7 @@ protected: virtual void binderDied(const wp& who); // helper function to obtain camera service handle - static const sp& getCameraService(); + static const sp getCameraService(); sp mCamera; status_t mStatus; -- cgit v1.1 From f2c6b991081806343e038a687c8d8f63a747abd7 Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Mon, 13 Feb 2017 16:31:20 -0800 Subject: EffectBundle: check nb channels to write speaker angles When speaker angles are queried, the size of the array for the returned data is 3x the number of channels (where really it should be max(2, nbChannels)). The code assumed it was at least 3x2 (where 2 is the number of virtual speakers this effect supports) and would thus crash when called for a mono channel mask. Test: see repro steps in bug Bug: 32591350 AOSP-Change-Id: I33d4bff6b2e19a9fc4284a85a446804878d3a410 CVE-2017-0545 Change-Id: Ie4480d9abcfafcd53fca15ab2fd8ef7ecb6fd48d (cherry picked from commit e5a54485e08400a976092cd5b1c6d909d0e1a4ab) --- .../libeffects/lvm/wrapper/Bundle/EffectBundle.cpp | 24 ++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp index 2588140..9cddf6a 100644 --- a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp +++ b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp @@ -1465,17 +1465,25 @@ int VirtualizerForceVirtualizationMode(EffectContext *pContext, audio_devices_t // horizontal plane, +90 is directly above the user, -90 below // //---------------------------------------------------------------------------- -void VirtualizerGetSpeakerAngles(audio_channel_mask_t channelMask __unused, +void VirtualizerGetSpeakerAngles(audio_channel_mask_t channelMask, audio_devices_t deviceType __unused, int32_t *pSpeakerAngles) { // the channel count is guaranteed to be 1 or 2 // the device is guaranteed to be of type headphone - // this virtualizer is always 2in with speakers at -90 and 90deg of azimuth, 0deg of elevation - *pSpeakerAngles++ = (int32_t) AUDIO_CHANNEL_OUT_FRONT_LEFT; - *pSpeakerAngles++ = -90; // azimuth - *pSpeakerAngles++ = 0; // elevation - *pSpeakerAngles++ = (int32_t) AUDIO_CHANNEL_OUT_FRONT_RIGHT; - *pSpeakerAngles++ = 90; // azimuth - *pSpeakerAngles = 0; // elevation + // this virtualizer is always using 2 virtual speakers at -90 and 90deg of azimuth, 0deg of + // elevation but the return information is sized for nbChannels * 3, so we have to consider + // the (false here) case of a single channel, and return only 3 fields. + if (audio_channel_count_from_out_mask(channelMask) == 1) { + *pSpeakerAngles++ = (int32_t) AUDIO_CHANNEL_OUT_MONO; // same as FRONT_LEFT + *pSpeakerAngles++ = 0; // azimuth + *pSpeakerAngles = 0; // elevation + } else { + *pSpeakerAngles++ = (int32_t) AUDIO_CHANNEL_OUT_FRONT_LEFT; + *pSpeakerAngles++ = -90; // azimuth + *pSpeakerAngles++ = 0; // elevation + *pSpeakerAngles++ = (int32_t) AUDIO_CHANNEL_OUT_FRONT_RIGHT; + *pSpeakerAngles++ = 90; // azimuth + *pSpeakerAngles = 0; // elevation + } } //---------------------------------------------------------------------------- -- cgit v1.1 From 0836dbb870ff45470e595ec921fd5fab6e07d1ce Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 6 Feb 2017 14:12:30 -0800 Subject: Fix overflow check and check read result Bug: 33861560 Test: build AOSP-Change-Id: Ia85519766e19a6e37237166f309750b3e8323c4e CVE-2017-0547 (cherry picked from commit 9667e3eff2d34c3797c3b529370de47b2c1f1bf6) Change-Id: I171aa1c7c4a4a5095ac7041371db14e3a4f3676a --- media/libmedia/IHDCP.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/media/libmedia/IHDCP.cpp b/media/libmedia/IHDCP.cpp index f3a8902..e8c8a3d 100644 --- a/media/libmedia/IHDCP.cpp +++ b/media/libmedia/IHDCP.cpp @@ -241,14 +241,11 @@ status_t BnHDCP::onTransact( case HDCP_ENCRYPT: { size_t size = data.readInt32(); - size_t bufSize = 2 * size; - - // watch out for overflow void *inData = NULL; - if (bufSize > size) { - inData = malloc(bufSize); + // watch out for overflow + if (size <= SIZE_MAX / 2) { + inData = malloc(2 * size); } - if (inData == NULL) { reply->writeInt32(ERROR_OUT_OF_RANGE); return OK; @@ -256,11 +253,16 @@ status_t BnHDCP::onTransact( void *outData = (uint8_t *)inData + size; - data.read(inData, size); + status_t err = data.read(inData, size); + if (err != OK) { + free(inData); + reply->writeInt32(err); + return OK; + } uint32_t streamCTR = data.readInt32(); uint64_t inputCTR; - status_t err = encrypt(inData, size, streamCTR, &inputCTR, outData); + err = encrypt(inData, size, streamCTR, &inputCTR, outData); reply->writeInt32(err); -- cgit v1.1 From dc7805b0c79d056385a076422894425984af2aa0 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 13 Feb 2017 14:19:40 -0800 Subject: resolve merge conflicts of 79cf158c51 to mnc-dev AOSP-Change-Id: Ied32e83215e386c801c02991a0b2fa4baa25b643 CVE-2017-0558 (cherry picked from commit 50358a80b1724f6cf1bcdf003e1abf9cc141b122) Change-Id: Ic2e40c7d6aec8427444a1fd145726e490e994d08 --- media/libstagefright/wifi-display/rtp/RTPSender.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/media/libstagefright/wifi-display/rtp/RTPSender.cpp b/media/libstagefright/wifi-display/rtp/RTPSender.cpp index c66a898..83af393 100644 --- a/media/libstagefright/wifi-display/rtp/RTPSender.cpp +++ b/media/libstagefright/wifi-display/rtp/RTPSender.cpp @@ -762,10 +762,16 @@ status_t RTPSender::parseTSFB(const uint8_t *data, size_t size) { return OK; } -status_t RTPSender::parseAPP(const uint8_t *data, size_t size __unused) { - if (!memcmp("late", &data[8], 4)) { - int64_t avgLatencyUs = (int64_t)U64_AT(&data[12]); - int64_t maxLatencyUs = (int64_t)U64_AT(&data[20]); +status_t RTPSender::parseAPP(const uint8_t *data, size_t size) { + static const size_t late_offset = 8; + static const char late_string[] = "late"; + static const size_t avgLatencyUs_offset = late_offset + sizeof(late_string) - 1; + static const size_t maxLatencyUs_offset = avgLatencyUs_offset + sizeof(int64_t); + + if ((size >= (maxLatencyUs_offset + sizeof(int64_t))) + && !memcmp(late_string, &data[late_offset], sizeof(late_string) - 1)) { + int64_t avgLatencyUs = (int64_t)U64_AT(&data[avgLatencyUs_offset]); + int64_t maxLatencyUs = (int64_t)U64_AT(&data[maxLatencyUs_offset]); sp notify = mNotify->dup(); notify->setInt32("what", kWhatInformSender); -- cgit v1.1