From c14f262876818498b3ca77f1d6df9d4fb7e77b1c Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Tue, 26 Jan 2021 20:29:25 -0800 Subject: [PATCH 01/11] Fix potential decrypt src pointer overflow. There is a potential integer overflow to bypass the source base size check in decrypt. The source pointer can then point to the outside of the source buffer, which could potentially leak arbitrary memory content to destination pointer. Test: sts-tradefed sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176496160#testPocBug_176496160 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-17649616064 Bug: 176496160 Bug: 176444786 Change-Id: I811a6f60948bde2a72906c2c6172fd7bc5feb6d9 --- drm/1.0/default/CryptoPlugin.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index 2db360765b..e6d4e8447b 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -124,7 +124,11 @@ namespace implementation { return Void(); } - if (source.offset + offset + source.size > sourceBase->getSize()) { + size_t totalSize = 0; + if (__builtin_add_overflow(source.offset, offset, &totalSize) || + __builtin_add_overflow(totalSize, source.size, &totalSize) || + totalSize > sourceBase->getSize()) { + android_errorWriteLog(0x534e4554, "176496160"); _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "invalid buffer size"); return Void(); } From b2bb9879513e9e535a73cef1037818d5b27c153a Mon Sep 17 00:00:00 2001 From: Pierce Huang Date: Thu, 4 Feb 2021 11:41:48 +0800 Subject: [PATCH 02/11] Camera: Set arbitrary dataspace for RAW streams Use 'arbitrary' dataspace when trying to configure RAW streams. Bug: 179158925 Test: pass vts with VtsHalCameraProviderV2_4TargetTest Change-Id: I6360e4a459ae51af4898a7329fe31ded152efe8f --- .../VtsHalCameraProviderV2_4TargetTest.cpp | 58 ++++++++----------- 1 file changed, 24 insertions(+), 34 deletions(-) diff --git a/camera/provider/2.4/vts/functional/VtsHalCameraProviderV2_4TargetTest.cpp b/camera/provider/2.4/vts/functional/VtsHalCameraProviderV2_4TargetTest.cpp index b0aae8e04d..560b24e5be 100644 --- a/camera/provider/2.4/vts/functional/VtsHalCameraProviderV2_4TargetTest.cpp +++ b/camera/provider/2.4/vts/functional/VtsHalCameraProviderV2_4TargetTest.cpp @@ -888,6 +888,8 @@ public: static Status getSystemCameraKind(const camera_metadata_t* staticMeta, SystemCameraKind* systemCameraKind); + static V3_2::DataspaceFlags getDataspace(PixelFormat format); + void processCaptureRequestInternal(uint64_t bufferusage, RequestTemplate reqTemplate, bool useSecureOnlyCameras); @@ -3173,7 +3175,6 @@ TEST_P(CameraHidlTest, constructDefaultRequestSettings) { } } - // Verify that all supported stream formats and sizes can be configured // successfully. TEST_P(CameraHidlTest, configureStreamsAvailableOutputs) { @@ -3216,17 +3217,7 @@ TEST_P(CameraHidlTest, configureStreamsAvailableOutputs) { uint32_t streamConfigCounter = 0; for (auto& it : outputStreams) { V3_2::Stream stream3_2; - V3_2::DataspaceFlags dataspaceFlag = 0; - switch (static_cast(it.format)) { - case PixelFormat::BLOB: - dataspaceFlag = static_cast(Dataspace::V0_JFIF); - break; - case PixelFormat::Y16: - dataspaceFlag = static_cast(Dataspace::DEPTH); - break; - default: - dataspaceFlag = static_cast(Dataspace::UNKNOWN); - } + V3_2::DataspaceFlags dataspaceFlag = getDataspace(static_cast(it.format)); stream3_2 = {streamId, StreamType::OUTPUT, static_cast(it.width), @@ -3346,17 +3337,8 @@ TEST_P(CameraHidlTest, configureConcurrentStreamsAvailableOutputs) { size_t j = 0; for (const auto& it : outputStreams) { V3_2::Stream stream3_2; - V3_2::DataspaceFlags dataspaceFlag = 0; - switch (static_cast(it.format)) { - case PixelFormat::BLOB: - dataspaceFlag = static_cast(Dataspace::V0_JFIF); - break; - case PixelFormat::Y16: - dataspaceFlag = static_cast(Dataspace::DEPTH); - break; - default: - dataspaceFlag = static_cast(Dataspace::UNKNOWN); - } + V3_2::DataspaceFlags dataspaceFlag = + getDataspace(static_cast(it.format)); stream3_2 = {streamId++, StreamType::OUTPUT, static_cast(it.width), @@ -5896,6 +5878,23 @@ Status CameraHidlTest::getSystemCameraKind(const camera_metadata_t* staticMeta, return ret; } +// Select an appropriate dataspace given a specific pixel format. +V3_2::DataspaceFlags CameraHidlTest::getDataspace(PixelFormat format) { + switch (format) { + case PixelFormat::BLOB: + return static_cast(Dataspace::V0_JFIF); + case PixelFormat::Y16: + return static_cast(Dataspace::DEPTH); + case PixelFormat::RAW16: + case PixelFormat::RAW_OPAQUE: + case PixelFormat::RAW10: + case PixelFormat::RAW12: + return static_cast(Dataspace::ARBITRARY); + default: + return static_cast(Dataspace::UNKNOWN); + } +} + // Check whether this is a monochrome camera using the static camera characteristics. Status CameraHidlTest::isMonochromeCamera(const camera_metadata_t *staticMeta) { Status ret = Status::METHOD_NOT_SUPPORTED; @@ -6270,17 +6269,8 @@ void CameraHidlTest::configureOfflineStillStream(const std::string &name, ASSERT_EQ(Status::OK, rc); ASSERT_FALSE(outputStreams.empty()); - V3_2::DataspaceFlags dataspaceFlag = 0; - switch (static_cast(outputStreams[idx].format)) { - case PixelFormat::BLOB: - dataspaceFlag = static_cast(Dataspace::V0_JFIF); - break; - case PixelFormat::Y16: - dataspaceFlag = static_cast(Dataspace::DEPTH); - break; - default: - dataspaceFlag = static_cast(Dataspace::UNKNOWN); - } + V3_2::DataspaceFlags dataspaceFlag = + getDataspace(static_cast(outputStreams[idx].format)); ::android::hardware::hidl_vec streams3_4(/*size*/1); V3_4::Stream stream3_4 = {{ 0 /*streamId*/, StreamType::OUTPUT, From 06947f4082ec86d91f929077d290798623a050fa Mon Sep 17 00:00:00 2001 From: Bob Badour Date: Tue, 23 Feb 2021 15:42:22 -0800 Subject: [PATCH 03/11] [LSC] Add LOCAL_LICENSE_KINDS to hardware/interfaces Added SPDX-license-identifier-Apache-2.0 to: security/keymint/aidl/vts/performance/Android.bp Bug: 68860345 Bug: 151177513 Bug: 151953481 Test: m all Exempt-From-Owner-Approval: janitorial work Change-Id: Icc32cf30d46931ab23bc7a3a8343c380774db532 Merged-in: If5e9ac8023cdc10d2370d16e7dd75a4bbb7085aa --- security/keymint/aidl/vts/performance/Android.bp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/security/keymint/aidl/vts/performance/Android.bp b/security/keymint/aidl/vts/performance/Android.bp index 03240c3459..79ed0d5442 100644 --- a/security/keymint/aidl/vts/performance/Android.bp +++ b/security/keymint/aidl/vts/performance/Android.bp @@ -14,6 +14,15 @@ // limitations under the License. // +package { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "hardware_interfaces_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["hardware_interfaces_license"], +} + cc_benchmark { name: "VtsAidlKeyMintBenchmarkTest", defaults: [ From a6e551feef4e5faeec9ecc6b287eeade751e7f8b Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Tue, 2 Feb 2021 10:42:38 -0800 Subject: [PATCH 04/11] [RESTRICT AUTOMERGE] Fix potential decrypt destPtr overflow. There is a potential integer overflow to bypass the destination base size check in decrypt. The destPtr can then point to the outside of the destination buffer. Test: sts-tradefed sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176444622#testPocBug_176444622 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-17644462264 Bug: 176444622 Bug: 176496353 Change-Id: I71b390846a17aecbb9180865e1f9538b4b464abf --- drm/1.0/default/CryptoPlugin.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index e6d4e8447b..653708e12c 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -79,7 +79,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -146,7 +146,10 @@ namespace implementation { return Void(); } - if (destBuffer.offset + destBuffer.size > destBase->getSize()) { + size_t totalDstSize = 0; + if (__builtin_add_overflow(destBuffer.offset, destBuffer.size, &totalDstSize) || + totalDstSize > destBase->getSize()) { + android_errorWriteLog(0x534e4554, "176496353"); _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "invalid buffer size"); return Void(); } @@ -157,7 +160,7 @@ namespace implementation { } base = static_cast(static_cast(destBase->getPointer())); - destPtr = static_cast(base + destination.nonsecureMemory.offset); + destPtr = static_cast(base + destination.nonsecureMemory.offset); } else if (destination.type == BufferType::NATIVE_HANDLE) { if (!secure) { _hidl_cb(Status::BAD_VALUE, 0, "native handle destination must be secure"); From 9191787d0e73712608eff22fca9aea9480d4691e Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Tue, 2 Feb 2021 22:28:41 -0800 Subject: [PATCH 05/11] Fix potential decrypt destPtr overflow. There is a potential integer overflow to bypass the destination base size check in decrypt. The destPtr can then point to the outside of the destination buffer. Test: sts-tradefed sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176444622#testPocBug_176444622 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-17644462264 Bug: 176444622 Bug: 176496353 Change-Id: I63043d10796f82ad805038ba1fad5bd7d5c89961 Merged-In: I63043d10796f82ad805038ba1fad5bd7d5c89961 --- drm/1.0/default/CryptoPlugin.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index 2db360765b..8af190b276 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -79,7 +79,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -142,7 +142,10 @@ namespace implementation { return Void(); } - if (destBuffer.offset + destBuffer.size > destBase->getSize()) { + size_t totalSize = 0; + if (__builtin_add_overflow(destBuffer.offset, destBuffer.size, &totalSize) || + totalSize > destBase->getSize()) { + android_errorWriteLog(0x534e4554, "176496353"); _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "invalid buffer size"); return Void(); } @@ -153,7 +156,7 @@ namespace implementation { } base = static_cast(static_cast(destBase->getPointer())); - destPtr = static_cast(base + destination.nonsecureMemory.offset); + destPtr = static_cast(base + destination.nonsecureMemory.offset); } else if (destination.type == BufferType::NATIVE_HANDLE) { if (!secure) { _hidl_cb(Status::BAD_VALUE, 0, "native handle destination must be secure"); From ecd3296f1eb58e250b46b28761b13f88dcb025f6 Mon Sep 17 00:00:00 2001 From: Jasmine Chen Date: Thu, 25 Feb 2021 21:57:31 +0800 Subject: [PATCH 06/11] Camera: Fix double-free in removeCamera When a camera is removed, |hidlMetadata| takes over ownership of |metadata|. Therefore, we should not free |metadata| again. Bug: 180014486 Test: Remove an external camera, and no crashes were observed. Marged-In: I85246067f8753911cbcb58af24a46f12962226f3 Change-Id: I85246067f8753911cbcb58af24a46f12962226f3 (cherry picked from commit f36a439e3a97315a9f0a8661ea77aff61fb79c82) (cherry picked from commit 682abf403969efcbc8072a26f687560ad9def69a) --- camera/common/1.0/default/CameraModule.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/camera/common/1.0/default/CameraModule.cpp b/camera/common/1.0/default/CameraModule.cpp index 27e74f1480..16fb85cc64 100644 --- a/camera/common/1.0/default/CameraModule.cpp +++ b/camera/common/1.0/default/CameraModule.cpp @@ -549,7 +549,6 @@ void CameraModule::removeCamera(int cameraId) { } } } - free_camera_metadata(metadata); } mCameraInfoMap.removeItem(cameraId); From 361d57017b1b1cef645aa1ec92d9ecbd8b4b8b0a Mon Sep 17 00:00:00 2001 From: Shuo Qian Date: Mon, 13 Jul 2020 13:52:22 -0700 Subject: [PATCH 07/11] Change range of SS-RSRQ per 3gpp Bug: 159761054 Test: VTS Merged-In: I377ef00015876b706ffeb20d9255c1b1ebf66c15 Change-Id: I377ef00015876b706ffeb20d9255c1b1ebf66c15 (cherry picked from commit cd3fd87d475a94b67d3544001cbfa0e2712fc7b6) --- current.txt | 2 ++ radio/1.4/types.hal | 4 ++-- radio/1.5/types.hal | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/current.txt b/current.txt index 71a761ae64..c90c7dd905 100644 --- a/current.txt +++ b/current.txt @@ -781,6 +781,8 @@ ba84f3a750b1cc43ac51074e8b8e22df924f3e6d9068fac50d95bcf57b2b1d61 android.hardwar e8c86c69c438da8d1549856c1bb3e2d1b8da52722f8235ff49a30f2cce91742c android.hardware.soundtrigger@2.1::ISoundTriggerHwCallback b9fbb6e2e061ed0960939d48b785e9700210add1f13ed32ecd688d0f1ca20ef7 android.hardware.renderscript@1.0::types 0f53d70e1eadf8d987766db4bf6ae2048004682168f4cab118da576787def3fa android.hardware.radio@1.0::types +38d65fb20c60a5b823298560fc0825457ecdc49603a4b4e94bf81511790737da android.hardware.radio@1.4::types +954c334efd80e8869b66d1ce5fe2755712d96ba4b3c38d415739c330af5fb4cb android.hardware.radio@1.5::types # HALs released in Android S # NOTE: waiting to freeze HALs until later in the release diff --git a/radio/1.4/types.hal b/radio/1.4/types.hal index 393716b7ce..a830816822 100644 --- a/radio/1.4/types.hal +++ b/radio/1.4/types.hal @@ -1847,9 +1847,9 @@ struct NrSignalStrength { /** * SS reference signal received quality, multipled by -1. * - * Reference: 3GPP TS 38.215. + * Reference: 3GPP TS 38.215, 3GPP TS 38.133 section 10. * - * Range [3, 20], INT_MAX means invalid/unreported. + * Range [-20 dB, 43 dB], INT_MAX means invalid/unreported. */ int32_t ssRsrq; diff --git a/radio/1.5/types.hal b/radio/1.5/types.hal index b061bd51c8..c1f3f0358e 100644 --- a/radio/1.5/types.hal +++ b/radio/1.5/types.hal @@ -107,9 +107,9 @@ enum SignalMeasurementType : int32_t { SSRSRP = 6, /** * 5G SS reference signal received quality. - * Range: -20 dB to -3 dB. + * Range: -43 dB to 20 dB. * Used RAN: NGRAN - * Reference: 3GPP TS 38.215. + * Reference: 3GPP TS 38.215, 3GPP TS 38.133 section 10 */ SSRSRQ = 7, /** From a0cbb80f581d691c02bd932169f39c7a876c11ee Mon Sep 17 00:00:00 2001 From: Hao Chen Date: Mon, 8 Mar 2021 12:04:00 -0800 Subject: [PATCH 08/11] Gracefully stop the GeneratorHub worker thread in destructor To unit test the virtualized VHAL, GeneratorHub needs to be destructable Test: run unit tests in pa/1878260 Bug: 181371253 Change-Id: Icfd3007a194da7ade037e359858b3dd48f24a0d6 --- .../2.0/default/impl/vhal_v2_0/GeneratorHub.cpp | 17 ++++++++++++++--- .../2.0/default/impl/vhal_v2_0/GeneratorHub.h | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp index 548285abfb..9be9ea7c8e 100644 --- a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp +++ b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.cpp @@ -31,6 +31,14 @@ namespace impl { GeneratorHub::GeneratorHub(const OnHalEvent& onHalEvent) : mOnHalEvent(onHalEvent), mThread(&GeneratorHub::run, this) {} +GeneratorHub::~GeneratorHub() { + mShuttingDownFlag.store(true); + mCond.notify_all(); + if (mThread.joinable()) { + mThread.join(); + } +} + void GeneratorHub::registerGenerator(int32_t cookie, FakeValueGeneratorPtr generator) { { std::lock_guard g(mLock); @@ -58,15 +66,18 @@ void GeneratorHub::unregisterGenerator(int32_t cookie) { } void GeneratorHub::run() { - while (true) { + while (!mShuttingDownFlag.load()) { std::unique_lock g(mLock); // Pop events whose generator does not exist (may be already unregistered) while (!mEventQueue.empty() && mGenerators.find(mEventQueue.top().cookie) == mGenerators.end()) { mEventQueue.pop(); } - // Wait until event queue is not empty - mCond.wait(g, [this] { return !mEventQueue.empty(); }); + // Wait until event queue is not empty or shutting down flag is set + mCond.wait(g, [this] { return !mEventQueue.empty() || mShuttingDownFlag.load(); }); + if (mShuttingDownFlag.load()) { + break; + } const VhalEvent& curEvent = mEventQueue.top(); diff --git a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.h b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.h index dcf6a4f06e..b25dbf1c09 100644 --- a/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.h +++ b/automotive/vehicle/2.0/default/impl/vhal_v2_0/GeneratorHub.h @@ -58,7 +58,7 @@ private: public: GeneratorHub(const OnHalEvent& onHalEvent); - ~GeneratorHub() = default; + ~GeneratorHub(); /** * Register a new generator. The generator will be discarded if it could not produce next event. @@ -84,6 +84,7 @@ private: mutable std::mutex mLock; std::condition_variable mCond; std::thread mThread; + std::atomic mShuttingDownFlag{false}; }; } // namespace impl From 9fcd4886a3e1ccbc18acfadd84906400c9882eda Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Sat, 23 Jan 2021 17:25:14 -0800 Subject: [PATCH 09/11] [RESTRICT AUTOMERGE] Fix CryptoPlugin use after free vulnerability. The shared memory buffer used by srcPtr can be freed by another thread because it is not protected by a mutex. Subsequently, a use after free AIGABRT can occur in a race condition. SafetyNet logging is not added to avoid log spamming. The mutex lock is called to setup for decryption, which is called frequently. The crash was reproduced on the device before the fix. Verified the test passes after the fix. Test: sts sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176495665#testPocBug_176495665 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-176495665_sts64 Bug: 176495665 Bug: 176444161 Change-Id: I3ec33cd444183f40ee76bec4c88dec0dac859cd3 --- drm/1.0/default/Android.bp | 3 ++- drm/1.0/default/CryptoPlugin.cpp | 9 +++++++-- drm/1.0/default/CryptoPlugin.h | 21 +++++++++++++-------- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/drm/1.0/default/Android.bp b/drm/1.0/default/Android.bp index ed6bcdeee6..1122e46a05 100644 --- a/drm/1.0/default/Android.bp +++ b/drm/1.0/default/Android.bp @@ -9,6 +9,7 @@ cc_library_static { "-Werror", "-Wextra", "-Wall", + "-Wthread-safety", ], shared_libs: [ "liblog", @@ -19,5 +20,5 @@ cc_library_static { export_header_lib_headers: [ "libutils_headers", ], - export_include_dirs : ["include"] + export_include_dirs: ["include"], } diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index 2db360765b..dd519e6b3a 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -53,6 +53,8 @@ namespace implementation { uint32_t bufferId) { sp hidlMemory = mapMemory(base); + std::unique_lock lock(mSharedBufferLock); + // allow mapMemory to return nullptr mSharedBufferMap[bufferId] = hidlMemory; return Void(); @@ -65,7 +67,7 @@ namespace implementation { const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, decrypt_cb _hidl_cb) { - + std::unique_lock lock(mSharedBufferLock); if (mSharedBufferMap.find(source.bufferId) == mSharedBufferMap.end()) { _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "source decrypt buffer base not set"); return Void(); @@ -79,7 +81,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -166,6 +168,9 @@ namespace implementation { _hidl_cb(Status::BAD_VALUE, 0, "invalid destination type"); return Void(); } + + // release mSharedBufferLock + lock.unlock(); ssize_t result = mLegacyPlugin->decrypt(secure, keyId.data(), iv.data(), legacyMode, legacyPattern, srcPtr, legacySubSamples.get(), subSamples.size(), destPtr, &detailMessage); diff --git a/drm/1.0/default/CryptoPlugin.h b/drm/1.0/default/CryptoPlugin.h index 11cc2aae47..0d091fae65 100644 --- a/drm/1.0/default/CryptoPlugin.h +++ b/drm/1.0/default/CryptoPlugin.h @@ -17,11 +17,14 @@ #ifndef ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H #define ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H -#include +#include #include +#include #include #include +#include + namespace android { namespace hardware { namespace drm { @@ -60,19 +63,21 @@ struct CryptoPlugin : public ICryptoPlugin { Return setSharedBufferBase(const ::android::hardware::hidl_memory& base, uint32_t bufferId) override; - Return decrypt(bool secure, const hidl_array& keyId, - const hidl_array& iv, Mode mode, const Pattern& pattern, - const hidl_vec& subSamples, const SharedBuffer& source, - uint64_t offset, const DestinationBuffer& destination, - decrypt_cb _hidl_cb) override; + Return decrypt( + bool secure, const hidl_array& keyId, const hidl_array& iv, + Mode mode, const Pattern& pattern, const hidl_vec& subSamples, + const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, + decrypt_cb _hidl_cb) override NO_THREAD_SAFETY_ANALYSIS; // use unique_lock -private: + private: android::CryptoPlugin *mLegacyPlugin; - std::map > mSharedBufferMap; + std::map> mSharedBufferMap GUARDED_BY(mSharedBufferLock); CryptoPlugin() = delete; CryptoPlugin(const CryptoPlugin &) = delete; void operator=(const CryptoPlugin &) = delete; + + std::mutex mSharedBufferLock; }; } // namespace implementation From e3681c542cf8abcbb78eac85af667275792c3355 Mon Sep 17 00:00:00 2001 From: Bob Badour Date: Tue, 30 Mar 2021 12:40:47 -0700 Subject: [PATCH 10/11] [LSC] Add LOCAL_LICENSE_KINDS to hardware/interfaces Added SPDX-license-identifier-Apache-2.0 to: tv/cec/1.0/vts/functional/Android.bp Bug: 68860345 Bug: 151177513 Bug: 151953481 Test: m all Exempt-From-Owner-Approval: janitorial work Change-Id: I8441f20cebce71d6ba3c5bcf1e6f4fc6afe27130 --- tv/cec/1.0/vts/functional/Android.bp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tv/cec/1.0/vts/functional/Android.bp b/tv/cec/1.0/vts/functional/Android.bp index 4d82da388b..9a2c71437a 100644 --- a/tv/cec/1.0/vts/functional/Android.bp +++ b/tv/cec/1.0/vts/functional/Android.bp @@ -14,6 +14,15 @@ // limitations under the License. // +package { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "hardware_interfaces_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["hardware_interfaces_license"], +} + cc_test { name: "VtsHalTvCecV1_0TargetTest", defaults: ["VtsHalTargetTestDefaults"], From a4e76aab230a565dd0cef11e2e6e2d782b685327 Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Mon, 8 Mar 2021 18:46:42 -0800 Subject: [PATCH 11/11] [RESTRICT AUTOMERGE] Fix CryptoPlugin use after free vulnerability. The shared memory buffer used by srcPtr can be freed by another thread because it is not protected by a mutex. Subsequently, a use after free AIGABRT can occur in a race condition. SafetyNet logging is not added to avoid log spamming. The mutex lock is called to setup for decryption, which is called frequently. The crash was reproduced on the device before the fix. Verified the test passes after the fix. Test: sts sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176495665#testPocBug_176495665 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-176495665_sts64 Bug: 176495665 Bug: 176444161 Change-Id: I4c83c44873eef960b654f387a3574fcad49c41a9 --- drm/1.0/default/Android.bp | 3 ++- drm/1.0/default/CryptoPlugin.cpp | 10 ++++++++-- drm/1.0/default/CryptoPlugin.h | 21 +++++++++++++-------- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drm/1.0/default/Android.bp b/drm/1.0/default/Android.bp index ed6bcdeee6..1122e46a05 100644 --- a/drm/1.0/default/Android.bp +++ b/drm/1.0/default/Android.bp @@ -9,6 +9,7 @@ cc_library_static { "-Werror", "-Wextra", "-Wall", + "-Wthread-safety", ], shared_libs: [ "liblog", @@ -19,5 +20,5 @@ cc_library_static { export_header_lib_headers: [ "libutils_headers", ], - export_include_dirs : ["include"] + export_include_dirs: ["include"], } diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index e6d4e8447b..de8bbd599c 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -53,6 +53,8 @@ namespace implementation { uint32_t bufferId) { sp hidlMemory = mapMemory(base); + std::lock_guard shared_buffer_lock(mSharedBufferLock); + // allow mapMemory to return nullptr mSharedBufferMap[bufferId] = hidlMemory; return Void(); @@ -65,7 +67,7 @@ namespace implementation { const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, decrypt_cb _hidl_cb) { - + std::unique_lock shared_buffer_lock(mSharedBufferLock); if (mSharedBufferMap.find(source.bufferId) == mSharedBufferMap.end()) { _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "source decrypt buffer base not set"); return Void(); @@ -79,7 +81,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -170,6 +172,10 @@ namespace implementation { _hidl_cb(Status::BAD_VALUE, 0, "invalid destination type"); return Void(); } + + // release mSharedBufferLock + shared_buffer_lock.unlock(); + ssize_t result = mLegacyPlugin->decrypt(secure, keyId.data(), iv.data(), legacyMode, legacyPattern, srcPtr, legacySubSamples.get(), subSamples.size(), destPtr, &detailMessage); diff --git a/drm/1.0/default/CryptoPlugin.h b/drm/1.0/default/CryptoPlugin.h index 11cc2aae47..0d091fae65 100644 --- a/drm/1.0/default/CryptoPlugin.h +++ b/drm/1.0/default/CryptoPlugin.h @@ -17,11 +17,14 @@ #ifndef ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H #define ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H -#include +#include #include +#include #include #include +#include + namespace android { namespace hardware { namespace drm { @@ -60,19 +63,21 @@ struct CryptoPlugin : public ICryptoPlugin { Return setSharedBufferBase(const ::android::hardware::hidl_memory& base, uint32_t bufferId) override; - Return decrypt(bool secure, const hidl_array& keyId, - const hidl_array& iv, Mode mode, const Pattern& pattern, - const hidl_vec& subSamples, const SharedBuffer& source, - uint64_t offset, const DestinationBuffer& destination, - decrypt_cb _hidl_cb) override; + Return decrypt( + bool secure, const hidl_array& keyId, const hidl_array& iv, + Mode mode, const Pattern& pattern, const hidl_vec& subSamples, + const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, + decrypt_cb _hidl_cb) override NO_THREAD_SAFETY_ANALYSIS; // use unique_lock -private: + private: android::CryptoPlugin *mLegacyPlugin; - std::map > mSharedBufferMap; + std::map> mSharedBufferMap GUARDED_BY(mSharedBufferLock); CryptoPlugin() = delete; CryptoPlugin(const CryptoPlugin &) = delete; void operator=(const CryptoPlugin &) = delete; + + std::mutex mSharedBufferLock; }; } // namespace implementation