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-Aug 2024 Security Patches integration #2543

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 0daae6bdcb..d14bd65167 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 := 2022-06-05
+ PLATFORM_SECURITY_PATCH := 2024-07-01
+ PLATFORM_SECURITY_PATCH := 2024-08-01
endif
.KATI_READONLY := PLATFORM_SECURITY_PATCH

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
From 07df1d8dd95fb6af803f6f44905f2d473bdf7efa 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 73d11ac20f91aff263e45555a85643884d656714 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 bffd7b3243..209b1f8036 100644
--- a/media/libmediaplayerservice/StagefrightRecorder.cpp
+++ b/media/libmediaplayerservice/StagefrightRecorder.cpp
@@ -2070,6 +2070,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 57ba37a9fc7f118d97d2e4ac7efc5a41d04f2501 Mon Sep 17 00:00:00 2001
From: Yi-an Chen <[email protected]>
Date: Tue, 20 Feb 2024 04:34:57 +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:2b5d63b64b2b8208ccc4f62eac3d8962f981dbf8)
Merged-In: I7336f2fc78804f26e4b2a329870ecdea776595d8
Change-Id: I7336f2fc78804f26e4b2a329870ecdea776595d8
---
.../android/server/pm/permission/PermissionManagerService.java | 1 +
1 file changed, 1 insertion(+)

diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
index dd8b96eab3d7..31babe0418b8 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
@@ -691,6 +691,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
// 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 58a0ab5bc7abaa174187a1548982fd74ff18f00d Mon Sep 17 00:00:00 2001
From: Yi-an Chen <[email protected]>
Date: Tue, 23 Apr 2024 21:53:02 +0000
Subject: [PATCH] Fix security vulnerability of non-dynamic permission removal

The original removePermission() code in PermissionManagerService
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:35d77a77feef62dc108f6478cb9228cc6044f70d)
Merged-In: Id573b75cdcfce3a1df5731ffb00c4228c513e686
Change-Id: Id573b75cdcfce3a1df5731ffb00c4228c513e686
---
.../android/server/pm/permission/PermissionManagerService.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
index 31babe0418b8..93f9e1c2295c 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
@@ -687,7 +687,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
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