From ce7ba619260d8e4a8b75fd999be629ef941dd9cc Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Mon, 10 Jul 2023 08:53:42 +0000 Subject: [PATCH 01/20] 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:56ae070c55debb8b2c691e296bded3d6e9f63518) 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 f069a83b5f..5faaac2026 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 { -- GitLab From 1deb1302cfcc4b9660baf822fc9177adaeac09a7 Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Thu, 13 Jul 2023 09:19:08 +0000 Subject: [PATCH 02/20] 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:e2c99e1e3a87368477f888f56944ec11c8d11a6e) 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 2ffd7759e0..ef8c9aa789 100644 --- a/media/mtp/MtpFfsHandle.cpp +++ b/media/mtp/MtpFfsHandle.cpp @@ -297,6 +297,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(); @@ -669,6 +673,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; @@ -680,6 +689,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 e552e03bec..51cdef0953 100644 --- a/media/mtp/MtpFfsHandle.h +++ b/media/mtp/MtpFfsHandle.h @@ -60,6 +60,10 @@ protected: bool mCanceled; bool mBatchCancel; + 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; -- GitLab From 1a233e80bc311213504544a31144576e46dab298 Mon Sep 17 00:00:00 2001 From: Venkatarama Avadhani Date: Fri, 11 Aug 2023 15:19:24 +0000 Subject: [PATCH 03/20] Initialise VPS buffer to NULL in constructor Missing initialisation of this pointer could lead to an incorrect free if the ARTWriter object is cleared immeddiately after the constructor call. Bug: 287298721 Test: rtp_writer_fuzzer (cherry picked from https://partner-android-review.googlesource.com/q/commit:2710696b001f2e95586151c1ee337a4e3c4da48a) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:900195c1d3589c7cbf9e116f61bebaefc0519101) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0efe2b4d6b739650039c2cab176ef11d5f5ac49c) Merged-In: I08eacd7a0201bc9a41b821e20cae916d8870147a Change-Id: I08eacd7a0201bc9a41b821e20cae916d8870147a --- media/libstagefright/rtsp/ARTPWriter.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/media/libstagefright/rtsp/ARTPWriter.cpp b/media/libstagefright/rtsp/ARTPWriter.cpp index 41f2d67bb7..bc8341078d 100644 --- a/media/libstagefright/rtsp/ARTPWriter.cpp +++ b/media/libstagefright/rtsp/ARTPWriter.cpp @@ -105,6 +105,7 @@ ARTPWriter::ARTPWriter(int fd) mRTCPAddr = mRTPAddr; mRTCPAddr.sin_port = htons(ntohs(mRTPAddr.sin_port) | 1); + mVPSBuf = NULL; mSPSBuf = NULL; mPPSBuf = NULL; -- GitLab From dc0af854d4dd6c5bda691f9199198f1928e5b679 Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Wed, 9 Aug 2023 16:21:48 -0700 Subject: [PATCH 04/20] Condition background record restriction on Sdk To prevent breaking existing apps, modify the checks around when an app should have its recording silenced to retain prior behavior unless an app has targetSdk U or greater. Test: oboetester conditionally restricted based on targetSdk level Bug: 268724205 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:dc4f375d570965775634d90856719b812aee9865) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:733ab192011df7afc48a7f4d88ed1a960e264608) Merged-In: I42b6cbca60db6ce1a073254239b48e9104c4ebfb Change-Id: I42b6cbca60db6ce1a073254239b48e9104c4ebfb --- .../service/AudioPolicyService.cpp | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/services/audiopolicy/service/AudioPolicyService.cpp b/services/audiopolicy/service/AudioPolicyService.cpp index df0c59f6fa..889ae960ca 100644 --- a/services/audiopolicy/service/AudioPolicyService.cpp +++ b/services/audiopolicy/service/AudioPolicyService.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -202,6 +203,26 @@ static void destroyAudioPolicyManager(AudioPolicyInterface *interface) { delete interface; } + +namespace { +int getTargetSdkForPackageName(std::string_view 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( + String16{packageName.data(), packageName.size()}, &targetSdk); + return status.isOk() ? targetSdk : -1; + } + } + return targetSdk; +} + +bool doesPackageTargetAtLeastU(std::string_view packageName) { + return getTargetSdkForPackageName(packageName) >= __ANDROID_API_U__; +} +} // anonymous // ---------------------------------------------------------------------------- AudioPolicyService::AudioPolicyService() @@ -1913,10 +1934,14 @@ void AudioPolicyService::OpRecordAudioMonitor::onFirstRef() checkOp(); mOpCallback = new RecordAudioOpCallback(this); ALOGV("start watching op %d for %s", mAppOp, mAttributionSource.toString().c_str()); + int flags = doesPackageTargetAtLeastU( + mAttributionSource.packageName.value_or("")) ? + AppOpsManager::WATCH_FOREGROUND_CHANGES : 0; // TODO: We need to always watch AppOpsManager::OP_RECORD_AUDIO too // since it controls the mic permission for legacy apps. mAppOpsManager.startWatchingMode(mAppOp, VALUE_OR_FATAL(aidl2legacy_string_view_String16( mAttributionSource.packageName.value_or(""))), + flags, mOpCallback); } -- GitLab From 7784a011a7e49509f03051272210d8896b1be1e1 Mon Sep 17 00:00:00 2001 From: guochuang Date: Fri, 15 Sep 2023 17:45:44 +0800 Subject: [PATCH 05/20] NdkMedia: fix android.mediav2.cts.CodecEncoderSurfaceTest failed. use Abuffer.base instead of data. Bug: 300861053 Test: run cts -m CtsMediaV2TestCases -t android.mediav2.cts.CodecEncoderSurfaceTest Signed-off-by: guochuang (cherry picked from https://android-review.googlesource.com/q/commit:184293b550dec1bcb7b8ba5f2f5342c355f5b4dc) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:95f411fbda480f986811e6f8010f0a2fd91fd206) Merged-In: I76ef2c404f8264843973e1af9be1b23fc1e6c3af Change-Id: I76ef2c404f8264843973e1af9be1b23fc1e6c3af --- media/ndk/NdkMediaCodec.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/media/ndk/NdkMediaCodec.cpp b/media/ndk/NdkMediaCodec.cpp index b230df5179..2fb5728d2e 100644 --- a/media/ndk/NdkMediaCodec.cpp +++ b/media/ndk/NdkMediaCodec.cpp @@ -672,7 +672,7 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abuf->capacity(); } - return abuf->data(); + return abuf->base(); } android::Vector > abufs; @@ -689,7 +689,7 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abufs[idx]->capacity(); } - return abufs[idx]->data(); + return abufs[idx]->base(); } ALOGE("couldn't get input buffers"); return NULL; @@ -707,7 +707,7 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out if (out_size != NULL) { *out_size = abuf->capacity(); } - return abuf->data(); + return abuf->base(); } android::Vector > abufs; @@ -720,7 +720,7 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out if (out_size != NULL) { *out_size = abufs[idx]->capacity(); } - return abufs[idx]->data(); + return abufs[idx]->base(); } ALOGE("couldn't get output buffers"); return NULL; -- GitLab From f980a9bee224396e1c895b62b6b190fc932edb6a Mon Sep 17 00:00:00 2001 From: Sohail Nagaraj Date: Thu, 24 Aug 2023 07:39:43 +0000 Subject: [PATCH 06/20] 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: 278166920 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:300e148b8e80387fa5c9a69feb38f8af53541d19) Merged-In: Id35ba181185bc93d9f268309a1514c5a18166e12 Change-Id: Id35ba181185bc93d9f268309a1514c5a18166e12 --- media/module/foundation/MetaDataBase.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/media/module/foundation/MetaDataBase.cpp b/media/module/foundation/MetaDataBase.cpp index 33707482c9..46a600a9d9 100644 --- a/media/module/foundation/MetaDataBase.cpp +++ b/media/module/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 { #if defined(__ANDROID__) && !defined(__ANDROID_VNDK__) && !defined(__ANDROID_APEX__) 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) { -- GitLab From b64110db98ca71cd10e5e2b887eedcfc106c6639 Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Thu, 19 Oct 2023 18:39:26 +0000 Subject: [PATCH 07/20] Revert "NdkMedia: fix android.mediav2.cts.CodecEncoderSurfaceTest failed." This reverts commit 95f411fbda480f986811e6f8010f0a2fd91fd206. Reason for revert: This breaks app compatibility (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:883e38b694a6a2a51d34ea145607b62b0bdb316f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a2118fa4988f1d8135894800de122296e146a157) Merged-In: I94d7034907d5befc0aabf7bf2b490d1fc86c69af Change-Id: I94d7034907d5befc0aabf7bf2b490d1fc86c69af --- media/ndk/NdkMediaCodec.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/media/ndk/NdkMediaCodec.cpp b/media/ndk/NdkMediaCodec.cpp index 2fb5728d2e..b230df5179 100644 --- a/media/ndk/NdkMediaCodec.cpp +++ b/media/ndk/NdkMediaCodec.cpp @@ -672,7 +672,7 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abuf->capacity(); } - return abuf->base(); + return abuf->data(); } android::Vector > abufs; @@ -689,7 +689,7 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abufs[idx]->capacity(); } - return abufs[idx]->base(); + return abufs[idx]->data(); } ALOGE("couldn't get input buffers"); return NULL; @@ -707,7 +707,7 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out if (out_size != NULL) { *out_size = abuf->capacity(); } - return abuf->base(); + return abuf->data(); } android::Vector > abufs; @@ -720,7 +720,7 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out if (out_size != NULL) { *out_size = abufs[idx]->capacity(); } - return abufs[idx]->base(); + return abufs[idx]->data(); } ALOGE("couldn't get output buffers"); return NULL; -- GitLab From 4f9fa2df202d2731451f8f76bb3b47ed6697fc7f Mon Sep 17 00:00:00 2001 From: Harish Mahendrakar Date: Mon, 28 Aug 2023 17:35:56 +0000 Subject: [PATCH 08/20] Codec2BufferUtils: Use cropped dimensions in RGB to YUV conversion Bug: 283099444 Test: poc in the bug (cherry picked from https://partner-android-review.googlesource.com/q/commit:3875b858a347e25db94574e6362798a849bf9ebd) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:4eba80f6698cb2d7aa48ea4f7728dbdf11f29fd3) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:aaddf2a49d758a55319b88c8331d5b1209858c41) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3ee0378ac5b39fe57fb91f0a8113e0fd18ec1822) Merged-In: I42c71616c9d50f61c92f461f6a91f5addb1d724a Change-Id: I42c71616c9d50f61c92f461f6a91f5addb1d724a --- media/codec2/sfplugin/utils/Codec2BufferUtils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/media/codec2/sfplugin/utils/Codec2BufferUtils.cpp b/media/codec2/sfplugin/utils/Codec2BufferUtils.cpp index 9004bcf8a7..261fd0577c 100644 --- a/media/codec2/sfplugin/utils/Codec2BufferUtils.cpp +++ b/media/codec2/sfplugin/utils/Codec2BufferUtils.cpp @@ -621,8 +621,8 @@ status_t ConvertRGBToPlanarYUV( uint8_t maxLvlChroma = colorRange == C2Color::RANGE_FULL ? 255 : 240; #define CLIP3(min,v,max) (((v) < (min)) ? (min) : (((max) > (v)) ? (v) : (max))) - for (size_t y = 0; y < src.height(); ++y) { - for (size_t x = 0; x < src.width(); ++x) { + for (size_t y = 0; y < src.crop().height; ++y) { + for (size_t x = 0; x < src.crop().width; ++x) { uint8_t r = *pRed; uint8_t g = *pGreen; uint8_t b = *pBlue; -- GitLab From a424120a625cb5ad08b01c43cac75d65957b1889 Mon Sep 17 00:00:00 2001 From: Songyue Han Date: Tue, 3 Oct 2023 22:40:14 +0000 Subject: [PATCH 09/20] Fix convertYUV420Planar16ToY410 overflow issue for unsupported cropwidth. Bug: 300476626 Test: color_conversion_fuzzer (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:de2ad0fad97d6d97d1e01f0e8d8309536eb268b4) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:745ab99f7343bc236b88b9d63cd7b06ab192f9e9) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:aa8298ec8eb903e1e3dd915fa24f32e1aea1f76c) Merged-In: I8631426188af3c5f9b6c1ff6a0039254c252f733 Change-Id: I8631426188af3c5f9b6c1ff6a0039254c252f733 --- media/libstagefright/colorconversion/ColorConverter.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/media/libstagefright/colorconversion/ColorConverter.cpp b/media/libstagefright/colorconversion/ColorConverter.cpp index f91a8b29c2..854cc08fd6 100644 --- a/media/libstagefright/colorconversion/ColorConverter.cpp +++ b/media/libstagefright/colorconversion/ColorConverter.cpp @@ -1549,7 +1549,8 @@ status_t ColorConverter::convertYUV420Planar16ToY410( uint32_t u01, v01, y01, y23, y45, y67, uv0, uv1; size_t x = 0; - for (; x < src.cropWidth() - 3; x += 4) { + // x % 4 is always 0 so x + 3 will never overflow. + for (; x + 3 < src.cropWidth(); x += 4) { u01 = *((uint32_t*)ptr_u); ptr_u += 2; v01 = *((uint32_t*)ptr_v); ptr_v += 2; -- GitLab From 2ca6c27dc0336fd98f47cfb96dc514efa98e8864 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Gupta Date: Tue, 21 Nov 2023 08:48:43 +0530 Subject: [PATCH 10/20] Update mtp packet buffer Currently, the buffer size is not changed when the packet size is increased. Ideally, the buffer size should be larger than the packet size. In our case, when the packet size is increased, we must reallocate the buffer of MTP packet. Bug: 300007708 Test: build and flash the device. Check MTP works Test: run fuzzer locally (cherry picked from commit e1494a2d8e7eee25d7ea5469be43740e97294c99) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5c0f99beb6fa5ff920caf5b0d06aaebc8e9eab24) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:38852806102bb7e9d46f4b0de8a3b4918d625ad4) Merged-In: I98398a9e15962e6d5f08445ee7b17f5d61a3a528 Change-Id: I98398a9e15962e6d5f08445ee7b17f5d61a3a528 --- media/mtp/MtpPacket.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/media/mtp/MtpPacket.cpp b/media/mtp/MtpPacket.cpp index 5faaac2026..634aa46dd9 100644 --- a/media/mtp/MtpPacket.cpp +++ b/media/mtp/MtpPacket.cpp @@ -168,8 +168,10 @@ void MtpPacket::setParameter(int index, uint32_t value) { return; } int offset = MTP_CONTAINER_PARAMETER_OFFSET + (index - 1) * sizeof(uint32_t); - if (mPacketSize < offset + sizeof(uint32_t)) + if (mPacketSize < offset + sizeof(uint32_t)) { mPacketSize = offset + sizeof(uint32_t); + allocate(mPacketSize); + } putUInt32(offset, value); } -- GitLab From d17ed6b7609eef0d141b95a8672baa85a283edd3 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Fri, 13 Oct 2023 18:13:43 +0200 Subject: [PATCH 11/20] Audio policy: anonymize Bluetooth MAC addresses Make sure APIs returning audio device descriptors from the native audioserver anonymize the Bluetooth MAC addresses because those are considered privacy sensitive. Only expose the full MAC address to system and apps with BLUETOOTH_CONNECT permission. APIs modified: listAudioPorts, listAudioPatches, getAudioPort APIs that can only be called from system server or only convey port IDs are not modified. Bug: 285588444 Test: atest AudioManagerTest Test: atest RoutingTest Test: atest AudioCommunicationDeviceTest (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:8f9a40ad4610cf59bd0a7e3fe278ca5ad4053fac) Merged-In: Ia6bac184f5f39ed9d538f762ebb89bcceb44ae50 Change-Id: Ia6bac184f5f39ed9d538f762ebb89bcceb44ae50 --- .../AidlConversionCppNdk.cpp | 20 +++++++- .../audio_aidl_legacy_conversion_tests.cpp | 20 ++++++++ media/utils/ServiceUtilities.cpp | 43 +++++++++++++++++ .../include/mediautils/ServiceUtilities.h | 4 ++ .../service/AudioPolicyInterfaceImpl.cpp | 46 +++++++++++++++++++ 5 files changed, 131 insertions(+), 2 deletions(-) diff --git a/media/audioaidlconversion/AidlConversionCppNdk.cpp b/media/audioaidlconversion/AidlConversionCppNdk.cpp index 3b06245e0b..f13cfa3aab 100644 --- a/media/audioaidlconversion/AidlConversionCppNdk.cpp +++ b/media/audioaidlconversion/AidlConversionCppNdk.cpp @@ -1052,6 +1052,13 @@ AudioDeviceAddress::Tag suggestDeviceAddressTag(const AudioDeviceDescription& de if (mac.size() != 6) return BAD_VALUE; snprintf(addressBuffer, AUDIO_DEVICE_MAX_ADDRESS_LEN, "%02X:%02X:%02X:%02X:%02X:%02X", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); + // special case for anonymized mac address: + // change anonymized bytes back from FD:FF:FF:FF to XX:XX:XX:XX + std::string address(addressBuffer); + if (address.compare(0, strlen("FD:FF:FF:FF"), "FD:FF:FF:FF") == 0) { + address.replace(0, strlen("FD:FF:FF:FF"), "XX:XX:XX:XX"); + } + strcpy(addressBuffer, address.c_str()); } break; case Tag::ipv4: { const std::vector& ipv4 = aidl.address.get(); @@ -1108,11 +1115,20 @@ legacy2aidl_audio_device_AudioDevice( if (!legacyAddress.empty()) { switch (suggestDeviceAddressTag(aidl.type)) { case Tag::mac: { + // special case for anonymized mac address: + // change anonymized bytes so that they can be scanned as HEX bytes + // Use '01' for LSB bits 0 and 1 as Bluetooth MAC addresses are never multicast + // and universaly administered + std::string address = legacyAddress; + if (address.compare(0, strlen("XX:XX:XX:XX"), "XX:XX:XX:XX") == 0) { + address.replace(0, strlen("XX:XX:XX:XX"), "FD:FF:FF:FF"); + } + std::vector mac(6); - int status = sscanf(legacyAddress.c_str(), "%hhX:%hhX:%hhX:%hhX:%hhX:%hhX", + int status = sscanf(address.c_str(), "%hhX:%hhX:%hhX:%hhX:%hhX:%hhX", &mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5]); if (status != mac.size()) { - ALOGE("%s: malformed MAC address: \"%s\"", __func__, legacyAddress.c_str()); + ALOGE("%s: malformed MAC address: \"%s\"", __func__, address.c_str()); return unexpected(BAD_VALUE); } aidl.address = AudioDeviceAddress::make(std::move(mac)); diff --git a/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp b/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp index 0d12f9d9e1..67473cec5e 100644 --- a/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp +++ b/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp @@ -475,8 +475,28 @@ INSTANTIATE_TEST_SUITE_P( AudioDeviceAddress::make( std::vector{1, 2})))); +TEST(AnonymizedBluetoothAddressRoundTripTest, Legacy2Aidl2Legacy) { + const std::vector sAnonymizedAidlAddress = + std::vector{0xFD, 0xFF, 0xFF, 0xFF, 0xAB, 0xCD}; + const std::string sAnonymizedLegacyAddress = std::string("XX:XX:XX:XX:AB:CD"); + auto device = legacy2aidl_audio_device_AudioDevice(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP, + sAnonymizedLegacyAddress); + ASSERT_TRUE(device.ok()); + ASSERT_EQ(AudioDeviceAddress::Tag::mac, device.value().address.getTag()); + ASSERT_EQ(sAnonymizedAidlAddress, device.value().address.get()); + + audio_devices_t legacyType; + std::string legacyAddress; + status_t status = + aidl2legacy_AudioDevice_audio_device(device.value(), &legacyType, &legacyAddress); + ASSERT_EQ(OK, status); + EXPECT_EQ(legacyType, AUDIO_DEVICE_OUT_BLUETOOTH_A2DP); + EXPECT_EQ(sAnonymizedLegacyAddress, legacyAddress); +} + class AudioFormatDescriptionRoundTripTest : public testing::TestWithParam { }; + TEST_P(AudioFormatDescriptionRoundTripTest, Aidl2Legacy2Aidl) { const auto initial = GetParam(); auto conv = aidl2legacy_AudioFormatDescription_audio_format_t(initial); diff --git a/media/utils/ServiceUtilities.cpp b/media/utils/ServiceUtilities.cpp index 83b84e392c..4ee45c795e 100644 --- a/media/utils/ServiceUtilities.cpp +++ b/media/utils/ServiceUtilities.cpp @@ -46,6 +46,7 @@ static const String16 sAndroidPermissionRecordAudio("android.permission.RECORD_A static const String16 sModifyPhoneState("android.permission.MODIFY_PHONE_STATE"); static const String16 sModifyAudioRouting("android.permission.MODIFY_AUDIO_ROUTING"); static const String16 sCallAudioInterception("android.permission.CALL_AUDIO_INTERCEPTION"); +static const String16 sAndroidPermissionBluetoothConnect("android.permission.BLUETOOTH_CONNECT"); static String16 resolveCallingPackage(PermissionController& permissionController, const std::optional opPackageName, uid_t uid) { @@ -374,6 +375,48 @@ status_t checkIMemory(const sp& iMemory) return NO_ERROR; } +/** + * Determines if the MAC address in Bluetooth device descriptors returned by APIs of + * a native audio service (audio flinger, audio policy) must be anonymized. + * MAC addresses returned to system server or apps with BLUETOOTH_CONNECT permission + * are not anonymized. + * + * @param attributionSource The attribution source of the calling app. + * @param caller string identifying the caller for logging. + * @return true if the MAC addresses must be anonymized, false otherwise. + */ +bool mustAnonymizeBluetoothAddress( + const AttributionSourceState& attributionSource, const String16& caller) { + uid_t uid = VALUE_OR_FATAL(aidl2legacy_int32_t_uid_t(attributionSource.uid)); + if (isAudioServerOrSystemServerUid(uid)) { + return false; + } + const std::optional resolvedAttributionSource = + resolveAttributionSource(attributionSource); + if (!resolvedAttributionSource.has_value()) { + return true; + } + permission::PermissionChecker permissionChecker; + return permissionChecker.checkPermissionForPreflightFromDatasource( + sAndroidPermissionBluetoothConnect, resolvedAttributionSource.value(), caller, + AppOpsManager::OP_BLUETOOTH_CONNECT) + != permission::PermissionChecker::PERMISSION_GRANTED; +} + +/** + * Modifies the passed MAC address string in place for consumption by unprivileged clients. + * the string is assumed to have a valid MAC address format. + * the anonymzation must be kept in sync with toAnonymizedAddress() in BluetoothUtils.java + * + * @param address input/output the char string contining the MAC address to anonymize. + */ +void anonymizeBluetoothAddress(char *address) { + if (address == nullptr || strlen(address) != strlen("AA:BB:CC:DD:EE:FF")) { + return; + } + memcpy(address, "XX:XX:XX:XX", strlen("XX:XX:XX:XX")); +} + sp MediaPackageManager::retrievePackageManager() { const sp sm = defaultServiceManager(); if (sm == nullptr) { diff --git a/media/utils/include/mediautils/ServiceUtilities.h b/media/utils/include/mediautils/ServiceUtilities.h index 3d7981ae97..0b3a3f96cd 100644 --- a/media/utils/include/mediautils/ServiceUtilities.h +++ b/media/utils/include/mediautils/ServiceUtilities.h @@ -108,6 +108,10 @@ bool modifyPhoneStateAllowed(const AttributionSourceState& attributionSource); bool bypassInterruptionPolicyAllowed(const AttributionSourceState& attributionSource); bool callAudioInterceptionAllowed(const AttributionSourceState& attributionSource); void purgePermissionCache(); +bool mustAnonymizeBluetoothAddress( + const AttributionSourceState& attributionSource, const String16& caller); +void anonymizeBluetoothAddress(char *address); + int32_t getOpForSource(audio_source_t source); AttributionSourceState getCallingAttributionSource(); diff --git a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp index 2e7b3ffb84..721ca1a26a 100644 --- a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp +++ b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp @@ -1528,6 +1528,19 @@ Status AudioPolicyService::isDirectOutputSupported( return Status::ok(); } +template +void anonymizePortBluetoothAddress(Port *port) { + if (port->type != AUDIO_PORT_TYPE_DEVICE) { + return; + } + if (!(audio_is_a2dp_device(port->ext.device.type) + || audio_is_ble_device(port->ext.device.type) + || audio_is_bluetooth_sco_device(port->ext.device.type) + || audio_is_hearing_aid_out_device(port->ext.device.type))) { + return; + } + anonymizeBluetoothAddress(port->ext.device.address); +} Status AudioPolicyService::listAudioPorts(media::AudioPortRole roleAidl, media::AudioPortType typeAidl, Int* count, @@ -1550,10 +1563,20 @@ Status AudioPolicyService::listAudioPorts(media::AudioPortRole roleAidl, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } + + const AttributionSourceState attributionSource = getCallingAttributionSource(); + AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( mAudioPolicyManager->listAudioPorts(role, type, &num_ports, ports.get(), &generation))); numPortsReq = std::min(numPortsReq, num_ports); + + if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { + for (size_t i = 0; i < numPortsReq; ++i) { + anonymizePortBluetoothAddress(&ports[i]); + } + } + RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( convertRange(ports.get(), ports.get() + numPortsReq, std::back_inserter(*portsAidl), legacy2aidl_audio_port_v7_AudioPortFw))); @@ -1580,8 +1603,16 @@ Status AudioPolicyService::getAudioPort(int portId, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } + + const AttributionSourceState attributionSource = getCallingAttributionSource(); + AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT(mAudioPolicyManager->getAudioPort(&port))); + + if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { + anonymizePortBluetoothAddress(&port); + } + *_aidl_return = VALUE_OR_RETURN_BINDER_STATUS(legacy2aidl_audio_port_v7_AudioPortFw(port)); return Status::ok(); } @@ -1643,10 +1674,25 @@ Status AudioPolicyService::listAudioPatches(Int* count, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } + + const AttributionSourceState attributionSource = getCallingAttributionSource(); + AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( mAudioPolicyManager->listAudioPatches(&num_patches, patches.get(), &generation))); numPatchesReq = std::min(numPatchesReq, num_patches); + + if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { + for (size_t i = 0; i < numPatchesReq; ++i) { + for (size_t j = 0; j < patches[i].num_sources; ++j) { + anonymizePortBluetoothAddress(&patches[i].sources[j]); + } + for (size_t j = 0; j < patches[i].num_sinks; ++j) { + anonymizePortBluetoothAddress(&patches[i].sinks[j]); + } + } + } + RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( convertRange(patches.get(), patches.get() + numPatchesReq, std::back_inserter(*patchesAidl), legacy2aidl_audio_patch_AudioPatchFw))); -- GitLab From 6a4f0a82bbf3fba44ab79e9380e0428123f3e668 Mon Sep 17 00:00:00 2001 From: Haripriya Deshmukh Date: Tue, 19 Sep 2023 20:42:45 +0000 Subject: [PATCH 12/20] Validate OMX Params for VPx encoders Bug: 273936274 Bug: 273937171 Bug: 273937136 Bug: 273936553 Bug: 273936601 Test: POC in bug descriptions (cherry picked from https://partner-android-review.googlesource.com/q/commit:022086b76536cd2e19a44053271190bdf6e181f7) (cherry picked from commit 0e4ca1cb5c16af8f1dfb0ae41941c16c104d38e8) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:90641b2799fd3940cdf0bf8a73b2f76839e651a6) Merged-In: I9bb17112d9f0217b6af0343afecc9c943453b757 Change-Id: I9bb17112d9f0217b6af0343afecc9c943453b757 --- media/libstagefright/codecs/on2/enc/SoftVP8Encoder.cpp | 10 ++++++++++ media/libstagefright/codecs/on2/enc/SoftVP9Encoder.cpp | 10 ++++++++++ media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp | 9 +++++++++ 3 files changed, 29 insertions(+) diff --git a/media/libstagefright/codecs/on2/enc/SoftVP8Encoder.cpp b/media/libstagefright/codecs/on2/enc/SoftVP8Encoder.cpp index 04737a9ccf..9198b7c327 100644 --- a/media/libstagefright/codecs/on2/enc/SoftVP8Encoder.cpp +++ b/media/libstagefright/codecs/on2/enc/SoftVP8Encoder.cpp @@ -120,6 +120,11 @@ OMX_ERRORTYPE SoftVP8Encoder::internalSetParameter(OMX_INDEXTYPE index, OMX_ERRORTYPE SoftVP8Encoder::internalGetVp8Params( OMX_VIDEO_PARAM_VP8TYPE* vp8Params) { + if (!isValidOMXParam(vp8Params)) { + android_errorWriteLog(0x534e4554, "273936274"); + return OMX_ErrorBadParameter; + } + if (vp8Params->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } @@ -133,6 +138,11 @@ OMX_ERRORTYPE SoftVP8Encoder::internalGetVp8Params( OMX_ERRORTYPE SoftVP8Encoder::internalSetVp8Params( const OMX_VIDEO_PARAM_VP8TYPE* vp8Params) { + if (!isValidOMXParam(vp8Params)) { + android_errorWriteLog(0x534e4554, "273937171"); + return OMX_ErrorBadParameter; + } + if (vp8Params->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } diff --git a/media/libstagefright/codecs/on2/enc/SoftVP9Encoder.cpp b/media/libstagefright/codecs/on2/enc/SoftVP9Encoder.cpp index 1ea1c85f76..f8495c2da4 100644 --- a/media/libstagefright/codecs/on2/enc/SoftVP9Encoder.cpp +++ b/media/libstagefright/codecs/on2/enc/SoftVP9Encoder.cpp @@ -119,6 +119,11 @@ OMX_ERRORTYPE SoftVP9Encoder::internalSetParameter( OMX_ERRORTYPE SoftVP9Encoder::internalGetVp9Params( OMX_VIDEO_PARAM_VP9TYPE *vp9Params) { + if (!isValidOMXParam(vp9Params)) { + android_errorWriteLog(0x534e4554, "273936553"); + return OMX_ErrorBadParameter; + } + if (vp9Params->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } @@ -133,6 +138,11 @@ OMX_ERRORTYPE SoftVP9Encoder::internalGetVp9Params( OMX_ERRORTYPE SoftVP9Encoder::internalSetVp9Params( const OMX_VIDEO_PARAM_VP9TYPE *vp9Params) { + if (!isValidOMXParam(vp9Params)) { + android_errorWriteLog(0x534e4554, "273937136"); + return OMX_ErrorBadParameter; + } + if (vp9Params->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } diff --git a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp index e9b4341458..cbedb723ed 100644 --- a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp +++ b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp @@ -485,6 +485,11 @@ OMX_ERRORTYPE SoftVPXEncoder::internalSetBitrateParams( OMX_ERRORTYPE SoftVPXEncoder::internalGetAndroidVpxParams( OMX_VIDEO_PARAM_ANDROID_VP8ENCODERTYPE *vpxAndroidParams) { + if (!isValidOMXParam(vpxAndroidParams)) { + android_errorWriteLog(0x534e4554, "273936601"); + return OMX_ErrorBadParameter; + } + if (vpxAndroidParams->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } @@ -501,6 +506,10 @@ OMX_ERRORTYPE SoftVPXEncoder::internalGetAndroidVpxParams( OMX_ERRORTYPE SoftVPXEncoder::internalSetAndroidVpxParams( const OMX_VIDEO_PARAM_ANDROID_VP8ENCODERTYPE *vpxAndroidParams) { + if (!isValidOMXParam(vpxAndroidParams)) { + android_errorWriteLog(0x534e4554, "273937551"); + return OMX_ErrorBadParameter; + } if (vpxAndroidParams->nPortIndex != kOutputPortIndex) { return OMX_ErrorUnsupportedIndex; } -- GitLab From 23a035af9be9f4ec6fe16a5fb9710d19ef669eec Mon Sep 17 00:00:00 2001 From: Harish Mahendrakar Date: Mon, 30 Oct 2023 20:38:56 +0000 Subject: [PATCH 13/20] SoftVideoDecodeOMXComponent: validate OMX params for dynamic HDR Bug: 273935108 Bug: 281065553 (cherry picked from https://partner-android-review.googlesource.com/q/commit:b2c67bdcf57149a5e19a04466205266dc543fd86) (cherry picked from commit a542f2c50700ca6df93e966fe8d4c468e1a15d9a) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:80e0acc096d201e80a1b65af944b1e47c9dd6f7b) Merged-In: I707745594a9196d8d85d4c4bb498eba3c6198b42 Change-Id: I707745594a9196d8d85d4c4bb498eba3c6198b42 --- media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp b/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp index e853da9763..418302389d 100644 --- a/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp +++ b/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp @@ -616,6 +616,10 @@ OMX_ERRORTYPE SoftVideoDecoderOMXComponent::getConfig( DescribeHDR10PlusInfoParams* outParams = (DescribeHDR10PlusInfoParams *)params; + if (!isValidOMXParam(outParams)) { + return OMX_ErrorBadParameter; + } + outParams->nParamSizeUsed = info->size(); // If the buffer provided by the client does not have enough @@ -694,6 +698,10 @@ OMX_ERRORTYPE SoftVideoDecoderOMXComponent::internalSetConfig( const DescribeHDR10PlusInfoParams* inParams = (DescribeHDR10PlusInfoParams *)params; + if (!isValidOMXParam(inParams)) { + return OMX_ErrorBadParameter; + } + if (*frameConfig) { // This is a request to append to the current frame config set. // For now, we only support kDescribeHdr10PlusInfoIndex, which -- GitLab From 3bedd18fd3f4465162666592a2850da9f4c0ff48 Mon Sep 17 00:00:00 2001 From: Haripriya Deshmukh Date: Tue, 5 Dec 2023 18:32:38 +0000 Subject: [PATCH 14/20] Fix out of bounds read and write in onQueueFilled in outQueue Bug: 276442130 Test: POC in bug descriptions (cherry picked from https://partner-android-review.googlesource.com/q/commit:7aef41e59412e2f95bab5de7e33f5f04bb808643) (cherry picked from commit 8f4cfda9fc75f1e9ba3b6dee3fbffda4b6111d64) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:208e430bc6380fafafca8041b239f835263a9d47) Merged-In: Ic230d10048193a785f185dc6a7de6f455f9318c1 Change-Id: Ic230d10048193a785f185dc6a7de6f455f9318c1 --- media/libstagefright/codecs/m4v_h263/dec/SoftMPEG4.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/media/libstagefright/codecs/m4v_h263/dec/SoftMPEG4.cpp b/media/libstagefright/codecs/m4v_h263/dec/SoftMPEG4.cpp index a4b3e2f434..14c3fa0875 100644 --- a/media/libstagefright/codecs/m4v_h263/dec/SoftMPEG4.cpp +++ b/media/libstagefright/codecs/m4v_h263/dec/SoftMPEG4.cpp @@ -312,8 +312,11 @@ void SoftMPEG4::onQueueFilled(OMX_U32 /* portIndex */) { outHeader->nFilledLen = frameSize; List::iterator it = outQueue.begin(); - while ((*it)->mHeader != outHeader) { - ++it; + while (it != outQueue.end() && (*it)->mHeader != outHeader) { + ++it; + } + if (it == outQueue.end()) { + return; } BufferInfo *outInfo = *it; -- GitLab From ab161a6327612a24df856120df4a44e3475be326 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Fri, 15 Mar 2024 16:01:21 +0000 Subject: [PATCH 15/20] RESTRICT AUTOMERGE - Revert "Audio policy: anonymize Bluetooth MAC addresses" Revert submission 25188138-anon_bt_address_udc_dev Reason for revert: b/329515274 Reverted changes: /q/submissionid:25188138-anon_bt_address_udc_dev (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:f580abdb79e5df889af8cbe454048e39a3ee9996) Merged-In: I8224b1a3386f25bb7fbe266ed2ce5b7db2093f40 Change-Id: I8224b1a3386f25bb7fbe266ed2ce5b7db2093f40 --- .../AidlConversionCppNdk.cpp | 20 +------- .../audio_aidl_legacy_conversion_tests.cpp | 20 -------- media/utils/ServiceUtilities.cpp | 43 ----------------- .../include/mediautils/ServiceUtilities.h | 4 -- .../service/AudioPolicyInterfaceImpl.cpp | 46 ------------------- 5 files changed, 2 insertions(+), 131 deletions(-) diff --git a/media/audioaidlconversion/AidlConversionCppNdk.cpp b/media/audioaidlconversion/AidlConversionCppNdk.cpp index f13cfa3aab..3b06245e0b 100644 --- a/media/audioaidlconversion/AidlConversionCppNdk.cpp +++ b/media/audioaidlconversion/AidlConversionCppNdk.cpp @@ -1052,13 +1052,6 @@ AudioDeviceAddress::Tag suggestDeviceAddressTag(const AudioDeviceDescription& de if (mac.size() != 6) return BAD_VALUE; snprintf(addressBuffer, AUDIO_DEVICE_MAX_ADDRESS_LEN, "%02X:%02X:%02X:%02X:%02X:%02X", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); - // special case for anonymized mac address: - // change anonymized bytes back from FD:FF:FF:FF to XX:XX:XX:XX - std::string address(addressBuffer); - if (address.compare(0, strlen("FD:FF:FF:FF"), "FD:FF:FF:FF") == 0) { - address.replace(0, strlen("FD:FF:FF:FF"), "XX:XX:XX:XX"); - } - strcpy(addressBuffer, address.c_str()); } break; case Tag::ipv4: { const std::vector& ipv4 = aidl.address.get(); @@ -1115,20 +1108,11 @@ legacy2aidl_audio_device_AudioDevice( if (!legacyAddress.empty()) { switch (suggestDeviceAddressTag(aidl.type)) { case Tag::mac: { - // special case for anonymized mac address: - // change anonymized bytes so that they can be scanned as HEX bytes - // Use '01' for LSB bits 0 and 1 as Bluetooth MAC addresses are never multicast - // and universaly administered - std::string address = legacyAddress; - if (address.compare(0, strlen("XX:XX:XX:XX"), "XX:XX:XX:XX") == 0) { - address.replace(0, strlen("XX:XX:XX:XX"), "FD:FF:FF:FF"); - } - std::vector mac(6); - int status = sscanf(address.c_str(), "%hhX:%hhX:%hhX:%hhX:%hhX:%hhX", + int status = sscanf(legacyAddress.c_str(), "%hhX:%hhX:%hhX:%hhX:%hhX:%hhX", &mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5]); if (status != mac.size()) { - ALOGE("%s: malformed MAC address: \"%s\"", __func__, address.c_str()); + ALOGE("%s: malformed MAC address: \"%s\"", __func__, legacyAddress.c_str()); return unexpected(BAD_VALUE); } aidl.address = AudioDeviceAddress::make(std::move(mac)); diff --git a/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp b/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp index 67473cec5e..0d12f9d9e1 100644 --- a/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp +++ b/media/libaudioclient/tests/audio_aidl_legacy_conversion_tests.cpp @@ -475,28 +475,8 @@ INSTANTIATE_TEST_SUITE_P( AudioDeviceAddress::make( std::vector{1, 2})))); -TEST(AnonymizedBluetoothAddressRoundTripTest, Legacy2Aidl2Legacy) { - const std::vector sAnonymizedAidlAddress = - std::vector{0xFD, 0xFF, 0xFF, 0xFF, 0xAB, 0xCD}; - const std::string sAnonymizedLegacyAddress = std::string("XX:XX:XX:XX:AB:CD"); - auto device = legacy2aidl_audio_device_AudioDevice(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP, - sAnonymizedLegacyAddress); - ASSERT_TRUE(device.ok()); - ASSERT_EQ(AudioDeviceAddress::Tag::mac, device.value().address.getTag()); - ASSERT_EQ(sAnonymizedAidlAddress, device.value().address.get()); - - audio_devices_t legacyType; - std::string legacyAddress; - status_t status = - aidl2legacy_AudioDevice_audio_device(device.value(), &legacyType, &legacyAddress); - ASSERT_EQ(OK, status); - EXPECT_EQ(legacyType, AUDIO_DEVICE_OUT_BLUETOOTH_A2DP); - EXPECT_EQ(sAnonymizedLegacyAddress, legacyAddress); -} - class AudioFormatDescriptionRoundTripTest : public testing::TestWithParam { }; - TEST_P(AudioFormatDescriptionRoundTripTest, Aidl2Legacy2Aidl) { const auto initial = GetParam(); auto conv = aidl2legacy_AudioFormatDescription_audio_format_t(initial); diff --git a/media/utils/ServiceUtilities.cpp b/media/utils/ServiceUtilities.cpp index 4ee45c795e..83b84e392c 100644 --- a/media/utils/ServiceUtilities.cpp +++ b/media/utils/ServiceUtilities.cpp @@ -46,7 +46,6 @@ static const String16 sAndroidPermissionRecordAudio("android.permission.RECORD_A static const String16 sModifyPhoneState("android.permission.MODIFY_PHONE_STATE"); static const String16 sModifyAudioRouting("android.permission.MODIFY_AUDIO_ROUTING"); static const String16 sCallAudioInterception("android.permission.CALL_AUDIO_INTERCEPTION"); -static const String16 sAndroidPermissionBluetoothConnect("android.permission.BLUETOOTH_CONNECT"); static String16 resolveCallingPackage(PermissionController& permissionController, const std::optional opPackageName, uid_t uid) { @@ -375,48 +374,6 @@ status_t checkIMemory(const sp& iMemory) return NO_ERROR; } -/** - * Determines if the MAC address in Bluetooth device descriptors returned by APIs of - * a native audio service (audio flinger, audio policy) must be anonymized. - * MAC addresses returned to system server or apps with BLUETOOTH_CONNECT permission - * are not anonymized. - * - * @param attributionSource The attribution source of the calling app. - * @param caller string identifying the caller for logging. - * @return true if the MAC addresses must be anonymized, false otherwise. - */ -bool mustAnonymizeBluetoothAddress( - const AttributionSourceState& attributionSource, const String16& caller) { - uid_t uid = VALUE_OR_FATAL(aidl2legacy_int32_t_uid_t(attributionSource.uid)); - if (isAudioServerOrSystemServerUid(uid)) { - return false; - } - const std::optional resolvedAttributionSource = - resolveAttributionSource(attributionSource); - if (!resolvedAttributionSource.has_value()) { - return true; - } - permission::PermissionChecker permissionChecker; - return permissionChecker.checkPermissionForPreflightFromDatasource( - sAndroidPermissionBluetoothConnect, resolvedAttributionSource.value(), caller, - AppOpsManager::OP_BLUETOOTH_CONNECT) - != permission::PermissionChecker::PERMISSION_GRANTED; -} - -/** - * Modifies the passed MAC address string in place for consumption by unprivileged clients. - * the string is assumed to have a valid MAC address format. - * the anonymzation must be kept in sync with toAnonymizedAddress() in BluetoothUtils.java - * - * @param address input/output the char string contining the MAC address to anonymize. - */ -void anonymizeBluetoothAddress(char *address) { - if (address == nullptr || strlen(address) != strlen("AA:BB:CC:DD:EE:FF")) { - return; - } - memcpy(address, "XX:XX:XX:XX", strlen("XX:XX:XX:XX")); -} - sp MediaPackageManager::retrievePackageManager() { const sp sm = defaultServiceManager(); if (sm == nullptr) { diff --git a/media/utils/include/mediautils/ServiceUtilities.h b/media/utils/include/mediautils/ServiceUtilities.h index 0b3a3f96cd..3d7981ae97 100644 --- a/media/utils/include/mediautils/ServiceUtilities.h +++ b/media/utils/include/mediautils/ServiceUtilities.h @@ -108,10 +108,6 @@ bool modifyPhoneStateAllowed(const AttributionSourceState& attributionSource); bool bypassInterruptionPolicyAllowed(const AttributionSourceState& attributionSource); bool callAudioInterceptionAllowed(const AttributionSourceState& attributionSource); void purgePermissionCache(); -bool mustAnonymizeBluetoothAddress( - const AttributionSourceState& attributionSource, const String16& caller); -void anonymizeBluetoothAddress(char *address); - int32_t getOpForSource(audio_source_t source); AttributionSourceState getCallingAttributionSource(); diff --git a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp index 721ca1a26a..2e7b3ffb84 100644 --- a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp +++ b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp @@ -1528,19 +1528,6 @@ Status AudioPolicyService::isDirectOutputSupported( return Status::ok(); } -template -void anonymizePortBluetoothAddress(Port *port) { - if (port->type != AUDIO_PORT_TYPE_DEVICE) { - return; - } - if (!(audio_is_a2dp_device(port->ext.device.type) - || audio_is_ble_device(port->ext.device.type) - || audio_is_bluetooth_sco_device(port->ext.device.type) - || audio_is_hearing_aid_out_device(port->ext.device.type))) { - return; - } - anonymizeBluetoothAddress(port->ext.device.address); -} Status AudioPolicyService::listAudioPorts(media::AudioPortRole roleAidl, media::AudioPortType typeAidl, Int* count, @@ -1563,20 +1550,10 @@ Status AudioPolicyService::listAudioPorts(media::AudioPortRole roleAidl, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } - - const AttributionSourceState attributionSource = getCallingAttributionSource(); - AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( mAudioPolicyManager->listAudioPorts(role, type, &num_ports, ports.get(), &generation))); numPortsReq = std::min(numPortsReq, num_ports); - - if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { - for (size_t i = 0; i < numPortsReq; ++i) { - anonymizePortBluetoothAddress(&ports[i]); - } - } - RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( convertRange(ports.get(), ports.get() + numPortsReq, std::back_inserter(*portsAidl), legacy2aidl_audio_port_v7_AudioPortFw))); @@ -1603,16 +1580,8 @@ Status AudioPolicyService::getAudioPort(int portId, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } - - const AttributionSourceState attributionSource = getCallingAttributionSource(); - AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT(mAudioPolicyManager->getAudioPort(&port))); - - if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { - anonymizePortBluetoothAddress(&port); - } - *_aidl_return = VALUE_OR_RETURN_BINDER_STATUS(legacy2aidl_audio_port_v7_AudioPortFw(port)); return Status::ok(); } @@ -1674,25 +1643,10 @@ Status AudioPolicyService::listAudioPatches(Int* count, if (mAudioPolicyManager == NULL) { return binderStatusFromStatusT(NO_INIT); } - - const AttributionSourceState attributionSource = getCallingAttributionSource(); - AutoCallerClear acc; RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( mAudioPolicyManager->listAudioPatches(&num_patches, patches.get(), &generation))); numPatchesReq = std::min(numPatchesReq, num_patches); - - if (mustAnonymizeBluetoothAddress(attributionSource, String16(__func__))) { - for (size_t i = 0; i < numPatchesReq; ++i) { - for (size_t j = 0; j < patches[i].num_sources; ++j) { - anonymizePortBluetoothAddress(&patches[i].sources[j]); - } - for (size_t j = 0; j < patches[i].num_sinks; ++j) { - anonymizePortBluetoothAddress(&patches[i].sinks[j]); - } - } - } - RETURN_IF_BINDER_ERROR(binderStatusFromStatusT( convertRange(patches.get(), patches.get() + numPatchesReq, std::back_inserter(*patchesAidl), legacy2aidl_audio_patch_AudioPatchFw))); -- GitLab From 4b68b00993849b6a7f0e6d075bc2c8bb2e184e61 Mon Sep 17 00:00:00 2001 From: Devin Moore Date: Fri, 23 Feb 2024 19:19:38 +0000 Subject: [PATCH 16/20] libmediatranscoding: handle death recipient cookie ownership differently The ownership of the death recipient cookie is now limited to the TranscodingResourcePolicy object and the binderDied callback. They both must be able to delete the cookie object and they both must be aware of it already being deleted. In all cases, the TranscodingResourcePolicy object that needs to be unregistered will outlive the cookie and the death recipient. Calling unlinkToDeath is unneccessary because the last strong ref to the binder that was linked to death is removed in the unregisterSelf method which will unlink the binder and death recipient. Test: atest CtsMediaTranscodingTestCases MediaSampleReaderNDKTests Test: adb shell kill -9 `pid media.resource_observer` Test: delete mResourcePolicy.get() to force destructor after linkToDeath Bug: 319210610 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0c674f5ff68daa64b90e1a234061ba9bebe6173c) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3bd045d3543190b1e8d2d26743356ad657f25e33) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:13ed07dee27c3affa4511f02c612701d6cbf603a) Merged-In: I8e6ba40fe3da30bf8753e7a16ad5c8cd5dfda40b Change-Id: I8e6ba40fe3da30bf8753e7a16ad5c8cd5dfda40b --- .../TranscodingResourcePolicy.cpp | 57 +++++++++++++++++-- .../include/media/TranscodingResourcePolicy.h | 4 ++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/media/module/libmediatranscoding/TranscodingResourcePolicy.cpp b/media/module/libmediatranscoding/TranscodingResourcePolicy.cpp index af53f64671..6a0c09a20a 100644 --- a/media/module/libmediatranscoding/TranscodingResourcePolicy.cpp +++ b/media/module/libmediatranscoding/TranscodingResourcePolicy.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -66,11 +67,31 @@ struct TranscodingResourcePolicy::ResourceObserver : public BnResourceObserver { TranscodingResourcePolicy* mOwner; }; +// cookie used for death recipients. The TranscodingResourcePolicy +// that this cookie is associated with must outlive this cookie. It is +// either deleted by binderDied, or in unregisterSelf which is also called +// in the destructor of TranscodingResourcePolicy +class TranscodingResourcePolicyCookie { + public: + TranscodingResourcePolicyCookie(TranscodingResourcePolicy* policy) : mPolicy(policy) {} + TranscodingResourcePolicyCookie() = delete; + TranscodingResourcePolicy* mPolicy; +}; + +static std::map> sCookies; +static uintptr_t sCookieKeyCounter; +static std::mutex sCookiesMutex; + // static void TranscodingResourcePolicy::BinderDiedCallback(void* cookie) { - TranscodingResourcePolicy* owner = reinterpret_cast(cookie); - if (owner != nullptr) { - owner->unregisterSelf(); + std::lock_guard guard(sCookiesMutex); + if (auto it = sCookies.find(reinterpret_cast(cookie)); it != sCookies.end()) { + ALOGI("BinderDiedCallback unregistering TranscodingResourcePolicy"); + auto policy = reinterpret_cast(it->second->mPolicy); + if (policy) { + policy->unregisterSelf(); + } + sCookies.erase(it); } // TODO(chz): retry to connecting to IResourceObserverService after failure. // Also need to have back-up logic if IResourceObserverService is offline for @@ -88,6 +109,24 @@ TranscodingResourcePolicy::TranscodingResourcePolicy() } TranscodingResourcePolicy::~TranscodingResourcePolicy() { + { + std::lock_guard guard(sCookiesMutex); + + // delete all of the cookies associated with this TranscodingResourcePolicy + // instance since they are holding pointers to this object that will no + // longer be valid. + for (auto it = sCookies.begin(); it != sCookies.end();) { + const uintptr_t key = it->first; + std::lock_guard guard(mCookieKeysLock); + if (mCookieKeys.find(key) != mCookieKeys.end()) { + // No longer need to track this cookie + mCookieKeys.erase(key); + it = sCookies.erase(it); + } else { + it++; + } + } + } unregisterSelf(); } @@ -123,7 +162,16 @@ void TranscodingResourcePolicy::registerSelf() { return; } - AIBinder_linkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast(this)); + std::unique_ptr cookie = + std::make_unique(this); + uintptr_t cookieKey = sCookieKeyCounter++; + sCookies.emplace(cookieKey, std::move(cookie)); + { + std::lock_guard guard(mCookieKeysLock); + mCookieKeys.insert(cookieKey); + } + + AIBinder_linkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast(cookieKey)); ALOGD("@@@ registered observer"); mRegistered = true; @@ -141,7 +189,6 @@ void TranscodingResourcePolicy::unregisterSelf() { ::ndk::SpAIBinder binder = mService->asBinder(); if (binder.get() != nullptr) { Status status = mService->unregisterObserver(mObserver); - AIBinder_unlinkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast(this)); } mService = nullptr; diff --git a/media/module/libmediatranscoding/include/media/TranscodingResourcePolicy.h b/media/module/libmediatranscoding/include/media/TranscodingResourcePolicy.h index ee232e7551..4d762b5832 100644 --- a/media/module/libmediatranscoding/include/media/TranscodingResourcePolicy.h +++ b/media/module/libmediatranscoding/include/media/TranscodingResourcePolicy.h @@ -22,6 +22,7 @@ #include #include +#include namespace aidl { namespace android { namespace media { @@ -48,6 +49,8 @@ private: bool mRegistered GUARDED_BY(mRegisteredLock); std::shared_ptr mService GUARDED_BY(mRegisteredLock); std::shared_ptr mObserver; + mutable std::mutex mCookieKeysLock; + std::set mCookieKeys; mutable std::mutex mCallbackLock; std::weak_ptr mResourcePolicyCallback @@ -59,6 +62,7 @@ private: static void BinderDiedCallback(void* cookie); void registerSelf(); + // must delete the associated TranscodingResourcePolicyCookie any time this is called void unregisterSelf(); void onResourceAvailable(pid_t pid); }; // class TranscodingUidPolicy -- GitLab From 6cfd048292b2cc706811a22c9078208cfa8e6d24 Mon Sep 17 00:00:00 2001 From: Rakesh Kumar Date: Thu, 30 May 2024 11:17:48 +0000 Subject: [PATCH 17/20] StagefrightRecoder: Disabling B-frame support Disabling b-frame support from stagefright recorder in case of audio source as mic and video source is surface use case only because screen recorder with microphone doesn't play in sync if b-frame is enabled. If the audio source selected is INTERNAL (i.e. device) or MIC_AND_INTERNAL with screen recorder then b frame is supported. Bug: 288549440 Test: manually check screen recording with audio from mic has audio/video in synch (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:af685c66bab17b71fe1624f76b5d55628f79e6fa) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:da3407f7688f35eb2dce79f1405feeb182241a3c) Merged-In: I4098655eb9687fb633085333bc140634441566e6 Change-Id: I4098655eb9687fb633085333bc140634441566e6 --- media/libmediaplayerservice/StagefrightRecorder.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/media/libmediaplayerservice/StagefrightRecorder.cpp b/media/libmediaplayerservice/StagefrightRecorder.cpp index 036508507b..e0449ffc0d 100644 --- a/media/libmediaplayerservice/StagefrightRecorder.cpp +++ b/media/libmediaplayerservice/StagefrightRecorder.cpp @@ -2099,6 +2099,11 @@ status_t StagefrightRecorder::setupVideoEncoder( if (tsLayers > 1) { uint32_t bLayers = std::min(2u, tsLayers - 1); // use up-to 2 B-layers + // TODO(b/341121900): Remove this once B frames are handled correctly in screen recorder + // use case in case of mic only + if (mAudioSource == AUDIO_SOURCE_MIC && mVideoSource == VIDEO_SOURCE_SURFACE) { + bLayers = 0; + } uint32_t pLayers = tsLayers - bLayers; format->setString( "ts-schema", AStringPrintf("android.generic.%u+%u", pLayers, bLayers)); -- GitLab From 6d23fa05a40e5462d4b9bad28afa932e6e12a4f3 Mon Sep 17 00:00:00 2001 From: Wonsik Kim Date: Fri, 28 Jun 2024 00:33:51 +0000 Subject: [PATCH 18/20] omx: check HDR10+ info param size Bug: 329641908 Test: presubmit Flag: EXEMPT security fix (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:53298956ba6bb8f147a632d7aaed8566dfc203ee) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:f816148a719d2a3bbf432f11da98b3d5fa7de74f) Merged-In: I72523e1de61e5f947174272b732e170e1c2964df Change-Id: I72523e1de61e5f947174272b732e170e1c2964df --- media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp b/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp index 418302389d..4ab5d10609 100644 --- a/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp +++ b/media/libstagefright/omx/SoftVideoDecoderOMXComponent.cpp @@ -619,6 +619,13 @@ OMX_ERRORTYPE SoftVideoDecoderOMXComponent::getConfig( if (!isValidOMXParam(outParams)) { return OMX_ErrorBadParameter; } + if (offsetof(DescribeHDR10PlusInfoParams, nValue) + outParams->nParamSize > + outParams->nSize) { + ALOGE("b/329641908: too large param size; nParamSize=%u nSize=%u", + outParams->nParamSize, outParams->nSize); + android_errorWriteLog(0x534e4554, "329641908"); + return OMX_ErrorBadParameter; + } outParams->nParamSizeUsed = info->size(); -- GitLab From b9a047c94deb06ab7ff956e4fb50b19ddd70cf9a Mon Sep 17 00:00:00 2001 From: Austin Borger Date: Wed, 8 May 2024 15:47:58 -0700 Subject: [PATCH 19/20] CameraService: Watch for foreground changes from AppOps AppOps now sends notifications when an app loses its camera capability. We should watch for these notifications and block clients when they happen, assuming they're untrusted. Otherwise, an app that should not have camera access outside of the foreground may retain it. Bug: 290086710 Test: Ran on physical device, tested Zoom + GCA (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5673af6bd01f27f50768e8e2d5b7d31cea461f1e) Merged-In: Iaa21d74873da1dcc2453ad98abb284bad3a211f5 Change-Id: Iaa21d74873da1dcc2453ad98abb284bad3a211f5 --- services/camera/libcameraservice/CameraService.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp index 668a51ae6d..5eb9192c95 100644 --- a/services/camera/libcameraservice/CameraService.cpp +++ b/services/camera/libcameraservice/CameraService.cpp @@ -3683,7 +3683,8 @@ status_t CameraService::BasicClient::startCameraOps() { // Notify app ops that the camera is not available mOpsCallback = new OpsCallback(this); mAppOpsManager->startWatchingMode(AppOpsManager::OP_CAMERA, - mClientPackageName, mOpsCallback); + mClientPackageName, + AppOpsManager::WATCH_FOREGROUND_CHANGES, mOpsCallback); // Just check for camera acccess here on open - delay startOp until // camera frames start streaming in startCameraStreamingOps @@ -3842,6 +3843,12 @@ void CameraService::BasicClient::opChanged(int32_t op, const String16&) { block(); } else if (res == AppOpsManager::MODE_IGNORED) { bool isUidActive = sCameraService->mUidPolicy->isUidActive(mClientUid, mClientPackageName); + + // Uid may be active, but not visible to the user (e.g. PROCESS_STATE_FOREGROUND_SERVICE). + // If not visible, but still active, then we want to block instead of muting the camera. + int32_t procState = sCameraService->mUidPolicy->getProcState(mClientUid); + bool isUidVisible = (procState <= ActivityManager::PROCESS_STATE_BOUND_TOP); + bool isCameraPrivacyEnabled = sCameraService->mSensorPrivacyPolicy->isCameraPrivacyEnabled(); ALOGI("Camera %s: Access for \"%s\" has been restricted, isUidTrusted %d, isUidActive %d", @@ -3851,10 +3858,9 @@ void CameraService::BasicClient::opChanged(int32_t op, const String16&) { // b/175320666), the AppOpsManager could return MODE_IGNORED. Do not treat such cases as // error. if (!mUidIsTrusted) { - if (isUidActive && isCameraPrivacyEnabled && supportsCameraMute()) { + if (isUidVisible && isCameraPrivacyEnabled && supportsCameraMute()) { setCameraMute(true); - } else if (!isUidActive - || (isCameraPrivacyEnabled && !supportsCameraMute())) { + } else { block(); } } -- GitLab From e28ca0c3d70c67cda2a09dc2d663a3395b13c779 Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Wed, 26 Feb 2025 18:16:50 -0800 Subject: [PATCH 20/20] mediandk: clean up AMediaCodec_getInput/OutputBuffer semantics AMediaCodec_getInputBuffer erroneously considered the offset, which could result in a smaller input buffer being returned and client confusion. In practice, input buffer offset is rarely used. Similarly, the buffer size returned from AMediaCodec_getOutputBuffer included padding after the output buffer leading to potential confusion. Now the size returned from both AMediaCodec_getOutputBuffer and AMediaCodec_dequeueOutputBuffer is correct. Also removed mentions of non-existing NDK methods AMediaCodec_getOutputBuffers and AMediaCodec_getInputBuffers. Bug: 301470262 Flag: EXEMPT bugfix (cherry picked from commit d69fe7b73a0ed14c2b5bc237f1a42314140c9458) Merged-in: I6bbd9b85023aef56a608362afde0662d3df7284a (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0e8d99cb7893e23a6a941379bca31455ea068fc8) Merged-In: I6bbd9b85023aef56a608362afde0662d3df7284a Change-Id: I6bbd9b85023aef56a608362afde0662d3df7284a --- media/ndk/NdkMediaCodec.cpp | 21 ++++++++++++++++----- media/ndk/include/media/NdkMediaCodec.h | 15 +++++++++++++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/media/ndk/NdkMediaCodec.cpp b/media/ndk/NdkMediaCodec.cpp index b230df5179..240848096a 100644 --- a/media/ndk/NdkMediaCodec.cpp +++ b/media/ndk/NdkMediaCodec.cpp @@ -672,7 +672,13 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abuf->capacity(); } - return abuf->data(); + + // When an input buffer is provided to the application, it is essentially + // empty. Ignore its offset as we will set it upon queueInputBuffer. + // This actually works as expected as we do not provide visibility of + // a potential internal offset to the client, so it is equivalent to + // setting the offset to 0 prior to returning the buffer to the client. + return abuf->base(); } android::Vector > abufs; @@ -689,7 +695,7 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec *mData, size_t idx, size_t *out_ if (out_size != NULL) { *out_size = abufs[idx]->capacity(); } - return abufs[idx]->data(); + return abufs[idx]->base(); } ALOGE("couldn't get input buffers"); return NULL; @@ -704,8 +710,12 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out return NULL; } + // Note that we do not provide visibility of the internal offset to the + // client, but it also does not make sense to provide visibility of the + // buffer capacity vs the actual size. + if (out_size != NULL) { - *out_size = abuf->capacity(); + *out_size = abuf->size(); } return abuf->data(); } @@ -718,7 +728,7 @@ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec *mData, size_t idx, size_t *out return NULL; } if (out_size != NULL) { - *out_size = abufs[idx]->capacity(); + *out_size = abufs[idx]->size(); } return abufs[idx]->data(); } @@ -748,7 +758,8 @@ ssize_t AMediaCodec_dequeueOutputBuffer(AMediaCodec *mData, requestActivityNotification(mData); switch (ret) { case OK: - info->offset = offset; + // the output buffer address is already offset in AMediaCodec_getOutputBuffer() + info->offset = 0; info->size = size; info->flags = flags; info->presentationTimeUs = presentationTimeUs; diff --git a/media/ndk/include/media/NdkMediaCodec.h b/media/ndk/include/media/NdkMediaCodec.h index 598beb709d..223d2f890b 100644 --- a/media/ndk/include/media/NdkMediaCodec.h +++ b/media/ndk/include/media/NdkMediaCodec.h @@ -251,6 +251,11 @@ uint8_t* AMediaCodec_getInputBuffer(AMediaCodec*, size_t idx, size_t *out_size) * dequeueOutputBuffer, and not yet queued. * * Available since API level 21. + *

+ * At or before API level 35, the out_size returned was invalid, and instead the + * size returned in the AMediaCodecBufferInfo struct from + * AMediaCodec_dequeueOutputBuffer() should be used. After API + * level 35, this API returns the correct output buffer size as well. */ uint8_t* AMediaCodec_getOutputBuffer(AMediaCodec*, size_t idx, size_t *out_size) __INTRODUCED_IN(21); @@ -309,9 +314,16 @@ media_status_t AMediaCodec_queueSecureInputBuffer(AMediaCodec*, size_t idx, #undef _off_t_compat /** - * Get the index of the next available buffer of processed data. + * Get the index of the next available buffer of processed data along with the + * metadata associated with it. * * Available since API level 21. + *

+ * At or before API level 35, the offset in the AMediaCodecBufferInfo struct + * was invalid and should be ignored; however, at the same time + * the buffer size could only be obtained from this struct. After API + * level 35, the offset returned in the struct is always set to 0, and the + * buffer size can also be obtained from the AMediaCodec_getOutputBuffer() call. */ ssize_t AMediaCodec_dequeueOutputBuffer(AMediaCodec*, AMediaCodecBufferInfo *info, int64_t timeoutUs) __INTRODUCED_IN(21); @@ -468,7 +480,6 @@ void AMediaCodec_releaseName(AMediaCodec*, char* name) __INTRODUCED_IN(28); /** * Set an asynchronous callback for actionable AMediaCodec events. * When asynchronous callback is enabled, it is an error for the client to call - * AMediaCodec_getInputBuffers(), AMediaCodec_getOutputBuffers(), * AMediaCodec_dequeueInputBuffer() or AMediaCodec_dequeueOutputBuffer(). * * AMediaCodec_flush() behaves differently in asynchronous mode. -- GitLab