From ff7996a2460d8efb9ed97a99e698d01f52bd2e6e Mon Sep 17 00:00:00 2001 From: Manisha Jajoo Date: Fri, 23 Jul 2021 23:03:50 +0530 Subject: [PATCH 01/18] C2SoftMp3Dec: fix OOB write in output buffer outputFrameSize, calOutSize and outSize are calculated at 8bit level However, the library expects outputFrameSize in int16 samples. One of the initialization of outputFrameSize was in bytes. This is now corrected. Test: clusterfuzz generated poc in bug Test: atest android.mediav2.cts.CodecDecoderTest Test: atest VtsHalMediaC2V1_0TargetAudioDecTest Bug: 193363621 Change-Id: Iac62c4e9d77e7f95f2c692f5ea236e7a5c536dcb (cherry picked from commit dc32721e28e79df4dd2f5bb896bcf586ebeda5e9) --- media/codec2/components/mp3/C2SoftMp3Dec.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/media/codec2/components/mp3/C2SoftMp3Dec.cpp b/media/codec2/components/mp3/C2SoftMp3Dec.cpp index 5ba7e3d78f..3984f62278 100644 --- a/media/codec2/components/mp3/C2SoftMp3Dec.cpp +++ b/media/codec2/components/mp3/C2SoftMp3Dec.cpp @@ -405,7 +405,7 @@ void C2SoftMP3::process( mConfig->inputBufferCurrentLength = (inSize - inPos); mConfig->inputBufferMaxLength = 0; mConfig->inputBufferUsedLength = 0; - mConfig->outputFrameSize = (calOutSize - outSize); + mConfig->outputFrameSize = (calOutSize - outSize) / sizeof(int16_t); mConfig->pOutputBuffer = reinterpret_cast (wView.data() + outSize); ERROR_CODE decoderErr; From db52d3fbbc3ea9c0717ba6d3f0e1210901951b9e Mon Sep 17 00:00:00 2001 From: Santiago Seifert Date: Thu, 30 Sep 2021 13:15:21 +0000 Subject: [PATCH 02/18] Fix heap-buffer-overflow in MPEG4Extractor am: d13a4efc7a Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/av/+/15747591 Bug: 201632451 Bug: 188893559 Change-Id: Ie775311a46cb1ddddd30e8cfa882d549b9ddfd05 Merged-In: I31f2b9a4f1b561c4466c76ea2af8dd532622102a (cherry picked from commit 3c5de138ed3b697e0119e7526ae7f6ed09f357cc) --- media/extractors/mp4/MPEG4Extractor.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) mode change 100755 => 100644 media/extractors/mp4/MPEG4Extractor.cpp diff --git a/media/extractors/mp4/MPEG4Extractor.cpp b/media/extractors/mp4/MPEG4Extractor.cpp old mode 100755 new mode 100644 index a976a2b12a..f157d359b2 --- a/media/extractors/mp4/MPEG4Extractor.cpp +++ b/media/extractors/mp4/MPEG4Extractor.cpp @@ -146,6 +146,7 @@ static const size_t kMaxPcmFrameSize = 8192; MediaBufferHelper *mBuffer; + size_t mSrcBufferSize; uint8_t *mSrcBuffer; bool mIsHeif; @@ -4882,6 +4883,7 @@ MPEG4Source::MPEG4Source( mNALLengthSize(0), mStarted(false), mBuffer(NULL), + mSrcBufferSize(0), mSrcBuffer(NULL), mIsHeif(itemTable != NULL), mItemTable(itemTable), @@ -5060,6 +5062,7 @@ media_status_t MPEG4Source::start() { // file probably specified a bad max size return AMEDIA_ERROR_MALFORMED; } + mSrcBufferSize = max_size; mStarted = true; @@ -5076,6 +5079,7 @@ media_status_t MPEG4Source::stop() { mBuffer = NULL; } + mSrcBufferSize = 0; delete[] mSrcBuffer; mSrcBuffer = NULL; @@ -6242,13 +6246,20 @@ media_status_t MPEG4Source::read( // Whole NAL units are returned but each fragment is prefixed by // the start code (0x00 00 00 01). ssize_t num_bytes_read = 0; - num_bytes_read = mDataSource->readAt(offset, mSrcBuffer, size); + bool mSrcBufferFitsDataToRead = size <= mSrcBufferSize; + if (mSrcBufferFitsDataToRead) { + num_bytes_read = mDataSource->readAt(offset, mSrcBuffer, size); + } else { + // We are trying to read a sample larger than the expected max sample size. + // Fall through and let the failure be handled by the following if. + android_errorWriteLog(0x534e4554, "188893559"); + } if (num_bytes_read < (ssize_t)size) { mBuffer->release(); mBuffer = NULL; - return AMEDIA_ERROR_IO; + return mSrcBufferFitsDataToRead ? AMEDIA_ERROR_IO : AMEDIA_ERROR_MALFORMED; } uint8_t *dstData = (uint8_t *)mBuffer->data(); From 0f5bd6a8c2f0c9aa38e06689de904aa8e2be8b3c Mon Sep 17 00:00:00 2001 From: Gopalakrishnan Nallasamy Date: Wed, 29 Sep 2021 08:24:26 -0700 Subject: [PATCH 03/18] SimpleDecodingSource:Prevent OOB write in heap mem doRead() doesn't handle situations when received byte do not fit into input buffer in case of vorbis audio compression. It results in OOB write in heap memory right after the allocated input buffer. Added code to copy kKeyValidSamples only if there was enough space. Otherwise, print a warning log. Bug: 194105348 Test: post-submit media cts tests Change-Id: I2b27580deff9ad937b68703a1e7c3ff2a6dccc60 (cherry picked from commit a625b40e1c210f1e8ed57962eee9f70cef06fb1b) (cherry picked from commit f3590a1b18d8cde4ac1cbc135c1022816096438d) Merged-In:I2b27580deff9ad937b68703a1e7c3ff2a6dccc60 --- media/libstagefright/SimpleDecodingSource.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/media/libstagefright/SimpleDecodingSource.cpp b/media/libstagefright/SimpleDecodingSource.cpp index 771dfeabf5..55aa86b8cb 100644 --- a/media/libstagefright/SimpleDecodingSource.cpp +++ b/media/libstagefright/SimpleDecodingSource.cpp @@ -318,18 +318,23 @@ status_t SimpleDecodingSource::doRead( } size_t cpLen = min(in_buf->range_length(), in_buffer->capacity()); memcpy(in_buffer->base(), (uint8_t *)in_buf->data() + in_buf->range_offset(), - cpLen ); + cpLen); if (mIsVorbis) { int32_t numPageSamples; if (!in_buf->meta_data().findInt32(kKeyValidSamples, &numPageSamples)) { numPageSamples = -1; } - memcpy(in_buffer->base() + cpLen, &numPageSamples, sizeof(numPageSamples)); + if (cpLen + sizeof(numPageSamples) <= in_buffer->capacity()) { + memcpy(in_buffer->base() + cpLen, &numPageSamples, sizeof(numPageSamples)); + cpLen += sizeof(numPageSamples); + } else { + ALOGW("Didn't have enough space to copy kKeyValidSamples"); + } } res = mCodec->queueInputBuffer( - in_ix, 0 /* offset */, in_buf->range_length() + (mIsVorbis ? 4 : 0), + in_ix, 0 /* offset */, cpLen, timestampUs, 0 /* flags */); if (res != OK) { ALOGI("[%s] failed to queue input buffer #%zu", mComponentName.c_str(), in_ix); From 75b1ca1d7052baaef0b66e400b8624c9056f0ce5 Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Tue, 9 Nov 2021 16:14:41 -0800 Subject: [PATCH 04/18] Better buffer-overrun prevention fixes end-of-buffer detection. Adds buffer-was-empty detection. Bug: 204445255 Test: ran poc from bug Change-Id: I42117ce1455d1cac2bd43f16d67d77ec436b0fe2 (cherry picked from commit b51ed962d5186b68f883540e557894e881a8272d) (cherry picked from commit 190e90959f3c34781c5276d50a5ee561c438db09) Merged-In:I42117ce1455d1cac2bd43f16d67d77ec436b0fe2 --- media/libmediametrics/include/media/MediaMetricsItem.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/media/libmediametrics/include/media/MediaMetricsItem.h b/media/libmediametrics/include/media/MediaMetricsItem.h index 303343f91c..e36f0a0f39 100644 --- a/media/libmediametrics/include/media/MediaMetricsItem.h +++ b/media/libmediametrics/include/media/MediaMetricsItem.h @@ -466,16 +466,15 @@ class BaseItem { template <> // static status_t extract(std::string *val, const char **bufferpptr, const char *bufferptrmax) { const char *ptr = *bufferpptr; - while (*ptr != 0) { + do { if (ptr >= bufferptrmax) { ALOGE("%s: buffer exceeded", __func__); return BAD_VALUE; } - ++ptr; - } - const size_t size = (ptr - *bufferpptr) + 1; + } while (*ptr++ != 0); + // ptr is terminator+1, == bufferptrmax if we finished entire buffer *val = *bufferpptr; - *bufferpptr += size; + *bufferpptr = ptr; return NO_ERROR; } template <> // static From 9f254c6681ca9aa0f12e011699278d7a12c24515 Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Mon, 6 Dec 2021 10:22:33 -0800 Subject: [PATCH 05/18] Safetynet logging for b/204445255 Bug: 204445255 Test: poc from original bug Change-Id: I569477d0771e1c03318df9ef271cf3201d472c99 (cherry picked from commit 94e58d6b2497d2e0f7e86e2c979e7f6958c84590) Merged-In:I569477d0771e1c03318df9ef271cf3201d472c99 --- media/libmediametrics/include/media/MediaMetricsItem.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/media/libmediametrics/include/media/MediaMetricsItem.h b/media/libmediametrics/include/media/MediaMetricsItem.h index e36f0a0f39..7ce75ed4f7 100644 --- a/media/libmediametrics/include/media/MediaMetricsItem.h +++ b/media/libmediametrics/include/media/MediaMetricsItem.h @@ -27,6 +27,7 @@ #include #include +#include #include #include // nsecs_t @@ -469,6 +470,7 @@ class BaseItem { do { if (ptr >= bufferptrmax) { ALOGE("%s: buffer exceeded", __func__); + android_errorWriteLog(0x534e4554, "204445255"); return BAD_VALUE; } } while (*ptr++ != 0); From bb8cdd2b270ac7198f765fc2bec9281ddd27722d Mon Sep 17 00:00:00 2001 From: Gopalakrishnan Nallasamy Date: Tue, 11 Jan 2022 23:44:20 -0800 Subject: [PATCH 06/18] C2AllocatorIon:protect mMappings using mutex Use mutex to prevent multiple threads accessing same member of mMappings list at the same time. Bug: 193790350 Test: adb shell UBSAN_OPTIONS=print_stacktrace=1 /data/local/tmp/C2FuzzerMp3Dec -rss_limit_mb=2560 -timeout=90 -runs=100 /data/local/tmp/clusterfuzz-testcase-minimized-C2FuzzerMp3Dec-5713156165206016 Change-Id: I24e53629d5a6dfad22b84dd2278eb1a288c9ab35 Merged-In: I24e53629d5a6dfad22b84dd2278eb1a288c9ab35 (cherry picked from commit 9d2295f3a008f60bcfa3d2da3b43c078efec1878) (cherry picked from commit 416da6e8da6b6a16c5c00bddd9fbc7a5f060cd58) Merged-In:I24e53629d5a6dfad22b84dd2278eb1a288c9ab35 --- media/codec2/vndk/C2AllocatorIon.cpp | 37 +++++++++++++++++----------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/media/codec2/vndk/C2AllocatorIon.cpp b/media/codec2/vndk/C2AllocatorIon.cpp index 6d27a0212d..9410ce93fc 100644 --- a/media/codec2/vndk/C2AllocatorIon.cpp +++ b/media/codec2/vndk/C2AllocatorIon.cpp @@ -202,6 +202,7 @@ class C2AllocationIon::Impl { c2_status_t err = mapInternal(mapSize, mapOffset, alignmentBytes, prot, flags, &(map.addr), addr); if (map.addr) { + std::lock_guard guard(mMutexMappings); mMappings.push_back(map); } return err; @@ -212,22 +213,26 @@ class C2AllocationIon::Impl { ALOGD("tried to unmap unmapped buffer"); return C2_NOT_FOUND; } - for (auto it = mMappings.begin(); it != mMappings.end(); ++it) { - if (addr != (uint8_t *)it->addr + it->alignmentBytes || - size + it->alignmentBytes != it->size) { - continue; + { // Scope for the lock_guard of mMutexMappings. + std::lock_guard guard(mMutexMappings); + for (auto it = mMappings.begin(); it != mMappings.end(); ++it) { + if (addr != (uint8_t *)it->addr + it->alignmentBytes || + size + it->alignmentBytes != it->size) { + continue; + } + int err = munmap(it->addr, it->size); + if (err != 0) { + ALOGD("munmap failed"); + return c2_map_errno(errno); + } + if (fence) { + *fence = C2Fence(); // not using fences + } + (void)mMappings.erase(it); + ALOGV("successfully unmapped: addr=%p size=%zu fd=%d", addr, size, + mHandle.bufferFd()); + return C2_OK; } - int err = munmap(it->addr, it->size); - if (err != 0) { - ALOGD("munmap failed"); - return c2_map_errno(errno); - } - if (fence) { - *fence = C2Fence(); // not using fences - } - (void)mMappings.erase(it); - ALOGV("successfully unmapped: addr=%p size=%zu fd=%d", addr, size, mHandle.bufferFd()); - return C2_OK; } ALOGD("unmap failed to find specified map"); return C2_BAD_VALUE; @@ -236,6 +241,7 @@ class C2AllocationIon::Impl { virtual ~Impl() { if (!mMappings.empty()) { ALOGD("Dangling mappings!"); + std::lock_guard guard(mMutexMappings); for (const Mapping &map : mMappings) { (void)munmap(map.addr, map.size); } @@ -315,6 +321,7 @@ class C2AllocationIon::Impl { size_t size; }; std::list mMappings; + std::mutex mMutexMappings; }; class C2AllocationIon::ImplV2 : public C2AllocationIon::Impl { From 4a0d46b1b42008dc041b237a44cbbda452a649ea Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Wed, 2 Feb 2022 13:33:50 -0800 Subject: [PATCH 07/18] Safe parsing of HEIF framecount information Bug: 215002587 Test: POC described in bug Change-Id: I92f8fdfe860cb360fb0ae099db3c92776ba7390f (cherry picked from commit e89e632f9aa04e15291ee096b3152b40474a993d) (cherry picked from commit 616bd340ecded759720199bcf5b8562e0fdf3f59) Merged-In:I92f8fdfe860cb360fb0ae099db3c92776ba7390f --- media/libheif/HeifDecoderImpl.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/media/libheif/HeifDecoderImpl.cpp b/media/libheif/HeifDecoderImpl.cpp index 273d91ccde..4a96e7b093 100644 --- a/media/libheif/HeifDecoderImpl.cpp +++ b/media/libheif/HeifDecoderImpl.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -421,7 +422,13 @@ bool HeifDecoderImpl::reinit(HeifFrameInfo* frameInfo) { initFrameInfo(&mSequenceInfo, videoFrame); - mSequenceLength = atoi(mRetriever->extractMetadata(METADATA_KEY_VIDEO_FRAME_COUNT)); + const char* frameCount = mRetriever->extractMetadata(METADATA_KEY_VIDEO_FRAME_COUNT); + if (frameCount == nullptr) { + android_errorWriteWithInfoLog(0x534e4554, "215002587", -1, NULL, 0); + ALOGD("No valid sequence information in metadata"); + return false; + } + mSequenceLength = atoi(frameCount); if (defaultInfo == nullptr) { defaultInfo = &mSequenceInfo; From 97740a37b2de2fe0b0324e5a44317318f5d07e46 Mon Sep 17 00:00:00 2001 From: Santiago Seifert Date: Thu, 19 May 2022 15:29:26 +0000 Subject: [PATCH 08/18] Avoid read out of bounds Bug: 230493653 Change-Id: Ieca5a5390d3cf73fff6aa552d065363d84e1ccc2 Merged-In: Ieca5a5390d3cf73fff6aa552d065363d84e1ccc2 Test: See bug for PoC. (cherry picked from commit 306aad773337f228bffcf5bf07a3e6663226f42c) (cherry picked from commit 9d33304ec75b366ed9750e7bde6f96f8c704e1c8) Merged-In: Ieca5a5390d3cf73fff6aa552d065363d84e1ccc2 --- media/extractors/mp4/MPEG4Extractor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/media/extractors/mp4/MPEG4Extractor.cpp b/media/extractors/mp4/MPEG4Extractor.cpp index f157d359b2..78e3f00b44 100644 --- a/media/extractors/mp4/MPEG4Extractor.cpp +++ b/media/extractors/mp4/MPEG4Extractor.cpp @@ -4573,7 +4573,7 @@ status_t MPEG4Extractor::updateAudioTrackInfoFromESDS_MPEG4Audio( if (len2 == 0) { return ERROR_MALFORMED; } - if (offset >= csd_size || csd[offset] != 0x01) { + if (offset + len1 > csd_size || csd[offset] != 0x01) { return ERROR_MALFORMED; } // formerly kKeyVorbisInfo From d9c234a9e92beba7121624432de648379073a9d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Budnik?= Date: Mon, 20 Jun 2022 10:36:28 +0000 Subject: [PATCH 09/18] Fix Out of Bounds read in TextDescriptions.cpp Fixing vulnerability in extract3GGPGlobalDescriptions() in TextDescriptions.cpp Bug: 233735886 Test: Run related PoC. See bug. Change-Id: I87955b911d0a40390755321d332a11ecc9b20354 (cherry picked from commit b63d4e785ba4d896bbbd50d4f09bda13294926af) Merged-In: I87955b911d0a40390755321d332a11ecc9b20354 --- media/libstagefright/timedtext/TextDescriptions.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/media/libstagefright/timedtext/TextDescriptions.cpp b/media/libstagefright/timedtext/TextDescriptions.cpp index 2c2d11d45b..3fec9edf65 100644 --- a/media/libstagefright/timedtext/TextDescriptions.cpp +++ b/media/libstagefright/timedtext/TextDescriptions.cpp @@ -466,6 +466,10 @@ status_t TextDescriptions::extract3GPPGlobalDescriptions( if (subChunkType == FOURCC('f', 't', 'a', 'b')) { + if(subChunkSize < 8) { + return OK; + } + tmpData += 8; size_t subChunkRemaining = subChunkSize - 8; From 16b5ee4aa1587135737189fb03820f2de2d49139 Mon Sep 17 00:00:00 2001 From: jiabin Date: Wed, 15 Jun 2022 19:26:01 +0000 Subject: [PATCH 10/18] Cache MMAP client silenced state. When starting MMAP input stream, APM will check if the client is allowed to capture at that moment or not and call setRecordSilenced if the client is not allowed. However, the client is not active when starting the MMAP input stream. In that case, the client silenced state will be lost and the client will be able to capture even though it is not allowed. In this CL, when setRecordSilenced is called, it will cache the client silenced state so that it can apply when the client is active. Test: atest AAudioTests Test: repo steps from the bug Bug: 235850634 Change-Id: I49b5a0f08d1747053f868db6e88c0f677256fc3c Merged-In: I49b5a0f08d1747053f868db6e88c0f677256fc3c (cherry picked from commit 0960903b2fee5d1d449ffcd598e0b5d3a945d99a) (cherry picked from commit a2f00f95e0e74efe439a591b236afb598dbf8972) Merged-In: I49b5a0f08d1747053f868db6e88c0f677256fc3c --- services/audioflinger/Threads.cpp | 12 ++++++++++++ services/audioflinger/Threads.h | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 1d0147d56b..f9a1d9cdcd 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -8885,6 +8885,12 @@ status_t AudioFlinger::MmapThread::start(const AudioClient& client, if (isOutput()) { ret = AudioSystem::startOutput(portId); } else { + { + // Add the track record before starting input so that the silent status for the + // client can be cached. + Mutex::Autolock _l(mLock); + setClientSilencedState_l(portId, false /*silenced*/); + } ret = AudioSystem::startInput(portId); } @@ -8903,6 +8909,7 @@ status_t AudioFlinger::MmapThread::start(const AudioClient& client, } else { mHalStream->stop(); } + eraseClientSilencedState_l(portId); return PERMISSION_DENIED; } @@ -8911,6 +8918,9 @@ status_t AudioFlinger::MmapThread::start(const AudioClient& client, mChannelMask, mSessionId, isOutput(), client.clientUid, client.clientPid, IPCThreadState::self()->getCallingPid(), portId); + if (!isOutput()) { + track->setSilenced_l(isClientSilenced_l(portId)); + } if (isOutput()) { // force volume update when a new track is added @@ -8967,6 +8977,7 @@ status_t AudioFlinger::MmapThread::stop(audio_port_handle_t handle) } mActiveTracks.remove(track); + eraseClientSilencedState_l(track->portId()); mLock.unlock(); if (isOutput()) { @@ -9746,6 +9757,7 @@ void AudioFlinger::MmapCaptureThread::setRecordSilenced(audio_port_handle_t port broadcast_l(); } } + setClientSilencedIfExists_l(portId, silenced); } void AudioFlinger::MmapCaptureThread::toAudioPortConfig(struct audio_port_config *config) diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h index 6b33ad5c2e..2c2d664bbe 100644 --- a/services/audioflinger/Threads.h +++ b/services/audioflinger/Threads.h @@ -1843,6 +1843,26 @@ class MmapThread : public ThreadBase virtual void setRecordSilenced(audio_port_handle_t portId __unused, bool silenced __unused) {} + void setClientSilencedState_l(audio_port_handle_t portId, bool silenced) { + mClientSilencedStates[portId] = silenced; + } + + size_t eraseClientSilencedState_l(audio_port_handle_t portId) { + return mClientSilencedStates.erase(portId); + } + + bool isClientSilenced_l(audio_port_handle_t portId) const { + const auto it = mClientSilencedStates.find(portId); + return it != mClientSilencedStates.end() ? it->second : false; + } + + void setClientSilencedIfExists_l(audio_port_handle_t portId, bool silenced) { + const auto it = mClientSilencedStates.find(portId); + if (it != mClientSilencedStates.end()) { + it->second = silenced; + } + } + protected: void dumpInternals_l(int fd, const Vector& args) override; void dumpTracks_l(int fd, const Vector& args) override; @@ -1862,6 +1882,7 @@ class MmapThread : public ThreadBase AudioHwDevice* const mAudioHwDev; ActiveTracks mActiveTracks; float mHalVolFloat; + std::map mClientSilencedStates; int32_t mNoCallbackWarningCount; static constexpr int32_t kMaxNoCallbackWarnings = 5; From 65039e412b4d1ab446e3c8c8a1340d813c601cac Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Tue, 21 Jun 2022 01:36:43 +0000 Subject: [PATCH 11/18] RESTRICT AUTOMERGE - [Fix vulnerability] setSecurityLevel in clearkey Potential race condition in clearkey setSecurityLevel. POC test in http://go/ag/19083795 Test: sts-tradefed run sts-dynamic-develop -m StsHostTestCases -t android.security.sts.CVE_2022_2209#testPocCVE_2022_2209 Bug: 235601882 Change-Id: I6447fb539ef0cb395772c61e6f3e1504ccde331b Merged-In: I2e2084e85fe45d7d7f958c59b0063a477c7d24bf (cherry picked from commit d37b69272aa68a92357baa95d0eb87012666a90b) Merged-In: I6447fb539ef0cb395772c61e6f3e1504ccde331b --- drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp | 2 ++ drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp b/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp index 6f69110d50..0e5169c9e0 100644 --- a/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp +++ b/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp @@ -623,6 +623,7 @@ Return DrmPlugin::getSecurityLevel(const hidl_vec& sessionId, return Void(); } + Mutex::Autolock lock(mSecurityLevelLock); std::map, SecurityLevel>::iterator itr = mSecurityLevel.find(sid); if (itr == mSecurityLevel.end()) { @@ -653,6 +654,7 @@ Return DrmPlugin::setSecurityLevel(const hidl_vec& sessionId, return Status::ERROR_DRM_SESSION_NOT_OPENED; } + Mutex::Autolock lock(mSecurityLevelLock); std::map, SecurityLevel>::iterator itr = mSecurityLevel.find(sid); if (itr != mSecurityLevel.end()) { diff --git a/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h b/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h index 894985bd1b..e957cee194 100644 --- a/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h +++ b/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h @@ -398,7 +398,8 @@ struct DrmPlugin : public IDrmPlugin { std::map mStringProperties; std::map > mByteArrayProperties; std::map > mReleaseKeysMap; - std::map, SecurityLevel> mSecurityLevel; + std::map, SecurityLevel> mSecurityLevel + GUARDED_BY(mSecurityLevelLock); sp mListener; sp mListenerV1_2; SessionLibrary *mSessionLibrary; @@ -419,6 +420,7 @@ struct DrmPlugin : public IDrmPlugin { DeviceFiles mFileHandle GUARDED_BY(mFileHandleLock); Mutex mFileHandleLock; Mutex mSecureStopLock; + Mutex mSecurityLevelLock; CLEARKEY_DISALLOW_COPY_AND_ASSIGN_AND_NEW(DrmPlugin); }; From 7738f239cae36714d8ec29576f9166e0fb84695e Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Thu, 1 Dec 2022 15:33:25 -0600 Subject: [PATCH 12/18] move MediaCodec metrics processing to looper thread consolidate to avoid concurrency/mutex problems. Bug: 256087846 Bug: 245860753 Test: atest CtsMediaV2TestCases Test: atest CtsMediaCodecTestCases Merged-In: Ie77f0028cab8091edd97d3a60ad4c80da3092cfe Merged-In: I56eceb6b12ce14348d3f9f2944968e70c6086aa8 Merged-In: I94b0a2ac029dc0b90a93e9ed844768e9da5259b9 Change-Id: I739248436a4801a4b9a96395f481640f2956cedf (cherry picked from commit 49e842e70836bbd58970beefac9c7b6bfe6a124b) Merged-In: I739248436a4801a4b9a96395f481640f2956cedf --- media/libstagefright/MediaCodec.cpp | 119 ++++++++++++++---- .../include/media/stagefright/MediaCodec.h | 4 + 2 files changed, 101 insertions(+), 22 deletions(-) diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp index 553f59a41f..8283187c80 100644 --- a/media/libstagefright/MediaCodec.cpp +++ b/media/libstagefright/MediaCodec.cpp @@ -720,6 +720,8 @@ MediaCodec::MediaCodec( }; } + // we want an empty metrics record for any early getMetrics() call + // this should be the *only* initMediametrics() call that's not on the Looper thread initMediametrics(); } @@ -728,8 +730,17 @@ MediaCodec::~MediaCodec() { mResourceManagerProxy->removeClient(); flushMediametrics(); + + // clean any saved metrics info we stored as part of configure() + if (mConfigureMsg != nullptr) { + mediametrics_handle_t metricsHandle; + if (mConfigureMsg->findInt64("metrics", &metricsHandle)) { + mediametrics_delete(metricsHandle); + } + } } +// except for in constructor, called from the looper thread (and therefore mutexed) void MediaCodec::initMediametrics() { if (mMetricsHandle == 0) { mMetricsHandle = mediametrics_create(kCodecKeyName); @@ -759,11 +770,12 @@ void MediaCodec::initMediametrics() { } void MediaCodec::updateMediametrics() { - ALOGV("MediaCodec::updateMediametrics"); if (mMetricsHandle == 0) { return; } + Mutex::Autolock _lock(mMetricsLock); + if (mLatencyHist.getCount() != 0 ) { mediametrics_setInt64(mMetricsHandle, kCodecLatencyMax, mLatencyHist.getMax()); mediametrics_setInt64(mMetricsHandle, kCodecLatencyMin, mLatencyHist.getMin()); @@ -798,6 +810,8 @@ void MediaCodec::updateMediametrics() { #endif } +// called to update info being passed back via getMetrics(), which is a +// unique copy for that call, no concurrent access worries. void MediaCodec::updateEphemeralMediametrics(mediametrics_handle_t item) { ALOGD("MediaCodec::updateEphemeralMediametrics()"); @@ -837,7 +851,13 @@ void MediaCodec::updateEphemeralMediametrics(mediametrics_handle_t item) { } void MediaCodec::flushMediametrics() { + ALOGD("flushMediametrics"); + + // update does its own mutex locking updateMediametrics(); + + // ensure mutex while we do our own work + Mutex::Autolock _lock(mMetricsLock); if (mMetricsHandle != 0) { if (mediametrics_count(mMetricsHandle) > 0) { mediametrics_selfRecord(mMetricsHandle); @@ -1220,6 +1240,8 @@ status_t MediaCodec::init(const AString &name) { } msg->setString("name", name); + // initial naming setup covers the period before the first call to ::configure(). + // after that, we manage this through ::configure() and the setup message. if (mMetricsHandle != 0) { mediametrics_setCString(mMetricsHandle, kCodecCodec, name.c_str()); mediametrics_setCString(mMetricsHandle, kCodecMode, @@ -1279,18 +1301,24 @@ status_t MediaCodec::configure( const sp &descrambler, uint32_t flags) { sp msg = new AMessage(kWhatConfigure, this); + mediametrics_handle_t nextMetricsHandle = mediametrics_create(kCodecKeyName); - if (mMetricsHandle != 0) { + if (nextMetricsHandle != 0) { int32_t profile = 0; if (format->findInt32("profile", &profile)) { - mediametrics_setInt32(mMetricsHandle, kCodecProfile, profile); + mediametrics_setInt32(nextMetricsHandle, kCodecProfile, profile); } int32_t level = 0; if (format->findInt32("level", &level)) { - mediametrics_setInt32(mMetricsHandle, kCodecLevel, level); + mediametrics_setInt32(nextMetricsHandle, kCodecLevel, level); } - mediametrics_setInt32(mMetricsHandle, kCodecEncoder, + mediametrics_setInt32(nextMetricsHandle, kCodecEncoder, (flags & CONFIGURE_FLAG_ENCODE) ? 1 : 0); + + // moved here from ::init() + mediametrics_setCString(nextMetricsHandle, kCodecCodec, mInitName.c_str()); + mediametrics_setCString(nextMetricsHandle, kCodecMode, + mIsVideo ? kCodecModeVideo : kCodecModeAudio); } if (mIsVideo) { @@ -1300,17 +1328,17 @@ status_t MediaCodec::configure( mRotationDegrees = 0; } - if (mMetricsHandle != 0) { - mediametrics_setInt32(mMetricsHandle, kCodecWidth, mVideoWidth); - mediametrics_setInt32(mMetricsHandle, kCodecHeight, mVideoHeight); - mediametrics_setInt32(mMetricsHandle, kCodecRotation, mRotationDegrees); + if (nextMetricsHandle != 0) { + mediametrics_setInt32(nextMetricsHandle, kCodecWidth, mVideoWidth); + mediametrics_setInt32(nextMetricsHandle, kCodecHeight, mVideoHeight); + mediametrics_setInt32(nextMetricsHandle, kCodecRotation, mRotationDegrees); int32_t maxWidth = 0; if (format->findInt32("max-width", &maxWidth)) { - mediametrics_setInt32(mMetricsHandle, kCodecMaxWidth, maxWidth); + mediametrics_setInt32(nextMetricsHandle, kCodecMaxWidth, maxWidth); } int32_t maxHeight = 0; if (format->findInt32("max-height", &maxHeight)) { - mediametrics_setInt32(mMetricsHandle, kCodecMaxHeight, maxHeight); + mediametrics_setInt32(nextMetricsHandle, kCodecMaxHeight, maxHeight); } } @@ -1334,13 +1362,23 @@ status_t MediaCodec::configure( } else { msg->setPointer("descrambler", descrambler.get()); } - if (mMetricsHandle != 0) { - mediametrics_setInt32(mMetricsHandle, kCodecCrypto, 1); + if (nextMetricsHandle != 0) { + mediametrics_setInt32(nextMetricsHandle, kCodecCrypto, 1); } } else if (mFlags & kFlagIsSecure) { ALOGW("Crypto or descrambler should be given for secure codec"); } + if (mConfigureMsg != nullptr) { + // if re-configuring, we have one of these from before. + // Recover the space before we discard the old mConfigureMsg + mediametrics_handle_t metricsHandle; + if (mConfigureMsg->findInt64("metrics", &metricsHandle)) { + mediametrics_delete(metricsHandle); + } + } + msg->setInt64("metrics", nextMetricsHandle); + // save msg for reset mConfigureMsg = msg; @@ -1851,24 +1889,42 @@ status_t MediaCodec::getCodecInfo(sp *codecInfo) const { return OK; } +// this is the user-callable entry point status_t MediaCodec::getMetrics(mediametrics_handle_t &reply) { reply = 0; - // shouldn't happen, but be safe - if (mMetricsHandle == 0) { - return UNKNOWN_ERROR; + sp msg = new AMessage(kWhatGetMetrics, this); + sp response; + status_t err; + if ((err = PostAndAwaitResponse(msg, &response)) != OK) { + return err; } - // update any in-flight data that's not carried within the record - updateMediametrics(); + CHECK(response->findInt64("metrics", &reply)); - // send it back to the caller. - reply = mediametrics_dup(mMetricsHandle); + return OK; +} - updateEphemeralMediametrics(reply); +// runs on the looper thread (for mutex purposes) +void MediaCodec::onGetMetrics(const sp& msg) { - return OK; + mediametrics_handle_t results = 0; + + sp replyID; + CHECK(msg->senderAwaitsResponse(&replyID)); + + if (mMetricsHandle != 0) { + updateMediametrics(); + results = mediametrics_dup(mMetricsHandle); + updateEphemeralMediametrics(results); + } else { + results = mediametrics_dup(mMetricsHandle); + } + + sp response = new AMessage; + response->setInt64("metrics", results); + response->postReply(replyID); } status_t MediaCodec::getInputBuffers(Vector > *buffers) const { @@ -2813,6 +2869,13 @@ void MediaCodec::onMessageReceived(const sp &msg) { break; } + case kWhatGetMetrics: + { + onGetMetrics(msg); + break; + } + + case kWhatConfigure: { if (mState != INITIALIZED) { @@ -2833,6 +2896,18 @@ void MediaCodec::onMessageReceived(const sp &msg) { sp format; CHECK(msg->findMessage("format", &format)); + // start with a copy of the passed metrics info for use in this run + mediametrics_handle_t handle; + CHECK(msg->findInt64("metrics", &handle)); + if (handle != 0) { + if (mMetricsHandle != 0) { + flushMediametrics(); + } + mMetricsHandle = mediametrics_dup(handle); + // and set some additional metrics values + initMediametrics(); + } + int32_t push; if (msg->findInt32("push-blank-buffers-on-shutdown", &push) && push != 0) { mFlags |= kFlagPushBlankBuffersOnShutdown; diff --git a/media/libstagefright/include/media/stagefright/MediaCodec.h b/media/libstagefright/include/media/stagefright/MediaCodec.h index 7614ba5e6b..24f148e717 100644 --- a/media/libstagefright/include/media/stagefright/MediaCodec.h +++ b/media/libstagefright/include/media/stagefright/MediaCodec.h @@ -328,6 +328,7 @@ struct MediaCodec : public AHandler { kWhatSetNotification = 'setN', kWhatDrmReleaseCrypto = 'rDrm', kWhatCheckBatteryStats = 'chkB', + kWhatGetMetrics = 'getM', }; enum { @@ -373,6 +374,7 @@ struct MediaCodec : public AHandler { sp mSurface; SoftwareRenderer *mSoftRenderer; + Mutex mMetricsLock; mediametrics_handle_t mMetricsHandle = 0; nsecs_t mLifetimeStartNs = 0; void initMediametrics(); @@ -380,6 +382,8 @@ struct MediaCodec : public AHandler { void flushMediametrics(); void updateEphemeralMediametrics(mediametrics_handle_t item); void updateLowLatency(const sp &msg); + void onGetMetrics(const sp& msg); + sp mOutputFormat; sp mInputFormat; From 58e6ac74409755976fa040ba2649e668ce0ea063 Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Mon, 27 Mar 2023 18:16:46 -0500 Subject: [PATCH 13/18] Fix NuMediaExtractor::readSampleData buffer Handling readSampleData() did not initialize buffer before filling it, leading to OOB memory references. Correct and clarify the book keeping around output buffer management. Bug: 275418191 Test: CtsMediaExtractorTestCases w/debug messages (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:943fc12219b21d2a98f0ddc070b9b316a6f5d412) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:84c69bca81175feb2fd97ebb22e432ee41572786) Merged-In: Ie744f118526f100d82a312c64f7c6fcf20773b6d Change-Id: Ie744f118526f100d82a312c64f7c6fcf20773b6d --- media/libstagefright/NuMediaExtractor.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/media/libstagefright/NuMediaExtractor.cpp b/media/libstagefright/NuMediaExtractor.cpp index c6385079dd..7c4855ba35 100644 --- a/media/libstagefright/NuMediaExtractor.cpp +++ b/media/libstagefright/NuMediaExtractor.cpp @@ -627,9 +627,11 @@ status_t NuMediaExtractor::appendVorbisNumPageSamples( numPageSamples = -1; } + // insert, including accounting for the space used. memcpy((uint8_t *)buffer->data() + mbuf->range_length(), &numPageSamples, sizeof(numPageSamples)); + buffer->setRange(buffer->offset(), buffer->size() + sizeof(numPageSamples)); uint32_t type; const void *data; @@ -678,6 +680,8 @@ status_t NuMediaExtractor::readSampleData(const sp &buffer) { ssize_t minIndex = fetchAllTrackSamples(); + buffer->setRange(0, 0); // start with an empty buffer + if (minIndex < 0) { return ERROR_END_OF_STREAM; } @@ -693,25 +697,25 @@ status_t NuMediaExtractor::readSampleData(const sp &buffer) { sampleSize += sizeof(int32_t); } + // capacity() is ok since we cleared out the buffer if (buffer->capacity() < sampleSize) { return -ENOMEM; } + const size_t srclen = it->mBuffer->range_length(); const uint8_t *src = (const uint8_t *)it->mBuffer->data() + it->mBuffer->range_offset(); - memcpy((uint8_t *)buffer->data(), src, it->mBuffer->range_length()); + memcpy((uint8_t *)buffer->data(), src, srclen); + buffer->setRange(0, srclen); status_t err = OK; if (info->mTrackFlags & kIsVorbis) { + // adjusts range when it inserts the extra bits err = appendVorbisNumPageSamples(it->mBuffer, buffer); } - if (err == OK) { - buffer->setRange(0, sampleSize); - } - return err; } From c2ff46dac4da3b0983ece233420ab9905cdae8c9 Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Thu, 6 Jul 2023 08:41:56 +0000 Subject: [PATCH 14/18] Fix Segv on unknown address error flagged by fuzzer test. The error is thrown when the destructor tries to free pointer memory. This is happening for cases where the pointer was not initialized. Initializing it to a default value fixes the error. Bug: 245135112 Test: Build mtp_host_property_fuzzer and run on the target device (cherry picked from commit 3afa6e80e8568fe63f893fa354bc79ef91d3dcc0) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:d44311374e41a26b28db56794c9a7890a13a6972) Merged-In: I255cd68b7641e96ac47ab81479b9b46b78c15580 Change-Id: I255cd68b7641e96ac47ab81479b9b46b78c15580 --- media/mtp/MtpProperty.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/media/mtp/MtpProperty.h b/media/mtp/MtpProperty.h index bfd5f7f59a..1eb8874af1 100644 --- a/media/mtp/MtpProperty.h +++ b/media/mtp/MtpProperty.h @@ -26,6 +26,9 @@ namespace android { class MtpDataPacket; struct MtpPropertyValue { + // pointer str initialized to NULL so that free operation + // is not called for pre-assigned value + MtpPropertyValue() : str (NULL) {} union { int8_t i8; uint8_t u8; From ca8a549e9a333f481a9692ab5903091e4ec58a6d Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Mon, 10 Jul 2023 08:53:42 +0000 Subject: [PATCH 15/18] Fix for heap buffer overflow issue flagged by fuzzer test. OOB write occurs when a value is assigned to a buffer index which is greater than the buffer size. Adding a check on buffer bounds fixes the issue. Similar checks have been added wherever applicable on other such methods of the class. Bug: 243463593 Test: Build mtp_packet_fuzzer and run on the target device (cherry picked from commit a669e34bb8e6f0f7b5d7a35144bd342271a24712) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:1401a723899766632363129265b30d433ac69c44) Merged-In: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b Change-Id: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b --- media/mtp/MtpPacket.cpp | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/media/mtp/MtpPacket.cpp b/media/mtp/MtpPacket.cpp index 3b298a9bf3..e4467bbfdc 100644 --- a/media/mtp/MtpPacket.cpp +++ b/media/mtp/MtpPacket.cpp @@ -92,24 +92,46 @@ void MtpPacket::copyFrom(const MtpPacket& src) { } uint16_t MtpPacket::getUInt16(int offset) const { - return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset]; + if ((unsigned long)(offset+2) <= mBufferSize) { + return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset]; + } + else { + ALOGE("offset for buffer read is greater than buffer size!"); + abort(); + } } uint32_t MtpPacket::getUInt32(int offset) const { - return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) | - ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset]; + if ((unsigned long)(offset+4) <= mBufferSize) { + return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) | + ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset]; + } + else { + ALOGE("offset for buffer read is greater than buffer size!"); + abort(); + } } void MtpPacket::putUInt16(int offset, uint16_t value) { - mBuffer[offset++] = (uint8_t)(value & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + if ((unsigned long)(offset+2) <= mBufferSize) { + mBuffer[offset++] = (uint8_t)(value & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + } + else { + ALOGE("offset for buffer write is greater than buffer size!"); + } } void MtpPacket::putUInt32(int offset, uint32_t value) { - mBuffer[offset++] = (uint8_t)(value & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF); + if ((unsigned long)(offset+4) <= mBufferSize) { + mBuffer[offset++] = (uint8_t)(value & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF); + } + else { + ALOGE("offset for buffer write is greater than buffer size!"); + } } uint16_t MtpPacket::getContainerCode() const { From 71733533e81e19e078fbd1671bd6afac6e4bfe99 Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Thu, 13 Jul 2023 09:19:08 +0000 Subject: [PATCH 16/18] Fix heap-use-after-free issue flagged by fuzzer test. A data member of class MtpFfsHandle is being accessed after the class object has been freed in the fuzzer. The method accessing the data member is running in a separate thread that gets detached from its parent. Using a conditional variable with an atomic int predicate in the close() function to ensure the detached thread's execution has completed before freeing the object fixes the issue without blocking the processing mid-way. Bug: 243381410 Test: Build mtp_handle_fuzzer and run on the target device (cherry picked from commit 50bf46a3f62136386548a9187a749936bda3ee8f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:73d89318a658ece5f337c5f9c1ec1149c52eb722) Merged-In: I41dde165a5eba151c958b81417d9e1065af1b411 Change-Id: I41dde165a5eba151c958b81417d9e1065af1b411 --- media/mtp/MtpFfsHandle.cpp | 14 ++++++++++++++ media/mtp/MtpFfsHandle.h | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/media/mtp/MtpFfsHandle.cpp b/media/mtp/MtpFfsHandle.cpp index bd6a6c679a..09eb96a00d 100644 --- a/media/mtp/MtpFfsHandle.cpp +++ b/media/mtp/MtpFfsHandle.cpp @@ -296,6 +296,10 @@ int MtpFfsHandle::start(bool ptp) { } void MtpFfsHandle::close() { + auto timeout = std::chrono::seconds(2); + std::unique_lock lk(m); + cv.wait_for(lk, timeout ,[this]{return child_threads==0;}); + io_destroy(mCtx); closeEndpoints(); closeConfig(); @@ -662,6 +666,11 @@ int MtpFfsHandle::sendEvent(mtp_event me) { char *temp = new char[me.length]; memcpy(temp, me.data, me.length); me.data = temp; + + std::unique_lock lk(m); + child_threads++; + lk.unlock(); + std::thread t([this, me]() { return this->doSendEvent(me); }); t.detach(); return 0; @@ -673,6 +682,11 @@ void MtpFfsHandle::doSendEvent(mtp_event me) { if (static_cast(ret) != length) PLOG(ERROR) << "Mtp error sending event thread!"; delete[] reinterpret_cast(me.data); + + std::unique_lock lk(m); + child_threads--; + lk.unlock(); + cv.notify_one(); } } // namespace android diff --git a/media/mtp/MtpFfsHandle.h b/media/mtp/MtpFfsHandle.h index fe343f74f6..ae78db2877 100644 --- a/media/mtp/MtpFfsHandle.h +++ b/media/mtp/MtpFfsHandle.h @@ -58,6 +58,10 @@ class MtpFfsHandle : public IMtpHandle { bool mCanceled; + std::mutex m; + std::condition_variable cv; + std::atomic child_threads{0}; + android::base::unique_fd mControl; // "in" from the host's perspective => sink for mtp server android::base::unique_fd mBulkIn; From b4827a209ccebe42c4a80e4fab3251cc77eb59ea Mon Sep 17 00:00:00 2001 From: Toni Heidenreich Date: Wed, 6 Sep 2023 12:49:33 +0000 Subject: [PATCH 17/18] httplive: fix use-after-free Implement a mutex to ensure secure multi-threaded access to the KeyedVector in MetaDataBase. Concurrent access by different threads can lead to accessing the wrong memory location due to potential changes in the vector Bug: 298057702 Test: HTTP Live Streaming test (cherry picked from https://partner-android-review.googlesource.com/q/commit:a2dfb31957a9d5358d0219a0eda7dcb5b0fff5fe) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:90fb4ca425444429ada6ce0de1c13d35829bc196) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3c1d9613ef64e01d2e81c4aa44c90dcd8ca958b9) Merged-In: I46b05c85d9c39f4ce549efc160c08a0646c9fd0a Change-Id: I46b05c85d9c39f4ce549efc160c08a0646c9fd0a --- media/libstagefright/foundation/MetaDataBase.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/media/libstagefright/foundation/MetaDataBase.cpp b/media/libstagefright/foundation/MetaDataBase.cpp index 4b439c60fc..bb323855a2 100644 --- a/media/libstagefright/foundation/MetaDataBase.cpp +++ b/media/libstagefright/foundation/MetaDataBase.cpp @@ -23,6 +23,8 @@ #include #include +#include + #include #include #include @@ -78,6 +80,7 @@ struct MetaDataBase::Rect { struct MetaDataBase::MetaDataInternal { + std::mutex mLock; KeyedVector mItems; }; @@ -102,10 +105,12 @@ MetaDataBase::~MetaDataBase() { } void MetaDataBase::clear() { + std::lock_guard guard(mInternalData->mLock); mInternalData->mItems.clear(); } bool MetaDataBase::remove(uint32_t key) { + std::lock_guard guard(mInternalData->mLock); ssize_t i = mInternalData->mItems.indexOfKey(key); if (i < 0) { @@ -252,6 +257,7 @@ bool MetaDataBase::setData( uint32_t key, uint32_t type, const void *data, size_t size) { bool overwrote_existing = true; + std::lock_guard guard(mInternalData->mLock); ssize_t i = mInternalData->mItems.indexOfKey(key); if (i < 0) { typed_data item; @@ -269,6 +275,7 @@ bool MetaDataBase::setData( bool MetaDataBase::findData(uint32_t key, uint32_t *type, const void **data, size_t *size) const { + std::lock_guard guard(mInternalData->mLock); ssize_t i = mInternalData->mItems.indexOfKey(key); if (i < 0) { @@ -283,6 +290,7 @@ bool MetaDataBase::findData(uint32_t key, uint32_t *type, } bool MetaDataBase::hasData(uint32_t key) const { + std::lock_guard guard(mInternalData->mLock); ssize_t i = mInternalData->mItems.indexOfKey(key); if (i < 0) { @@ -429,6 +437,7 @@ static void MakeFourCCString(uint32_t x, char *s) { String8 MetaDataBase::toString() const { String8 s; + std::lock_guard guard(mInternalData->mLock); for (int i = mInternalData->mItems.size(); --i >= 0;) { int32_t key = mInternalData->mItems.keyAt(i); char cc[5]; @@ -443,6 +452,7 @@ String8 MetaDataBase::toString() const { } void MetaDataBase::dumpToLog() const { + std::lock_guard guard(mInternalData->mLock); for (int i = mInternalData->mItems.size(); --i >= 0;) { int32_t key = mInternalData->mItems.keyAt(i); char cc[5]; @@ -455,6 +465,7 @@ void MetaDataBase::dumpToLog() const { #ifndef __ANDROID_VNDK__ status_t MetaDataBase::writeToParcel(Parcel &parcel) { status_t ret; + std::lock_guard guard(mInternalData->mLock); size_t numItems = mInternalData->mItems.size(); ret = parcel.writeUint32(uint32_t(numItems)); if (ret) { From 57a1c867fb86ee52a843482bb22e49503b28ef6a Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Mon, 12 Jun 2023 15:32:49 -0700 Subject: [PATCH 18/18] [RESTRICT AUTOMERGE] Update uid state based on capability field Use the passed capability field in the to prevent clients from recording in the background. To work around existing issues in the implementation, the approach is - if we don't hold the capability, simulate an onUidIdle. - if we hold the capability, to simulate an onUidActive and then the existing behavior (update the AM state). Only update behavior for apps targetSdk > 34. Bug: 268724205 Test: OboeTester recording silenced in background for all paths Test: OboeTester recording permitted after returning to foreground Test: AGSA works Test: atest AudioRecordTest (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:7aa76cedc006500e4db1e5084c77b6183d8bac35) Merged-In: Ida37fec306417b40006dfac5b5ed04f17418b7c8 Change-Id: Ida37fec306417b40006dfac5b5ed04f17418b7c8 --- .../service/AudioPolicyService.cpp | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/services/audiopolicy/service/AudioPolicyService.cpp b/services/audiopolicy/service/AudioPolicyService.cpp index fbd7614c08..28a2b31c46 100644 --- a/services/audiopolicy/service/AudioPolicyService.cpp +++ b/services/audiopolicy/service/AudioPolicyService.cpp @@ -21,9 +21,11 @@ #undef __STRICT_ANSI__ #define __STDINT_LIMITS #define __STDC_LIMIT_MACROS +#include #include #include +#include #include #include #include @@ -54,6 +56,37 @@ static const nsecs_t kAudioCommandTimeoutNs = seconds(3); // 3 seconds static const String16 sManageAudioPolicyPermission("android.permission.MANAGE_AUDIO_POLICY"); +namespace { +int getTargetSdkForPackageName(String16 packageName) { + const auto binder = defaultServiceManager()->checkService(String16{"package_native"}); + int targetSdk = -1; + if (binder != nullptr) { + const auto pm = interface_cast(binder); + if (pm != nullptr) { + const auto status = pm->getTargetSdkVersionForPackage(packageName, &targetSdk); + ALOGI("Capy check package %s, sdk %d", String8(packageName).string(), targetSdk); + return status.isOk() ? targetSdk : -1; + } + } + return targetSdk; +} + +bool doesUidTargetAtLeastU(uid_t uid) { + Vector packages; + PermissionController pc; + pc.getPackagesForUid(uid, packages); + constexpr int ANDROID_API_U = 34; + return packages.empty() || (getTargetSdkForPackageName(packages[0]) >= ANDROID_API_U); +} + +bool doesUidTargetAtLeastUCached(uid_t uid) { + static std::map cache; + const auto it = cache.find(uid); + return it == cache.end() ? (cache[uid] = doesUidTargetAtLeastU(uid)) : it->second; +} + + +} // anonymous // ---------------------------------------------------------------------------- AudioPolicyService::AudioPolicyService() @@ -1076,9 +1109,29 @@ void AudioPolicyService::UidPolicy::onUidIdle(uid_t uid, __unused bool disabled) void AudioPolicyService::UidPolicy::onUidStateChanged(uid_t uid, int32_t procState, int64_t procStateSeq __unused, - int32_t capability __unused) { + int32_t capability) { if (procState != ActivityManager::PROCESS_STATE_UNKNOWN) { - updateUid(&mCachedUids, uid, true, procState, true); + if (doesUidTargetAtLeastUCached(uid)) { + // See ActivityManager.java + constexpr int32_t PROCESS_CAPABILITY_FOREGROUND_MICROPHONE = 1 << 2; + if (capability & PROCESS_CAPABILITY_FOREGROUND_MICROPHONE) { + // The implementation relies on the assumption that this callback is preceded by + // onUidActive. This may not be the case if we have lost and then regained the + // capability, since we simulate onUidIdle below. So, we simulate onUidActive. + // In the typical case where we haven't regained capability, this is a no-op, since + // we should've been preceded by an onUidActive callback anyway. + updateUid(&mCachedUids, uid, true /* active */, + ActivityManager::PROCESS_STATE_UNKNOWN, true /* insert */); + updateUid(&mCachedUids, uid, true /* active */, procState, true /* insert */); + } else { + // If we have lost the capability (e.g. moving to background), treat as-if we have + // gotten onUidIdle. + updateUid(&mCachedUids, uid, false /* active */, + ActivityManager::PROCESS_STATE_UNKNOWN, true /* insert */); + } + } else { + updateUid(&mCachedUids, uid, true, procState, true); + } } }