From b6663f9d82ca092e4b77664012e84f67409f7b4d Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 26 Oct 2022 00:37:45 +0000 Subject: [PATCH 1/2] Parcel: lift 'isForRpc' here from native Platform API, in order to fix bugs like b/254835230 where certain utilities call getCallingUid, which doesn't make sense for RPC calls. Bug: 254835230 Test: android.os.ParcelTest Change-Id: If2dd13d8edd515c945ccde2a2554f6f2b7b87b26 --- core/java/android/os/Parcel.java | 11 +++++++++++ core/jni/android_os_Parcel.cpp | 7 +++++++ core/tests/coretests/src/android/os/ParcelTest.java | 7 +++++++ 3 files changed, 25 insertions(+) diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index a6ae663dabb0b..8ffb94b6ffd12 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -368,6 +368,8 @@ public final class Parcel { @FastNative private static native void nativeMarkForBinder(long nativePtr, IBinder binder); @CriticalNative + private static native boolean nativeIsForRpc(long nativePtr); + @CriticalNative private static native int nativeDataSize(long nativePtr); @CriticalNative private static native int nativeDataAvail(long nativePtr); @@ -645,6 +647,15 @@ public final class Parcel { nativeMarkForBinder(mNativePtr, binder); } + /** + * Whether this Parcel is written for an RPC transaction. + * + * @hide + */ + public final boolean isForRpc() { + return nativeIsForRpc(mNativePtr); + } + /** @hide */ @ParcelFlags @TestApi diff --git a/core/jni/android_os_Parcel.cpp b/core/jni/android_os_Parcel.cpp index 1f64df49cb569..4d8dac1daaf0e 100644 --- a/core/jni/android_os_Parcel.cpp +++ b/core/jni/android_os_Parcel.cpp @@ -116,6 +116,11 @@ static void android_os_Parcel_markForBinder(JNIEnv* env, jclass clazz, jlong nat } } +static jboolean android_os_Parcel_isForRpc(jlong nativePtr) { + Parcel* parcel = reinterpret_cast(nativePtr); + return parcel ? parcel->isForRpc() : false; +} + static jint android_os_Parcel_dataSize(jlong nativePtr) { Parcel* parcel = reinterpret_cast(nativePtr); @@ -808,6 +813,8 @@ static const JNINativeMethod gParcelMethods[] = { // @FastNative {"nativeMarkForBinder", "(JLandroid/os/IBinder;)V", (void*)android_os_Parcel_markForBinder}, // @CriticalNative + {"nativeIsForRpc", "(J)Z", (void*)android_os_Parcel_isForRpc}, + // @CriticalNative {"nativeDataSize", "(J)I", (void*)android_os_Parcel_dataSize}, // @CriticalNative {"nativeDataAvail", "(J)I", (void*)android_os_Parcel_dataAvail}, diff --git a/core/tests/coretests/src/android/os/ParcelTest.java b/core/tests/coretests/src/android/os/ParcelTest.java index fdd278b9c6217..e2fe87b4cfe32 100644 --- a/core/tests/coretests/src/android/os/ParcelTest.java +++ b/core/tests/coretests/src/android/os/ParcelTest.java @@ -36,6 +36,13 @@ public class ParcelTest { private static final String INTERFACE_TOKEN_1 = "IBinder interface token"; private static final String INTERFACE_TOKEN_2 = "Another IBinder interface token"; + @Test + public void testIsForRpc() { + Parcel p = Parcel.obtain(); + assertEquals(false, p.isForRpc()); + p.recycle(); + } + @Test public void testCallingWorkSourceUidAfterWrite() { Parcel p = Parcel.obtain(); From b95831cbd9c8cd24b33e9a947055c10b4524eff8 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 26 Oct 2022 01:10:28 +0000 Subject: [PATCH 2/2] Binder: don't call getCallingUid for RPC calls All the code using getCallingUid here should be ripped out anyway because it's Java-centric. Here are some of the issues with the current stuff around here: - it's broken because it won't handle things happening in native code - it's slow (e.g. big transactions are already logged in native code anyway) Bug: 254835230 Test: boots, atest MicrodroidTestApp (with new tests) Change-Id: I7315059bff138966e9509f1d529a6417466d2b7f --- core/java/android/os/Binder.java | 40 +++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java index 2f4b2c411c1d6..d54c641bd07db 100644 --- a/core/java/android/os/Binder.java +++ b/core/java/android/os/Binder.java @@ -1219,25 +1219,40 @@ public class Binder implements IBinder { @UnsupportedAppUsage private boolean execTransact(int code, long dataObj, long replyObj, int flags) { + + Parcel data = Parcel.obtain(dataObj); + Parcel reply = Parcel.obtain(replyObj); + // At that point, the parcel request headers haven't been parsed so we do not know what // {@link WorkSource} the caller has set. Use calling UID as the default. - final int callingUid = Binder.getCallingUid(); - final long origWorkSource = ThreadLocalWorkSource.setUid(callingUid); + // + // TODO: this is wrong - we should attribute along the entire call route + // also this attribution logic should move to native code - it only works + // for Java now + // + // This attribution support is not generic and therefore not support in RPC mode + final int callingUid = data.isForRpc() ? -1 : Binder.getCallingUid(); + final long origWorkSource = callingUid == -1 + ? -1 : ThreadLocalWorkSource.setUid(callingUid); + try { - return execTransactInternal(code, dataObj, replyObj, flags, callingUid); + return execTransactInternal(code, data, reply, flags, callingUid); } finally { - ThreadLocalWorkSource.restore(origWorkSource); + reply.recycle(); + data.recycle(); + + if (callingUid != -1) { + ThreadLocalWorkSource.restore(origWorkSource); + } } } - private boolean execTransactInternal(int code, long dataObj, long replyObj, int flags, + private boolean execTransactInternal(int code, Parcel data, Parcel reply, int flags, int callingUid) { // Make sure the observer won't change while processing a transaction. final BinderInternal.Observer observer = sObserver; final CallSession callSession = observer != null ? observer.callStarted(this, code, UNSET_WORKSOURCE) : null; - Parcel data = Parcel.obtain(dataObj); - Parcel reply = Parcel.obtain(replyObj); // Theoretically, we should call transact, which will call onTransact, // but all that does is rewind it, and we just got these from an IPC, // so we'll just call it directly. @@ -1268,8 +1283,10 @@ public class Binder implements IBinder { final boolean tracingEnabled = tagEnabled && transactionTraceName != null; try { + // TODO - this logic should not be in Java - it should be in native + // code in libbinder so that it works for all binder users. final BinderCallHeavyHitterWatcher heavyHitterWatcher = sHeavyHitterWatcher; - if (heavyHitterWatcher != null) { + if (heavyHitterWatcher != null && callingUid != -1) { // Notify the heavy hitter watcher, if it's enabled. heavyHitterWatcher.onTransaction(callingUid, getClass(), code); } @@ -1277,7 +1294,10 @@ public class Binder implements IBinder { Trace.traceBegin(Trace.TRACE_TAG_AIDL, transactionTraceName); } - if ((flags & FLAG_COLLECT_NOTED_APP_OPS) != 0) { + // TODO - this logic should not be in Java - it should be in native + // code in libbinder so that it works for all binder users. Further, + // this should not re-use flags. + if ((flags & FLAG_COLLECT_NOTED_APP_OPS) != 0 && callingUid != -1) { AppOpsManager.startNotedAppOpsCollection(callingUid); try { res = onTransact(code, data, reply, flags); @@ -1320,8 +1340,6 @@ public class Binder implements IBinder { } checkParcel(this, code, reply, "Unreasonably large binder reply buffer"); - reply.recycle(); - data.recycle(); } // Just in case -- we are done with the IPC, so there should be no more strict