libbinder: stability check moved to trans time
Before: stability check done when binder is read from a parcel
After: stability check done when binder is transacted on
Why this change is being made/benefits:
- vendor binders can be used as tokens in system context
- pingBinder/interfaceChain/etc.. can be done on vendor binders in a
system context, so code can generically operate on binders. This is
particularly useful for service manager and dumpstate, which previously
I was going to special-case
- policy on which binders go where is entirely reliant on SELinux
whereas before there were additional runtime restrictions
Cons to this change:
- allowed binders must be determined by context. BpBinder now checks
stability based on kLocalStability. More work would need to be done to
get this working with APEX.
Bug: 136027762
Test: binderStabilityTest
Change-Id: Iff026e81a130dbb8885ca82ec24e69a5768847eb
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 268c85e..f3483ca 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -124,7 +124,6 @@
uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags)
{
data.setDataPosition(0);
- data.setTransactingBinder(this);
status_t err = NO_ERROR;
switch (code) {
@@ -139,7 +138,6 @@
// In case this is being transacted on in the same process.
if (reply != nullptr) {
reply->setDataPosition(0);
- reply->setTransactingBinder(this);
}
return err;
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 57440d5..74ffde2 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -21,6 +21,7 @@
#include <binder/IPCThreadState.h>
#include <binder/IResultReceiver.h>
+#include <binder/Stability.h>
#include <cutils/compiler.h>
#include <utils/Log.h>
@@ -213,14 +214,21 @@
{
// Once a binder has died, it will never come back to life.
if (mAlive) {
+ // user transactions require a given stability level
+ if (code >= FIRST_CALL_TRANSACTION && code <= LAST_CALL_TRANSACTION) {
+ using android::internal::Stability;
+
+ auto stability = Stability::get(this);
+
+ if (CC_UNLIKELY(!Stability::check(stability, Stability::kLocalStability))) {
+ return BAD_TYPE;
+ }
+ }
+
status_t status = IPCThreadState::self()->transact(
mHandle, code, data, reply, flags);
if (status == DEAD_OBJECT) mAlive = 0;
- if (reply != nullptr) {
- reply->setTransactingBinder(this);
- }
-
return status;
}
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 82053ad..8751ecb 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -35,6 +35,7 @@
#include <binder/IPCThreadState.h>
#include <binder/Parcel.h>
#include <binder/ProcessState.h>
+#include <binder/Stability.h>
#include <binder/Status.h>
#include <binder/TextOutput.h>
@@ -170,11 +171,10 @@
status_t Parcel::finishFlattenBinder(
const sp<IBinder>& binder, const flat_binder_object& flat)
{
- internal::Stability::tryMarkCompilationUnit(binder.get());
-
status_t status = writeObject(flat, false);
if (status != OK) return status;
+ internal::Stability::tryMarkCompilationUnit(binder.get());
return writeInt32(internal::Stability::get(binder.get()));
}
@@ -185,10 +185,6 @@
status_t status = readInt32(&stability);
if (status != OK) return status;
- if (binder != nullptr && !internal::Stability::check(stability, mRequiredStability)) {
- return BAD_TYPE;
- }
-
status = internal::Stability::set(binder.get(), stability, true /*log*/);
if (status != OK) return status;
@@ -356,10 +352,6 @@
return NO_ERROR;
}
-void Parcel::setTransactingBinder(const sp<IBinder>& binder) const {
- mRequiredStability = internal::Stability::get(binder.get());
-}
-
status_t Parcel::setData(const uint8_t* buffer, size_t len)
{
if (len > INT32_MAX) {
@@ -2705,10 +2697,9 @@
mObjectsCapacity = 0;
mNextObjectHint = 0;
mObjectsSorted = false;
- mAllowFds = true;
mHasFds = false;
mFdsKnown = true;
- mRequiredStability = internal::Stability::UNDECLARED;
+ mAllowFds = true;
mOwner = nullptr;
mOpenAshmemSize = 0;
mWorkSourceRequestHeaderPosition = 0;
diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp
index 1e1bc3a..07db50f 100644
--- a/libs/binder/ProcessState.cpp
+++ b/libs/binder/ProcessState.cpp
@@ -21,6 +21,7 @@
#include <binder/BpBinder.h>
#include <binder/IPCThreadState.h>
#include <binder/IServiceManager.h>
+#include <binder/Stability.h>
#include <cutils/atomic.h>
#include <utils/Log.h>
#include <utils/String8.h>
@@ -109,7 +110,13 @@
sp<IBinder> ProcessState::getContextObject(const sp<IBinder>& /*caller*/)
{
- return getStrongProxyForHandle(0);
+ sp<IBinder> context = getStrongProxyForHandle(0);
+
+ // The root object is special since we get it directly from the driver, it is never
+ // written by Parcell::writeStrongBinder.
+ internal::Stability::tryMarkCompilationUnit(context.get());
+
+ return context;
}
void ProcessState::startThreadPool()
diff --git a/libs/binder/Stability.cpp b/libs/binder/Stability.cpp
index 0a10a1d..b6f10c8 100644
--- a/libs/binder/Stability.cpp
+++ b/libs/binder/Stability.cpp
@@ -32,6 +32,11 @@
ALOGE("%s: stability is %s", tag.c_str(), stabilityString(get(binder.get())).c_str());
}
+void Stability::markVndk(IBinder* binder) {
+ status_t result = set(binder, Level::VENDOR, true /*log*/);
+ LOG_ALWAYS_FATAL_IF(result != OK, "Should only mark known object.");
+}
+
void Stability::tryMarkCompilationUnit(IBinder* binder) {
(void) set(binder, kLocalStability, false /*log*/);
}
@@ -95,9 +100,9 @@
}
if (!stable) {
- ALOGE("Interface with %s cannot accept interface with %s.",
- stabilityString(required).c_str(),
- stabilityString(provided).c_str());
+ ALOGE("Cannot do a user transaction on a %s binder in a %s context.",
+ stabilityString(provided).c_str(),
+ stabilityString(required).c_str());
}
return stable;
diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h
index 4e5f1aa..50ca983 100644
--- a/libs/binder/include/binder/Parcel.h
+++ b/libs/binder/include/binder/Parcel.h
@@ -33,7 +33,6 @@
#include <binder/IInterface.h>
#include <binder/Parcelable.h>
-#include <binder/Stability.h>
// ---------------------------------------------------------------------------
namespace android {
@@ -69,8 +68,6 @@
void setDataPosition(size_t pos) const;
status_t setDataCapacity(size_t size);
- void setTransactingBinder(const sp<IBinder>& binder) const;
-
status_t setData(const uint8_t* buffer, size_t len);
status_t appendFrom(const Parcel *parcel,
@@ -475,15 +472,13 @@
size_t mObjectsCapacity;
mutable size_t mNextObjectHint;
mutable bool mObjectsSorted;
- bool mAllowFds;
mutable bool mRequestHeaderPresent;
mutable size_t mWorkSourceRequestHeaderPosition;
mutable bool mFdsKnown;
mutable bool mHasFds;
-
- mutable internal::Stability::Level mRequiredStability;
+ bool mAllowFds;
release_func mOwner;
void* mOwnerCookie;
diff --git a/libs/binder/include/binder/Stability.h b/libs/binder/include/binder/Stability.h
index 9d98c7f..f8240e4 100644
--- a/libs/binder/include/binder/Stability.h
+++ b/libs/binder/include/binder/Stability.h
@@ -20,6 +20,10 @@
#include <string>
namespace android {
+
+class BpBinder;
+class ProcessState;
+
namespace internal {
// WARNING: These APIs are only ever expected to be called by auto-generated code.
@@ -43,14 +47,30 @@
// WARNING: for debugging only
static void debugLogStability(const std::string& tag, const sp<IBinder>& binder);
+ // WARNING: This is only ever expected to be called by auto-generated code or tests.
+ // You likely want to change or modify the stability of the interface you are using.
+ // This must be called as soon as the binder in question is constructed. No thread safety
+ // is provided.
+ // E.g. stability is according to libbinder_ndk or Java SDK AND the interface
+ // expressed here is guaranteed to be stable for multiple years (Stable AIDL)
+ // If this is called when __ANDROID_VNDK__ is not defined, then it is UB and will likely
+ // break the device during GSI or other tests.
+ static void markVndk(IBinder* binder);
+
private:
- // Parcel needs to store stability level since this is more efficient than storing and looking
- // up the efficiency level of a binder object. So, we expose the underlying type.
+ // Parcel needs to read/write stability level in an unstable format.
friend ::android::Parcel;
+ // only expose internal APIs inside of libbinder, for checking stability
+ friend ::android::BpBinder;
+
+ // so that it can mark the context object (only the root object doesn't go
+ // through Parcel)
+ friend ::android::ProcessState;
+
static void tryMarkCompilationUnit(IBinder* binder);
- enum Level : int16_t {
+ enum Level : int32_t {
UNDECLARED = 0,
VENDOR = 0b000011,
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index 05db81e..724cb15 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -144,6 +144,7 @@
srcs: [
"binderStabilityTest.cpp",
"IBinderStabilityTest.aidl",
+ "IBinderStabilityTestSub.aidl",
],
shared_libs: [
diff --git a/libs/binder/tests/IBinderStabilityTest.aidl b/libs/binder/tests/IBinderStabilityTest.aidl
index 7540ec9..9559196 100644
--- a/libs/binder/tests/IBinderStabilityTest.aidl
+++ b/libs/binder/tests/IBinderStabilityTest.aidl
@@ -14,24 +14,34 @@
* limitations under the License.
*/
+import IBinderStabilityTestSub;
+
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
interface IBinderStabilityTest {
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
- void sendBinder(IBinder binder);
+ void sendBinder(IBinderStabilityTestSub binder);
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
- IBinder returnNoStabilityBinder();
+ void sendAndCallBinder(IBinderStabilityTestSub binder);
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
- IBinder returnLocalStabilityBinder();
+ IBinderStabilityTestSub returnNoStabilityBinder();
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
- IBinder returnVintfStabilityBinder();
+ IBinderStabilityTestSub returnLocalStabilityBinder();
+
+ // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
+ // THIS IS ONLY FOR TESTING!
+ IBinderStabilityTestSub returnVintfStabilityBinder();
+
+ // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
+ // THIS IS ONLY FOR TESTING!
+ IBinderStabilityTestSub returnVendorStabilityBinder();
}
// DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS!
// THIS IS ONLY FOR TESTING!
diff --git a/libs/binder/tests/IBinderStabilityTestSub.aidl b/libs/binder/tests/IBinderStabilityTestSub.aidl
new file mode 100644
index 0000000..a81ebf9
--- /dev/null
+++ b/libs/binder/tests/IBinderStabilityTestSub.aidl
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+interface IBinderStabilityTestSub {
+ void userDefinedTransaction();
+}
diff --git a/libs/binder/tests/binderStabilityTest.cpp b/libs/binder/tests/binderStabilityTest.cpp
index 0218b94..ecd4f0d 100644
--- a/libs/binder/tests/binderStabilityTest.cpp
+++ b/libs/binder/tests/binderStabilityTest.cpp
@@ -25,6 +25,7 @@
#include <sys/prctl.h>
+#include "BnBinderStabilityTestSub.h"
#include "BnBinderStabilityTest.h"
#include "BpBinderStabilityTest.h"
@@ -36,20 +37,34 @@
const String16 kCompilationUnitServer = String16("binder_stability_test_service_compl");
const String16 kVintfServer = String16("binder_stability_test_service_vintf");
-sp<IBinder> getCompilationUnitStability() {
- sp<IBinder> binder = new BBinder();
+class BadStabilityTestSub : public BnBinderStabilityTestSub {
+ Status userDefinedTransaction() {
+ return Status::ok();
+ }
+};
+
+sp<IBinderStabilityTestSub> getCompilationUnitStability() {
+ sp<BnBinderStabilityTestSub> iface = new BadStabilityTestSub();
// NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS?
// WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS
- internal::Stability::markCompilationUnit(binder.get()); // <- BAD, NO! DO NOT COPY
- return binder;
+ internal::Stability::markCompilationUnit(iface.get()); // <- BAD, NO! DO NOT COPY
+ return iface;
}
-sp<IBinder> getVintfStability() {
- sp<IBinder> binder = new BBinder();
+sp<IBinderStabilityTestSub> getVintfStability() {
+ sp<BnBinderStabilityTestSub> iface = new BadStabilityTestSub();
// NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS?
// WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS
- internal::Stability::markVintf(binder.get()); // <- BAD, NO! DO NOT COPY
- return binder;
+ internal::Stability::markVintf(iface.get()); // <- BAD, NO! DO NOT COPY
+ return iface;
+}
+
+sp<IBinderStabilityTestSub> getVendorStability() {
+ sp<BnBinderStabilityTestSub> iface = new BadStabilityTestSub();
+ // NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS?
+ // WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS
+ internal::Stability::markVndk(iface.get()); // <- BAD, NO! DO NOT COPY
+ return iface;
}
// NO! NO! NO! Do not even think of doing something like this!
@@ -57,92 +72,67 @@
// it would ruin everything!
class BadStabilityTester : public BnBinderStabilityTest {
public:
- Status sendBinder(const sp<IBinder>& /*binder*/) override {
+ Status sendBinder(const sp<IBinderStabilityTestSub>& /*binder*/) override {
return Status::ok();
}
- Status returnNoStabilityBinder(sp<IBinder>* _aidl_return) override {
- *_aidl_return = new BBinder();
+ Status sendAndCallBinder(const sp<IBinderStabilityTestSub>& binder) override {
+ return binder->userDefinedTransaction();
+ }
+ Status returnNoStabilityBinder(sp<IBinderStabilityTestSub>* _aidl_return) override {
+ *_aidl_return = new BadStabilityTestSub();
return Status::ok();
}
- Status returnLocalStabilityBinder(sp<IBinder>* _aidl_return) override {
+ Status returnLocalStabilityBinder(sp<IBinderStabilityTestSub>* _aidl_return) override {
*_aidl_return = getCompilationUnitStability();
return Status::ok();
}
- Status returnVintfStabilityBinder(sp<IBinder>* _aidl_return) override {
+ Status returnVintfStabilityBinder(sp<IBinderStabilityTestSub>* _aidl_return) override {
*_aidl_return = getVintfStability();
return Status::ok();
}
-
- static sp<IBinderStabilityTest> getNoStabilityServer() {
- sp<IBinder> remote = new BadStabilityTester;
- return new BpBinderStabilityTest(remote);
- }
- static sp<IBinderStabilityTest> getCompilationUnitStabilityServer() {
- sp<IBinder> remote = new BadStabilityTester;
- internal::Stability::markCompilationUnit(remote.get());
- return new BpBinderStabilityTest(remote);
- }
- static sp<IBinderStabilityTest> getVintfStabilityServer() {
- sp<IBinder> remote = new BadStabilityTester;
- internal::Stability::markVintf(remote.get()); // <- BAD, NO! DO NOT COPY
- return new BpBinderStabilityTest(remote);
+ Status returnVendorStabilityBinder(sp<IBinderStabilityTestSub>* _aidl_return) override {
+ *_aidl_return = getVendorStability();
+ return Status::ok();
}
};
-void checkLocalStabilityBinder(const sp<IBinderStabilityTest>& complServer) {
- // this binder should automatically be set to local stability
- EXPECT_TRUE(complServer->sendBinder(new BBinder()).isOk());
+void checkSystemStabilityBinder(const sp<IBinderStabilityTest>& complServer) {
+ EXPECT_TRUE(complServer->sendBinder(new BadStabilityTestSub()).isOk());
EXPECT_TRUE(complServer->sendBinder(getCompilationUnitStability()).isOk());
EXPECT_TRUE(complServer->sendBinder(getVintfStability()).isOk());
+ EXPECT_TRUE(complServer->sendBinder(getVendorStability()).isOk());
- sp<IBinder> out;
- // should automatically be set to local stability
+ EXPECT_TRUE(complServer->sendAndCallBinder(new BadStabilityTestSub()).isOk());
+ EXPECT_TRUE(complServer->sendAndCallBinder(getCompilationUnitStability()).isOk());
+ EXPECT_TRUE(complServer->sendAndCallBinder(getVintfStability()).isOk());
+
+ // !!! user-defined transaction may not be stable for remote server !!!
+ EXPECT_FALSE(complServer->sendAndCallBinder(getVendorStability()).isOk());
+
+ sp<IBinderStabilityTestSub> out;
EXPECT_TRUE(complServer->returnNoStabilityBinder(&out).isOk());
- EXPECT_NE(nullptr, out.get());
+ ASSERT_NE(nullptr, out.get());
+ EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder());
+ EXPECT_TRUE(out->userDefinedTransaction().isOk());
EXPECT_TRUE(complServer->returnLocalStabilityBinder(&out).isOk());
- EXPECT_NE(nullptr, out.get());
+ ASSERT_NE(nullptr, out.get());
+ EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder());
+ EXPECT_TRUE(out->userDefinedTransaction().isOk());
EXPECT_TRUE(complServer->returnVintfStabilityBinder(&out).isOk());
- EXPECT_NE(nullptr, out.get());
-}
+ ASSERT_NE(nullptr, out.get());
+ EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder());
+ EXPECT_TRUE(out->userDefinedTransaction().isOk());
-void checkHighStabilityServer(const sp<IBinderStabilityTest>& highStability) {
- EXPECT_FALSE(highStability->sendBinder(new BBinder()).isOk());
- EXPECT_FALSE(highStability->sendBinder(getCompilationUnitStability()).isOk());
- EXPECT_TRUE(highStability->sendBinder(getVintfStability()).isOk());
+ EXPECT_TRUE(complServer->returnVendorStabilityBinder(&out).isOk());
+ ASSERT_NE(nullptr, out.get());
- sp<IBinder> out;
- EXPECT_FALSE(highStability->returnNoStabilityBinder(&out).isOk());
- EXPECT_EQ(nullptr, out.get());
+ // !!! libbinder-defined transaction works !!!
+ EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder());
- EXPECT_FALSE(highStability->returnLocalStabilityBinder(&out).isOk());
- EXPECT_EQ(nullptr, out.get());
-
- EXPECT_TRUE(highStability->returnVintfStabilityBinder(&out).isOk());
- EXPECT_NE(nullptr, out.get());
-}
-
-TEST(BinderStability, LocalNoStabilityServer) {
- // in practice, a low stability server is probably one that hasn't been rebuilt
- // or was written by hand.
- auto server = BadStabilityTester::getNoStabilityServer();
- ASSERT_NE(nullptr, IInterface::asBinder(server)->localBinder());
-
- // it should be considered to have local stability
- checkLocalStabilityBinder(server);
-}
-
-TEST(BinderStability, LocalLowStabilityServer) {
- auto server = BadStabilityTester::getCompilationUnitStabilityServer();
- ASSERT_NE(nullptr, IInterface::asBinder(server)->localBinder());
- checkLocalStabilityBinder(server);
-}
-
-TEST(BinderStability, LocalHighStabilityServer) {
- auto server = BadStabilityTester::getVintfStabilityServer();
- ASSERT_NE(nullptr, IInterface::asBinder(server)->localBinder());
- checkHighStabilityServer(server);
+ // !!! user-defined transaction may not be stable !!!
+ EXPECT_FALSE(out->userDefinedTransaction().isOk());
}
TEST(BinderStability, RemoteNoStabilityServer) {
@@ -152,8 +142,7 @@
ASSERT_NE(nullptr, remoteServer.get());
ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder());
- // it should be considered to have local stability
- checkLocalStabilityBinder(remoteServer);
+ checkSystemStabilityBinder(remoteServer);
}
TEST(BinderStability, RemoteLowStabilityServer) {
@@ -163,7 +152,7 @@
ASSERT_NE(nullptr, remoteServer.get());
ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder());
- checkLocalStabilityBinder(remoteServer);
+ checkSystemStabilityBinder(remoteServer);
}
TEST(BinderStability, RemoteVintfServer) {
@@ -173,7 +162,7 @@
ASSERT_NE(nullptr, remoteServer.get());
ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder());
- checkHighStabilityServer(remoteServer);
+ checkSystemStabilityBinder(remoteServer);
}
class MarksStabilityInConstructor : public BBinder {