Re: [PATCH 1/2] rust_binder: reject sync transaction with pending sync work

[Carlos Llamas]

On Sun, Nov 08, 2026 at 03:11:01PM +0000, Chris Wiseman wrote:
> When a thread replies to a transaction with TF_DEFER_COMPLETE, its
> BR_TRANSACTION_COMPLETE work item stays in inner.work_list while the
> next blocking read waits in Process.ready_threads. A new incoming
> synchronous transaction can then be queued behind that deferred
> completion in inner.work_list. If the thread's read buffer only has
> room for the deferred completion, the ioctl returns to userspace with
> inner.current_transaction cleared while the incoming synchronous
> transaction remains queued in inner.work_list.
> 
> If that thread sends an outgoing synchronous BC_TRANSACTION before
> draining its work queue, Thread::transaction_inner sets
> inner.current_transaction to Some(t_out), and the read phase of the
> same BINDER_WRITE_READ ioctl pops the pending incoming transaction and
> overwrites inner.current_transaction with Some(t_in). This orphans
> t_out from the sender's transaction stack so
> Thread::unwind_transaction_stack aborts on exit, leaving a permanent
> Arc<Thread> <-> DArc<Transaction> reference cycle that pins both
> Arc<Process> instances and their mapped ShrinkablePageRange pages
> after the processes exit.
> 
> Resolve the TODO in Thread::transaction_inner by rejecting outgoing
> synchronous transactions with EINVAL when inner.work_list contains any
> item where DeliverToRead::should_sync_wakeup() is true. Checking
> should_sync_wakeup() matches C Binder's BINDER_WORK_TRANSACTION check
> and ignores Node or NodeWrapper BR_INCREFS/BR_ACQUIRE items pushed
> onto inner.work_list when Transaction::new translates flat binder
> objects.
> 
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Fixes: 8eb05e8e47f1 ("rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chris Wiseman <chriswiseman@xxxxxxxxxx>
> ---
>  drivers/android/binder/thread.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
> index f5965e838a96..3ca5c95695e4 100644
> --- a/drivers/android/binder/thread.rs
> +++ b/drivers/android/binder/thread.rs
> @@ -1393,8 +1393,6 @@ fn transaction_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> BinderResu
>          info.to_pid = node_ref.node.owner.task.pid();
>          info.to_node_debug_id = node_ref.node.debug_id;
>          security::binder_transaction(&self.process.cred, &node_ref.node.owner.cred)?;
> -        // TODO: We need to ensure that there isn't a pending transaction in the work queue. How
> -        // could this happen?
>          let top = self.top_of_transaction_stack()?;
>          let list_completion = DTRWrap::arc_try_new(DeliverCode::new(
>              BR_TRANSACTION_COMPLETE,
> @@ -1407,7 +1405,9 @@ fn transaction_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> BinderResu
>          // it with the new transaction.
>          {
>              let mut inner = self.inner.lock();
> -            if !transaction.is_stacked_on(&inner.current_transaction) {
> +            if inner.work_list.iter().any(|w| w.should_sync_wakeup())
> +                || !transaction.is_stacked_on(&inner.current_transaction)
> +            {
>                  binder_debug!(UserError, "got new transaction with bad transaction stack");
>                  return Err(EINVAL.into());
>              }
> -- 
> 2.56.0.317.g454f8302e1-goog
> 

If the issue happens when there is already a pending transaction, would
it be better to relocate the new check to top_of_transaction_stack(),
before we execute Transaction::new()? If possible, we can skip all the
unwinding associated with this. If not....

Reviewed-by: Carlos Llamas <cmllamas@xxxxxxxxxx>

Thanks
--
Carlos Llamas