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(-) (limited to 'services') 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) --- services/audioflinger/Effects.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'services') 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(-) (limited to 'services') 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(-) (limited to 'services') 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