ANDROID: rust_binder: split out logic for zero-to-one refcount increment

When userspace tells the kernel to perform a zero-to-one refcount,
userspace promises that such increments are okay, but this promise
requires that the kernel delivers them to the current thread before the
kernel delivers BR_TRANSACTION_COMPLETE. Otherwise, if the increment is
delivered to another thread, then it is possible that userspace could
destroy the object before it sees the increment, which would be a
use-after-free in userspace.

The current implementation may deliver refcount increments to the wrong
thread in cases like the above. This patch changes introduces enough
tracking to detect such cases, and will trigger a kernel panic if we are
about to trigger the bug.

In the next patch, we will replace the kernel panic with a correct
implementation.

Bug: 333535706
Change-Id: I51d9b87334498d1a166f151acf3dbddb8b3333a0
Signed-off-by: Alice Ryhl <aliceryhl@google.com>

drivers/android/binder/node.rs

@@ -25,6 +25,88 @@ use crate::{
     DArc, DLArc, DTRWrap, DeliverToRead,
 };
 
+#[derive(Debug)]
+pub(crate) struct CouldNotDeliverCriticalIncrement;
+
+/// Keeps track of how this node is scheduled.
+///
+/// There are two ways to schedule a node to a work list. Just schedule the node itself, or
+/// allocate a wrapper that references the node and schedule the wrapper. These wrappers exists to
+/// make it possible to "move" a node from one list to another - when `do_work` is called directly
+/// on the `Node`, then it's a no-op if there's also a pending wrapper.
+///
+/// Wrappers are generally only needed for zero-to-one refcount increments, and there are two cases
+/// of this: weak increments and strong increments. We call such increments "critical" because it
+/// is critical that they are delivered to the thread doing the increment. Some examples:
+///
+/// * One thread makes a zero-to-one strong increment, and another thread makes a zero-to-one weak
+///   increment. Delivering the node to the thread doing the weak increment is wrong, since the
+///   thread doing the strong increment may have ended a long time ago when the command is actually
+///   processed by userspace.
+///
+/// * We have a weak reference and are about to drop it on one thread. But then another thread does
+///   a zero-to-one strong increment. If the strong increment gets sent to the thread that was
+///   about to drop the weak reference, then the strong increment could be processed after the
+///   other thread has already exited, which would be too late.
+///
+/// Note that trying to create a `ListArc` to the node can succeed even if `has_normal_push` is
+/// set. This is because another thread might just have popped the node from a todo list, but not
+/// yet called `do_work`. However, if `has_normal_push` is false, then creating a `ListArc` should
+/// always succeed.
+///
+/// Like the other fields in `NodeInner`, the delivery state is protected by the process lock.
+struct DeliveryState {
+    /// Is the `Node` currently scheduled?
+    has_pushed_node: bool,
+
+    /// Is the currently scheduled `Node` scheduled due to a weak zero2one increment?
+    has_weak_zero2one: bool,
+
+    /// Is the currently scheduled `Node` scheduled due to a strong zero2one increment?
+    has_strong_zero2one: bool,
+}
+
+impl DeliveryState {
+    fn should_normal_push(&self) -> bool {
+        !self.has_pushed_node
+    }
+
+    fn did_normal_push(&mut self) {
+        assert!(self.should_normal_push());
+        self.has_pushed_node = true;
+    }
+
+    fn should_push_weak_zero2one(&self) -> bool {
+        !self.has_weak_zero2one && !self.has_strong_zero2one
+    }
+
+    fn can_push_weak_zero2one_normally(&self) -> bool {
+        !self.has_pushed_node
+    }
+
+    fn did_push_weak_zero2one(&mut self) {
+        assert!(self.should_push_weak_zero2one());
+        assert!(self.can_push_weak_zero2one_normally());
+        self.has_pushed_node = true;
+        self.has_weak_zero2one = true;
+    }
+
+    fn should_push_strong_zero2one(&self) -> bool {
+        !self.has_strong_zero2one
+    }
+
+    fn can_push_strong_zero2one_normally(&self) -> bool {
+        !self.has_pushed_node
+    }
+
+    fn did_push_strong_zero2one(&mut self) {
+        assert!(self.should_push_strong_zero2one());
+        assert!(self.can_push_strong_zero2one_normally());
+        self.has_pushed_node = true;
+        self.has_strong_zero2one = true;
+    }
+}
+
 struct CountState {
     /// The reference count.
     count: usize,
@@ -47,6 +129,7 @@ struct NodeInner {
     strong: CountState,
     /// Weak refcounts held on this node by `NodeRef` objects.
     weak: CountState,
+    delivery_state: DeliveryState,
     /// The binder driver guarantees that oneway transactions sent to the same node are serialized,
     /// that is, userspace will not be given the next one until it has finished processing the
     /// previous oneway transaction. This is done to avoid the case where two oneway transactions
@@ -117,6 +200,11 @@ impl Node {
                 NodeInner {
                     strong: CountState::new(),
                     weak: CountState::new(),
+                    delivery_state: DeliveryState {
+                        has_pushed_node: false,
+                        has_weak_zero2one: false,
+                        has_strong_zero2one: false,
+                    },
                     death_list: List::new(),
                     oneway_todo: List::new(),
                     has_oneway_transaction: false,
@@ -274,8 +362,9 @@ impl Node {
             // that.
             let need_push = should_drop_weak || should_drop_strong;
 
-            if need_push {
+            if need_push && inner.delivery_state.should_normal_push() {
                 let list_arc = ListArc::try_from_arc(self.clone()).ok().unwrap();
+                inner.delivery_state.did_normal_push();
                 Some(list_arc)
             } else {
                 None
@@ -316,14 +405,57 @@ impl Node {
             !is_dead && state.count == 0 && state.has_count
         };
 
-        if need_push {
+        if need_push && inner.delivery_state.should_normal_push() {
             let list_arc = ListArc::try_from_arc(self.clone()).ok().unwrap();
+            inner.delivery_state.did_normal_push();
             Some(list_arc)
         } else {
             None
         }
     }
 
+    pub(crate) fn incr_refcount_allow_zero2one(
+        self: &DArc<Self>,
+        strong: bool,
+        owner_inner: &mut ProcessInner,
+    ) -> Result<Option<DLArc<Node>>, CouldNotDeliverCriticalIncrement> {
+        let is_dead = owner_inner.is_dead;
+        let inner = self.inner.access_mut(owner_inner);
+
+        // Get a reference to the state we'll update.
+        let state = if strong {
+            &mut inner.strong
+        } else {
+            &mut inner.weak
+        };
+
+        // Update the count and determine whether we need to push work.
+        state.count += 1;
+        if is_dead || state.has_count {
+            return Ok(None);
+        }
+
+        // Userspace needs to be notified of this.
+        if !strong && inner.delivery_state.should_push_weak_zero2one() {
+            assert!(inner.delivery_state.can_push_weak_zero2one_normally());
+            let list_arc = ListArc::try_from_arc(self.clone()).ok().unwrap();
+            inner.delivery_state.did_push_weak_zero2one();
+            Ok(Some(list_arc))
+        } else if strong && inner.delivery_state.should_push_strong_zero2one() {
+            if inner.delivery_state.can_push_strong_zero2one_normally() {
+                let list_arc = ListArc::try_from_arc(self.clone()).ok().unwrap();
+                inner.delivery_state.did_push_strong_zero2one();
+                Ok(Some(list_arc))
+            } else {
+                state.count -= 1;
+                Err(CouldNotDeliverCriticalIncrement)
+            }
+        } else {
+            // Work is already pushed, and we don't need to push again.
+            Ok(None)
+        }
+    }
+
     pub(crate) fn update_refcount(self: &DArc<Self>, inc: bool, count: usize, strong: bool) {
         self.owner
             .inner
@@ -451,6 +583,11 @@ impl DeliverToRead for Node {
     fn do_work(self: DArc<Self>, _thread: &Thread, writer: &mut UserSliceWriter) -> Result<bool> {
         let mut owner_inner = self.owner.inner.lock();
         let inner = self.inner.access_mut(&mut owner_inner);
+
+        inner.delivery_state.has_pushed_node = false;
+        inner.delivery_state.has_weak_zero2one = false;
+        inner.delivery_state.has_strong_zero2one = false;
+
         let strong = inner.strong.count > 0;
         let has_strong = inner.strong.has_count;
         let weak = strong || inner.weak.count > 0;

drivers/android/binder/process.rs

@@ -38,7 +38,7 @@ use crate::{
     context::Context,
     defs::*,
     error::{BinderError, BinderResult},
-    node::{Node, NodeDeath, NodeRef},
+    node::{CouldNotDeliverCriticalIncrement, Node, NodeDeath, NodeRef},
     prio::{self, BinderPriority},
     range_alloc::{self, RangeAllocator},
     thread::{PushWorkRes, Thread},
@@ -216,6 +216,20 @@ impl ProcessInner {
         NodeRef::new(node, strong_count, 1 - strong_count)
     }
 
+    pub(crate) fn new_node_ref_with_thread(
+        &mut self,
+        node: DArc<Node>,
+        strong: bool,
+        thread: &Thread,
+    ) -> Result<NodeRef, CouldNotDeliverCriticalIncrement> {
+        let push = node.incr_refcount_allow_zero2one(strong, self)?;
+        if let Some(node) = push {
+            thread.push_work_deferred(node);
+        }
+        let strong_count = if strong { 1 } else { 0 };
+        Ok(NodeRef::new(node, strong_count, 1 - strong_count))
+    }
+
     /// Returns an existing node with the given pointer and cookie, if one exists.
     ///
     /// Returns an error if a node with the given pointer but a different cookie exists.
@@ -233,21 +247,6 @@ impl ProcessInner {
         }
     }
 
-    /// Returns a reference to an existing node with the given pointer and cookie. It requires a
-    /// mutable reference because it needs to increment the ref count on the node, which may
-    /// require pushing work to the work queue (to notify userspace of 0 to 1 transitions).
-    fn get_existing_node_ref(
-        &mut self,
-        ptr: u64,
-        cookie: u64,
-        strong: bool,
-        thread: Option<&Thread>,
-    ) -> Result<Option<NodeRef>> {
-        Ok(self
-            .get_existing_node(ptr, cookie)?
-            .map(|node| self.new_node_ref(node, strong, thread)))
-    }
-
     fn register_thread(&mut self) -> bool {
         if self.requested_thread_count == 0 {
             return false;
@@ -632,7 +631,7 @@ impl Process {
         } else {
             (0, 0, 0)
         };
-        let node_ref = self.get_node(ptr, cookie, flags as _, true, Some(thread))?;
+        let node_ref = self.get_node(ptr, cookie, flags as _, true, thread)?;
         let node = node_ref.node.clone();
         self.ctx.set_manager_node(node_ref)?;
         self.inner.lock().is_manager = true;
@@ -643,19 +642,19 @@ impl Process {
         Ok(())
     }
 
-    pub(crate) fn get_node(
+    fn get_node_inner(
         self: ArcBorrow<'_, Self>,
         ptr: u64,
         cookie: u64,
         flags: u32,
         strong: bool,
-        thread: Option<&Thread>,
+        thread: &Thread,
-    ) -> Result<NodeRef> {
+    ) -> Result<Result<NodeRef, CouldNotDeliverCriticalIncrement>> {
         // Try to find an existing node.
         {
             let mut inner = self.inner.lock();
-            if let Some(node) = inner.get_existing_node_ref(ptr, cookie, strong, thread)? {
+            if let Some(node) = inner.get_existing_node(ptr, cookie)? {
-                return Ok(node);
+                return Ok(inner.new_node_ref_with_thread(node, strong, thread));
             }
         }
 
@@ -663,12 +662,33 @@ impl Process {
         let node = DTRWrap::arc_pin_init(Node::new(ptr, cookie, flags, self.into()))?.into_arc();
         let rbnode = RBTree::try_allocate_node(ptr, node.clone())?;
         let mut inner = self.inner.lock();
-        if let Some(node) = inner.get_existing_node_ref(ptr, cookie, strong, thread)? {
+        if let Some(node) = inner.get_existing_node(ptr, cookie)? {
-            return Ok(node);
+            return Ok(inner.new_node_ref_with_thread(node, strong, thread));
         }
 
         inner.nodes.insert(rbnode);
-        Ok(inner.new_node_ref(node, strong, thread))
+        // This can only fail if someone has already pushed the node to a list, but we just created
+        // it and still hold the lock, so it can't fail right now.
+        let node_ref = inner
+            .new_node_ref_with_thread(node, strong, thread)
+            .unwrap();
+
+        Ok(Ok(node_ref))
+    }
+
+    pub(crate) fn get_node(
+        self: ArcBorrow<'_, Self>,
+        ptr: u64,
+        cookie: u64,
+        flags: u32,
+        strong: bool,
+        thread: &Thread,
+    ) -> Result<NodeRef> {
+        match self.get_node_inner(ptr, cookie, flags, strong, thread) {
+            Err(err) => Err(err),
+            Ok(Ok(node_ref)) => Ok(node_ref),
+            Ok(Err(CouldNotDeliverCriticalIncrement)) => todo!(),
+        }
     }
 
     pub(crate) fn insert_or_update_handle(

drivers/android/binder/thread.rs

@@ -749,13 +749,10 @@ impl Thread {
                 let ptr = unsafe { obj.__bindgen_anon_1.binder } as _;
                 let cookie = obj.cookie as _;
                 let flags = obj.flags as _;
-                let node = self.process.as_arc_borrow().get_node(
+                let node = self
-                    ptr,
+                    .process
-                    cookie,
+                    .as_arc_borrow()
-                    flags,
+                    .get_node(ptr, cookie, flags, strong, self)?;
-                    strong,
-                    Some(self),
-                )?;
                 security::binder_transfer_binder(&self.process.cred, &view.alloc.process.cred)?;
                 view.transfer_binder_object(offset, obj, strong, node)?;
             }