Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASB-August 2024 Security Patches integration #2546

Merged
merged 1 commit into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ index 419ff1aadc..fbbe777754 100644
# It must match one of the Android Security Patch Level strings of the Public Security Bulletins.
# If there is no $PLATFORM_SECURITY_PATCH set, keep it empty.
- PLATFORM_SECURITY_PATCH := 2023-05-05
+ PLATFORM_SECURITY_PATCH := 2024-07-01
+ PLATFORM_SECURITY_PATCH := 2024-08-01
endif

include $(BUILD_SYSTEM)/version_util.mk
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
From f21c760322b58d7cb92579cb18ff75869ddd345c Mon Sep 17 00:00:00 2001
From: Devin Moore <[email protected]>
Date: Fri, 23 Feb 2024 19:19:38 +0000
Subject: [PATCH] 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:0213a7d62ea5545b954145cc53e2bd65ed5dc609)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:686987b9613678fe6540079e85e0d94519551738)
Merged-In: I8e6ba40fe3da30bf8753e7a16ad5c8cd5dfda40b
Change-Id: I8e6ba40fe3da30bf8753e7a16ad5c8cd5dfda40b
---
.../TranscodingResourcePolicy.cpp | 58 +++++++++++++++++--
.../include/media/TranscodingResourcePolicy.h | 4 ++
2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/media/libmediatranscoding/TranscodingResourcePolicy.cpp b/media/libmediatranscoding/TranscodingResourcePolicy.cpp
index af53f64671..1b91362463 100644
--- a/media/libmediatranscoding/TranscodingResourcePolicy.cpp
+++ b/media/libmediatranscoding/TranscodingResourcePolicy.cpp
@@ -24,6 +24,8 @@
#include <media/TranscodingResourcePolicy.h>
#include <utils/Log.h>

+#include <map>
+
namespace android {

using Status = ::ndk::ScopedAStatus;
@@ -66,11 +68,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<uintptr_t, std::unique_ptr<TranscodingResourcePolicyCookie>> sCookies;
+static uintptr_t sCookieKeyCounter;
+static std::mutex sCookiesMutex;
+
// static
void TranscodingResourcePolicy::BinderDiedCallback(void* cookie) {
- TranscodingResourcePolicy* owner = reinterpret_cast<TranscodingResourcePolicy*>(cookie);
- if (owner != nullptr) {
- owner->unregisterSelf();
+ std::lock_guard<std::mutex> guard(sCookiesMutex);
+ if (auto it = sCookies.find(reinterpret_cast<uintptr_t>(cookie)); it != sCookies.end()) {
+ ALOGI("BinderDiedCallback unregistering TranscodingResourcePolicy");
+ auto policy = reinterpret_cast<TranscodingResourcePolicy*>(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 +110,24 @@ TranscodingResourcePolicy::TranscodingResourcePolicy()
}

TranscodingResourcePolicy::~TranscodingResourcePolicy() {
+ {
+ std::lock_guard<std::mutex> 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 +163,16 @@ void TranscodingResourcePolicy::registerSelf() {
return;
}

- AIBinder_linkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast<void*>(this));
+ std::unique_ptr<TranscodingResourcePolicyCookie> cookie =
+ std::make_unique<TranscodingResourcePolicyCookie>(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<void*>(cookieKey));

ALOGD("@@@ registered observer");
mRegistered = true;
@@ -141,7 +190,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<void*>(this));
}

mService = nullptr;
diff --git a/media/libmediatranscoding/include/media/TranscodingResourcePolicy.h b/media/libmediatranscoding/include/media/TranscodingResourcePolicy.h
index ee232e7551..4d762b5832 100644
--- a/media/libmediatranscoding/include/media/TranscodingResourcePolicy.h
+++ b/media/libmediatranscoding/include/media/TranscodingResourcePolicy.h
@@ -22,6 +22,7 @@
#include <utils/Condition.h>

#include <mutex>
+#include <set>
namespace aidl {
namespace android {
namespace media {
@@ -48,6 +49,8 @@ private:
bool mRegistered GUARDED_BY(mRegisteredLock);
std::shared_ptr<IResourceObserverService> mService GUARDED_BY(mRegisteredLock);
std::shared_ptr<ResourceObserver> mObserver;
+ mutable std::mutex mCookieKeysLock;
+ std::set<uintptr_t> mCookieKeys;

mutable std::mutex mCallbackLock;
std::weak_ptr<ResourcePolicyCallbackInterface> 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
--
2.45.2.505.gda0bf45e8d-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
From 999f881dd3fb1c6396299cb65b9817fffe23e2dd Mon Sep 17 00:00:00 2001
From: Rakesh Kumar <[email protected]>
Date: Thu, 30 May 2024 11:17:48 +0000
Subject: [PATCH] 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 ea1fdf4bf8..e2e77ee367 100644
--- a/media/libmediaplayerservice/StagefrightRecorder.cpp
+++ b/media/libmediaplayerservice/StagefrightRecorder.cpp
@@ -2075,6 +2075,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));
--
2.45.2.505.gda0bf45e8d-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
From a7245613009e19d83a43c2bad50beba58145c920 Mon Sep 17 00:00:00 2001
From: Yi-an Chen <[email protected]>
Date: Wed, 21 Feb 2024 01:56:22 +0000
Subject: [PATCH] Fix error handling for non-dynamic permissions

We only allow removing dynamic permissions. When removePermission() is
called for a non-dynamic permission, in addition to logging it, we
should also return early to avoid the removePermission() call.

Test: manual
Bug: 321555066
Fixes: 321711213
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:528a87e90ff9354581d54fd37fbe9f95cccbcdb1)
Merged-In: Ie2f43663bc71a06ffadb868d2d0eea5ee78f76e5
Change-Id: Ie2f43663bc71a06ffadb868d2d0eea5ee78f76e5
---
.../server/pm/permission/PermissionManagerServiceImpl.java | 1 +
1 file changed, 1 insertion(+)

diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
index 5eff14f24f77..f2834fd8b997 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
@@ -678,6 +678,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt
// TODO: switch this back to SecurityException
Slog.wtf(TAG, "Not allowed to modify non-dynamic permission "
+ permName);
+ return;
}
mRegistry.removePermission(permName);
}
--
2.45.2.505.gda0bf45e8d-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
From 10aaf9ae5b6e30d20de62ecaa6df5b790f1b13bc Mon Sep 17 00:00:00 2001
From: Yi-an Chen <[email protected]>
Date: Tue, 23 Apr 2024 21:17:44 +0000
Subject: [PATCH] Fix security vulnerability of non-dynamic permission removal

The original removePermission() code in PermissionManagerServiceImpl
missed a logical negation operator when handling non-dynamic
permissions, causing both
testPermissionPermission_nonDynamicPermission_permissionUnchanged and
testRemovePermission_dynamicPermission_permissionRemoved tests in
DynamicPermissionsTest to fail.

The corresponding test DynamicPermissionsTest is also updated in the
other CL: ag/27073864

Bug: 321711213
Test: DynamicPermissionsTest on sc-dev and tm-dev locally
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0ead58f69f5de82b00406316b333366d556239f1)
Merged-In: Ia146d4098643d9c473f8c83d33a8a125a53101fc
Change-Id: Ia146d4098643d9c473f8c83d33a8a125a53101fc
---
.../server/pm/permission/PermissionManagerServiceImpl.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
index f2834fd8b997..f43f5f6b3935 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java
@@ -674,7 +674,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt
if (bp == null) {
return;
}
- if (bp.isDynamic()) {
+ if (!bp.isDynamic()) {
// TODO: switch this back to SecurityException
Slog.wtf(TAG, "Not allowed to modify non-dynamic permission "
+ permName);
--
2.45.2.505.gda0bf45e8d-goog

Loading
Loading