Support deleting param for preconnected sessions
Although we can pass arbitrary pointers to setup preconnected clients,
the lifetime isn't clear and it's very easy to make UAF bugs.
To prevent UAF, an additional function can be passed to delete param
when param is no longer required, effectively transferring the ownership
of param to RPC session.
Bug: 398890208
Test: atest MicrodroidTests
Change-Id: I1a1c149a56876f56fba81b89cdc90ee372d2d7fe
diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp
index 16023ff..1f3a45a 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -188,7 +188,9 @@
}
status_t RpcSession::setupPreconnectedClient(unique_fd fd, std::function<unique_fd()>&& request) {
- return setupClient([&](const std::vector<uint8_t>& sessionId, bool incoming) -> status_t {
+ return setupClient([&, fd = std::move(fd),
+ request = std::move(request)](const std::vector<uint8_t>& sessionId,
+ bool incoming) mutable -> status_t {
if (!fd.ok()) {
fd = request();
if (!fd.ok()) return BAD_VALUE;
@@ -476,8 +478,10 @@
return server;
}
-status_t RpcSession::setupClient(const std::function<status_t(const std::vector<uint8_t>& sessionId,
- bool incoming)>& connectAndInit) {
+template <typename Fn,
+ typename /* = std::enable_if_t<std::is_invocable_r_v<
+ status_t, Fn, const std::vector<uint8_t>&, bool>> */>
+status_t RpcSession::setupClient(Fn&& connectAndInit) {
{
RpcMutexLockGuard _l(mMutex);
LOG_ALWAYS_FATAL_IF(mStartedSetup, "Must only setup session once");
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index af37bf2..c9f92da 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -25,6 +25,7 @@
#include <map>
#include <optional>
+#include <type_traits>
#include <vector>
namespace android {
@@ -291,9 +292,14 @@
// join on thread passed to preJoinThreadOwnership
static void join(sp<RpcSession>&& session, PreJoinSetupResult&& result);
- [[nodiscard]] status_t setupClient(
- const std::function<status_t(const std::vector<uint8_t>& sessionId, bool incoming)>&
- connectAndInit);
+ // This is a workaround to support move-only functors.
+ // TODO: use std::move_only_function when it becomes available.
+ template <typename Fn,
+ // Fn must be a callable type taking (const std::vector<uint8_t>&, bool) and returning
+ // status_t
+ typename = std::enable_if_t<
+ std::is_invocable_r_v<status_t, Fn, const std::vector<uint8_t>&, bool>>>
+ [[nodiscard]] status_t setupClient(Fn&& connectAndInit);
[[nodiscard]] status_t setupSocketClient(const RpcSocketAddress& address);
[[nodiscard]] status_t setupOneSocketConnection(const RpcSocketAddress& address,
const std::vector<uint8_t>& sessionId,
diff --git a/libs/binder/include_rpc_unstable/binder_rpc_unstable.hpp b/libs/binder/include_rpc_unstable/binder_rpc_unstable.hpp
index 48c0ea6..1949cbb 100644
--- a/libs/binder/include_rpc_unstable/binder_rpc_unstable.hpp
+++ b/libs/binder/include_rpc_unstable/binder_rpc_unstable.hpp
@@ -134,9 +134,14 @@
//
// param will be passed to requestFd. Callers can use param to pass contexts to
// the requestFd function.
+//
+// paramDeleteFd will be called when requestFd and param are no longer in use.
+// Callers can pass a function deleting param if necessary. Callers can set
+// paramDeleteFd to null if callers doesn't need to clean up param.
AIBinder* ARpcSession_setupPreconnectedClient(ARpcSession* session,
int (*requestFd)(void* param),
- void* param);
+ void* param,
+ void (*paramDeleteFd)(void* param));
// Sets the file descriptor transport mode for this session.
void ARpcSession_setFileDescriptorTransportMode(ARpcSession* session,
diff --git a/libs/binder/libbinder_rpc_unstable.cpp b/libs/binder/libbinder_rpc_unstable.cpp
index 64b1be2..8177fb0 100644
--- a/libs/binder/libbinder_rpc_unstable.cpp
+++ b/libs/binder/libbinder_rpc_unstable.cpp
@@ -248,9 +248,18 @@
#endif // __TRUSTY__
AIBinder* ARpcSession_setupPreconnectedClient(ARpcSession* handle, int (*requestFd)(void* param),
- void* param) {
+ void* param, void (*paramDeleteFd)(void* param)) {
auto session = handleToStrongPointer<RpcSession>(handle);
- auto request = [=] { return unique_fd{requestFd(param)}; };
+ auto deleter = [=](void* param) {
+ if (paramDeleteFd) {
+ paramDeleteFd(param);
+ }
+ };
+ // TODO: use unique_ptr once setupPreconnectedClient uses std::move_only_function.
+ std::shared_ptr<void> sharedParam(param, deleter);
+ auto request = [=, sharedParam = std::move(sharedParam)] {
+ return unique_fd{requestFd(sharedParam.get())};
+ };
if (status_t status = session->setupPreconnectedClient(unique_fd{}, request); status != OK) {
ALOGE("Failed to set up preconnected client. error: %s", statusToString(status).c_str());
return nullptr;
diff --git a/libs/binder/rust/rpcbinder/src/session.rs b/libs/binder/rust/rpcbinder/src/session.rs
index 09688a2..411b9de 100644
--- a/libs/binder/rust/rpcbinder/src/session.rs
+++ b/libs/binder/rust/rpcbinder/src/session.rs
@@ -195,11 +195,13 @@
/// take ownership of) file descriptors already connected to it.
pub fn setup_preconnected_client<T: FromIBinder + ?Sized>(
&self,
- mut request_fd: impl FnMut() -> Option<RawFd>,
+ request_fd: impl FnMut() -> Option<RawFd>,
) -> Result<Strong<T>, StatusCode> {
- // Double reference the factory because trait objects aren't FFI safe.
- let mut request_fd_ref: RequestFd = &mut request_fd;
- let param = &mut request_fd_ref as *mut RequestFd as *mut c_void;
+ // Trait objects aren't FFI safe, so *mut c_void can't be converted back to
+ // *mut dyn FnMut() -> Option<RawFd>>. Double box the factory to make it possible to get
+ // the factory from *mut c_void (to *mut Box<dyn<...>>) in the callbacks.
+ let request_fd_box: Box<dyn FnMut() -> Option<RawFd>> = Box::new(request_fd);
+ let param = Box::into_raw(Box::new(request_fd_box)) as *mut c_void;
// SAFETY: AIBinder returned by RpcPreconnectedClient has correct reference count, and the
// ownership can be safely taken by new_spibinder. RpcPreconnectedClient does not take ownership
@@ -209,6 +211,7 @@
self.as_ptr(),
Some(request_fd_wrapper),
param,
+ Some(param_delete_fd_wrapper),
))
};
Self::get_interface(service)
@@ -225,13 +228,18 @@
}
}
-type RequestFd<'a> = &'a mut dyn FnMut() -> Option<RawFd>;
-
unsafe extern "C" fn request_fd_wrapper(param: *mut c_void) -> c_int {
- let request_fd_ptr = param as *mut RequestFd;
+ let request_fd_ptr = param as *mut Box<dyn FnMut() -> Option<RawFd>>;
// SAFETY: This is only ever called by RpcPreconnectedClient, within the lifetime of the
// BinderFdFactory reference, with param being a properly aligned non-null pointer to an
// initialized instance.
let request_fd = unsafe { request_fd_ptr.as_mut().unwrap() };
request_fd().unwrap_or(-1)
}
+
+unsafe extern "C" fn param_delete_fd_wrapper(param: *mut c_void) {
+ // SAFETY: This is only ever called by RpcPreconnectedClient, with param being the
+ // pointer returned from Box::into_raw.
+ let request_fd_box = unsafe { Box::from_raw(param as *mut Box<dyn FnMut() -> Option<RawFd>>) };
+ drop(request_fd_box);
+}
diff --git a/libs/binder/trusty/rust/binder_rpc_unstable_bindgen/rules.mk b/libs/binder/trusty/rust/binder_rpc_unstable_bindgen/rules.mk
index ef1b7c3..40fc218 100644
--- a/libs/binder/trusty/rust/binder_rpc_unstable_bindgen/rules.mk
+++ b/libs/binder/trusty/rust/binder_rpc_unstable_bindgen/rules.mk
@@ -30,6 +30,8 @@
trusty/user/base/lib/libstdc++-trusty \
trusty/user/base/lib/trusty-sys \
+MODULE_SRCDEPS := $(LIBBINDER_DIR)/include_rpc_unstable/binder_rpc_unstable.hpp
+
MODULE_BINDGEN_SRC_HEADER := $(LOCAL_DIR)/BinderBindings.hpp
MODULE_BINDGEN_FLAGS += \