From 19fc9f7991f27fd75b6a1d872e91d7377b89401d Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Thu, 10 Jun 2021 03:57:30 +0000 Subject: libbinder: RPC disambiguate server/client Now: - server: RpcServer - client: a client of an RpcServer - incoming: a thread processing commands (either as part of an RpcServer's sessions or back to a client which has a threadpool) - outgoing: a thread for sending commands (either to an RpcServer's sessions or back to a client which has a threadpool) Bug: 167966510 Test: binderRpcTest Change-Id: Iea286ab0ff6f9fb775994247003b8d29c999e10a --- libs/binder/RpcServer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'libs/binder/RpcServer.cpp') diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp index 60be406f6f..ad377d3737 100644 --- a/libs/binder/RpcServer.cpp +++ b/libs/binder/RpcServer.cpp @@ -301,7 +301,7 @@ void RpcServer::establishConnection(sp&& server, base::unique_fd clie } if (reverse) { - LOG_ALWAYS_FATAL_IF(!session->addClientConnection(std::move(clientFd)), + LOG_ALWAYS_FATAL_IF(!session->addOutgoingConnection(std::move(clientFd)), "server state must already be initialized"); return; } @@ -350,7 +350,7 @@ bool RpcServer::setupSocketServer(const RpcSocketAddress& addr) { return true; } -void RpcServer::onSessionLockedAllServerThreadsEnded(const sp& session) { +void RpcServer::onSessionLockedAllIncomingThreadsEnded(const sp& session) { auto id = session->mId; LOG_ALWAYS_FATAL_IF(id == std::nullopt, "Server sessions must be initialized with ID"); LOG_RPC_DETAIL("Dropping session %d", *id); @@ -362,7 +362,7 @@ void RpcServer::onSessionLockedAllServerThreadsEnded(const sp& sessi (void)mSessions.erase(it); } -void RpcServer::onSessionServerThreadEnded() { +void RpcServer::onSessionIncomingThreadEnded() { mShutdownCv.notify_all(); } -- cgit v1.2.3-59-g8ed1b From 01a6bad2e1441c4ec89d6157dc663cb43c6d9cf9 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Fri, 11 Jun 2021 00:59:20 +0000 Subject: libbinder: RPC session ID uses the long binder ID This is 'unguessable' (pending security review and constant time compare). Right now, it's unclear if we'll go with full TLS for on-device communication or use some other authentication scheme. However, this is being used similarly to TLS session tickets. Bug: 167966510 Test: binderRpcTest Change-Id: I4c5edd2de6cc3f6ae37b0815e7f45c7a08bac2b1 --- libs/binder/RpcServer.cpp | 34 +++++++++++++++++++++++---------- libs/binder/RpcSession.cpp | 22 ++++++++++----------- libs/binder/RpcState.cpp | 15 +++++---------- libs/binder/RpcState.h | 2 +- libs/binder/RpcWireFormat.h | 22 ++++++++++----------- libs/binder/include/binder/RpcAddress.h | 6 +----- libs/binder/include/binder/RpcServer.h | 4 ++-- libs/binder/include/binder/RpcSession.h | 9 ++++----- 8 files changed, 58 insertions(+), 56 deletions(-) (limited to 'libs/binder/RpcServer.cpp') diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp index ad377d3737..3599427f5a 100644 --- a/libs/binder/RpcServer.cpp +++ b/libs/binder/RpcServer.cpp @@ -270,14 +270,25 @@ void RpcServer::establishConnection(sp&& server, base::unique_fd clie return; } - if (header.sessionId == RPC_SESSION_ID_NEW) { + RpcAddress sessionId = RpcAddress::fromRawEmbedded(&header.sessionId); + + if (sessionId.isZero()) { if (reverse) { ALOGE("Cannot create a new session with a reverse connection, would leak"); return; } - LOG_ALWAYS_FATAL_IF(server->mSessionIdCounter >= INT32_MAX, "Out of session IDs"); - server->mSessionIdCounter++; + RpcAddress sessionId = RpcAddress::zero(); + size_t tries = 0; + do { + // don't block if there is some entropy issue + if (tries++ > 5) { + ALOGE("Cannot find new address: %s", sessionId.toString().c_str()); + return; + } + + sessionId = RpcAddress::random(true /*forServer*/); + } while (server->mSessions.end() != server->mSessions.find(sessionId)); session = RpcSession::make(); session->setMaxThreads(server->mMaxThreads); @@ -285,16 +296,17 @@ void RpcServer::establishConnection(sp&& server, base::unique_fd clie sp::fromExisting( static_cast( server.get())), - server->mSessionIdCounter)) { + sessionId)) { ALOGE("Failed to attach server to session"); return; } - server->mSessions[server->mSessionIdCounter] = session; + server->mSessions[sessionId] = session; } else { - auto it = server->mSessions.find(header.sessionId); + auto it = server->mSessions.find(sessionId); if (it == server->mSessions.end()) { - ALOGE("Cannot add thread, no record of session with ID %d", header.sessionId); + ALOGE("Cannot add thread, no record of session with ID %s", + sessionId.toString().c_str()); return; } session = it->second; @@ -353,12 +365,14 @@ bool RpcServer::setupSocketServer(const RpcSocketAddress& addr) { void RpcServer::onSessionLockedAllIncomingThreadsEnded(const sp& session) { auto id = session->mId; LOG_ALWAYS_FATAL_IF(id == std::nullopt, "Server sessions must be initialized with ID"); - LOG_RPC_DETAIL("Dropping session %d", *id); + LOG_RPC_DETAIL("Dropping session with address %s", id->toString().c_str()); std::lock_guard _l(mLock); auto it = mSessions.find(*id); - LOG_ALWAYS_FATAL_IF(it == mSessions.end(), "Bad state, unknown session id %d", *id); - LOG_ALWAYS_FATAL_IF(it->second != session, "Bad state, session has id mismatch %d", *id); + LOG_ALWAYS_FATAL_IF(it == mSessions.end(), "Bad state, unknown session id %s", + id->toString().c_str()); + LOG_ALWAYS_FATAL_IF(it->second != session, "Bad state, session has id mismatch %s", + id->toString().c_str()); (void)mSessions.erase(it); } diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index f953a05c08..931a876ac4 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -218,18 +218,17 @@ status_t RpcSession::readId() { LOG_ALWAYS_FATAL_IF(mForServer != nullptr, "Can only update ID for client."); } - int32_t id; - ExclusiveConnection connection; status_t status = ExclusiveConnection::find(sp::fromExisting(this), ConnectionUse::CLIENT, &connection); if (status != OK) return status; - status = state()->getSessionId(connection.get(), sp::fromExisting(this), &id); + mId = RpcAddress::zero(); + status = state()->getSessionId(connection.get(), sp::fromExisting(this), + &mId.value()); if (status != OK) return status; - LOG_RPC_DETAIL("RpcSession %p has id %d", this, id); - mId = id; + LOG_RPC_DETAIL("RpcSession %p has id %s", this, mId->toString().c_str()); return OK; } @@ -329,7 +328,7 @@ bool RpcSession::setupSocketClient(const RpcSocketAddress& addr) { mOutgoingConnections.size()); } - if (!setupOneSocketConnection(addr, RPC_SESSION_ID_NEW, false /*reverse*/)) return false; + if (!setupOneSocketConnection(addr, RpcAddress::zero(), false /*reverse*/)) return false; // TODO(b/189955605): we should add additional sessions dynamically // instead of all at once. @@ -366,7 +365,8 @@ bool RpcSession::setupSocketClient(const RpcSocketAddress& addr) { return true; } -bool RpcSession::setupOneSocketConnection(const RpcSocketAddress& addr, int32_t id, bool reverse) { +bool RpcSession::setupOneSocketConnection(const RpcSocketAddress& addr, const RpcAddress& id, + bool reverse) { for (size_t tries = 0; tries < 5; tries++) { if (tries > 0) usleep(10000); @@ -390,9 +390,9 @@ bool RpcSession::setupOneSocketConnection(const RpcSocketAddress& addr, int32_t return false; } - RpcConnectionHeader header{ - .sessionId = id, - }; + RpcConnectionHeader header{.options = 0}; + memcpy(&header.sessionId, &id.viewRawEmbedded(), sizeof(RpcWireAddress)); + if (reverse) header.options |= RPC_CONNECTION_OPTION_REVERSE; if (sizeof(header) != TEMP_FAILURE_RETRY(write(serverFd.get(), &header, sizeof(header)))) { @@ -469,7 +469,7 @@ bool RpcSession::addOutgoingConnection(unique_fd fd) { } bool RpcSession::setForServer(const wp& server, const wp& eventListener, - int32_t sessionId) { + const RpcAddress& sessionId) { LOG_ALWAYS_FATAL_IF(mForServer != nullptr); LOG_ALWAYS_FATAL_IF(server == nullptr); LOG_ALWAYS_FATAL_IF(mEventListener != nullptr); diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index 15eec20d1d..fd2eff6870 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -369,7 +369,7 @@ status_t RpcState::getMaxThreads(const sp& connection } status_t RpcState::getSessionId(const sp& connection, - const sp& session, int32_t* sessionIdOut) { + const sp& session, RpcAddress* sessionIdOut) { Parcel data; data.markForRpc(session); Parcel reply; @@ -382,12 +382,7 @@ status_t RpcState::getSessionId(const sp& connection, return status; } - int32_t sessionId; - status = reply.readInt32(&sessionId); - if (status != OK) return status; - - *sessionIdOut = sessionId; - return OK; + return sessionIdOut->readFromParcel(reply); } status_t RpcState::transact(const sp& connection, @@ -767,9 +762,9 @@ processTransactInternalTailCall: } case RPC_SPECIAL_TRANSACT_GET_SESSION_ID: { // for client connections, this should always report the value - // originally returned from the server - int32_t id = session->mId.value(); - replyStatus = reply.writeInt32(id); + // originally returned from the server, so this is asserting + // that it exists + replyStatus = session->mId.value().writeToParcel(&reply); break; } default: { diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h index d306595d83..529dee534c 100644 --- a/libs/binder/RpcState.h +++ b/libs/binder/RpcState.h @@ -62,7 +62,7 @@ public: status_t getMaxThreads(const sp& connection, const sp& session, size_t* maxThreadsOut); status_t getSessionId(const sp& connection, - const sp& session, int32_t* sessionIdOut); + const sp& session, RpcAddress* sessionIdOut); [[nodiscard]] status_t transact(const sp& connection, const sp& address, uint32_t code, const Parcel& data, diff --git a/libs/binder/RpcWireFormat.h b/libs/binder/RpcWireFormat.h index 4bd8e36aad..2016483138 100644 --- a/libs/binder/RpcWireFormat.h +++ b/libs/binder/RpcWireFormat.h @@ -20,20 +20,26 @@ namespace android { #pragma clang diagnostic push #pragma clang diagnostic error "-Wpadded" -constexpr int32_t RPC_SESSION_ID_NEW = -1; - enum : uint8_t { RPC_CONNECTION_OPTION_REVERSE = 0x1, }; +constexpr uint64_t RPC_WIRE_ADDRESS_OPTION_CREATED = 1 << 0; // distinguish from '0' address +constexpr uint64_t RPC_WIRE_ADDRESS_OPTION_FOR_SERVER = 1 << 1; + +struct RpcWireAddress { + uint64_t options; + uint8_t address[32]; +}; + /** * This is sent to an RpcServer in order to request a new connection is created, * either as part of a new session or an existing session */ struct RpcConnectionHeader { - int32_t sessionId; + RpcWireAddress sessionId; uint8_t options; - uint8_t reserved[3]; + uint8_t reserved[7]; }; #define RPC_CONNECTION_INIT_OKAY "cci" @@ -89,14 +95,6 @@ struct RpcWireHeader { uint32_t reserved[2]; }; -constexpr uint64_t RPC_WIRE_ADDRESS_OPTION_CREATED = 1 << 0; // distinguish from '0' address -constexpr uint64_t RPC_WIRE_ADDRESS_OPTION_FOR_SERVER = 1 << 1; - -struct RpcWireAddress { - uint64_t options; - uint8_t address[32]; -}; - struct RpcWireTransaction { RpcWireAddress address; uint32_t code; diff --git a/libs/binder/include/binder/RpcAddress.h b/libs/binder/include/binder/RpcAddress.h index e856fa94ba..e428908c88 100644 --- a/libs/binder/include/binder/RpcAddress.h +++ b/libs/binder/include/binder/RpcAddress.h @@ -29,11 +29,7 @@ class Parcel; struct RpcWireAddress; /** - * This class represents an identifier of a binder object. - * - * The purpose of this class it to hide the ABI of an RpcWireAddress, and - * potentially allow us to change the size of it in the future (RpcWireAddress - * is PIMPL, essentially - although the type that is used here is not exposed). + * This class represents an identifier across an RPC boundary. */ class RpcAddress { public: diff --git a/libs/binder/include/binder/RpcServer.h b/libs/binder/include/binder/RpcServer.h index fdcb3a88c6..c8d2857347 100644 --- a/libs/binder/include/binder/RpcServer.h +++ b/libs/binder/include/binder/RpcServer.h @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -171,8 +172,7 @@ private: std::map mConnectingThreads; sp mRootObject; wp mRootObjectWeak; - std::map> mSessions; - int32_t mSessionIdCounter = 0; + std::map> mSessions; std::unique_ptr mShutdownTrigger; std::condition_variable mShutdownCv; }; diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 218de205f5..1548e763b4 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -221,12 +221,12 @@ private: static void join(sp&& session, PreJoinSetupResult&& result); [[nodiscard]] bool setupSocketClient(const RpcSocketAddress& address); - [[nodiscard]] bool setupOneSocketConnection(const RpcSocketAddress& address, int32_t sessionId, - bool server); + [[nodiscard]] bool setupOneSocketConnection(const RpcSocketAddress& address, + const RpcAddress& sessionId, bool server); [[nodiscard]] bool addOutgoingConnection(base::unique_fd fd); [[nodiscard]] bool setForServer(const wp& server, const wp& eventListener, - int32_t sessionId); + const RpcAddress& sessionId); sp assignIncomingConnectionToThisThread(base::unique_fd fd); [[nodiscard]] bool removeIncomingConnection(const sp& connection); @@ -278,8 +278,7 @@ private: sp mShutdownListener; // used for client sessions wp mEventListener; // mForServer if server, mShutdownListener if client - // TODO(b/183988761): this shouldn't be guessable - std::optional mId; + std::optional mId; std::unique_ptr mShutdownTrigger; -- cgit v1.2.3-59-g8ed1b From b86e26b735ee912acf23c670398f2d88fdfe5994 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Sat, 12 Jun 2021 00:35:58 +0000 Subject: libbinder: RPC skip init on /dev/null This started breaking the fuzzer, since we can't do a socket operation on /dev/null. Bug: N/A # yet! Test: fuzzer no longer crashes Change-Id: I881f63b85108ff488cb5798b1f0b96629b592329 --- libs/binder/RpcServer.cpp | 2 +- libs/binder/RpcSession.cpp | 11 +++++++---- libs/binder/include/binder/RpcSession.h | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) (limited to 'libs/binder/RpcServer.cpp') diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp index 3599427f5a..a8f3fa8f6f 100644 --- a/libs/binder/RpcServer.cpp +++ b/libs/binder/RpcServer.cpp @@ -313,7 +313,7 @@ void RpcServer::establishConnection(sp&& server, base::unique_fd clie } if (reverse) { - LOG_ALWAYS_FATAL_IF(!session->addOutgoingConnection(std::move(clientFd)), + LOG_ALWAYS_FATAL_IF(!session->addOutgoingConnection(std::move(clientFd), true), "server state must already be initialized"); return; } diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index 931a876ac4..4f55eef2d1 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -100,7 +100,7 @@ bool RpcSession::addNullDebuggingClient() { return false; } - return addOutgoingConnection(std::move(serverFd)); + return addOutgoingConnection(std::move(serverFd), false); } sp RpcSession::getRootObject() { @@ -432,7 +432,7 @@ bool RpcSession::setupOneSocketConnection(const RpcSocketAddress& addr, const Rp LOG_ALWAYS_FATAL_IF(!ownershipTransferred); return true; } else { - return addOutgoingConnection(std::move(serverFd)); + return addOutgoingConnection(std::move(serverFd), true); } } @@ -440,7 +440,7 @@ bool RpcSession::setupOneSocketConnection(const RpcSocketAddress& addr, const Rp return false; } -bool RpcSession::addOutgoingConnection(unique_fd fd) { +bool RpcSession::addOutgoingConnection(unique_fd fd, bool init) { sp connection = sp::make(); { std::lock_guard _l(mMutex); @@ -458,7 +458,10 @@ bool RpcSession::addOutgoingConnection(unique_fd fd) { mOutgoingConnections.push_back(connection); } - status_t status = mState->sendConnectionInit(connection, sp::fromExisting(this)); + status_t status = OK; + if (init) { + mState->sendConnectionInit(connection, sp::fromExisting(this)); + } { std::lock_guard _l(mMutex); diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 1548e763b4..69c2a1a956 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -223,7 +223,7 @@ private: [[nodiscard]] bool setupSocketClient(const RpcSocketAddress& address); [[nodiscard]] bool setupOneSocketConnection(const RpcSocketAddress& address, const RpcAddress& sessionId, bool server); - [[nodiscard]] bool addOutgoingConnection(base::unique_fd fd); + [[nodiscard]] bool addOutgoingConnection(base::unique_fd fd, bool init); [[nodiscard]] bool setForServer(const wp& server, const wp& eventListener, const RpcAddress& sessionId); -- cgit v1.2.3-59-g8ed1b