Re: [PATCH v2 2/2] binder: fix UAF in binder_free_transaction()

[Carlos Llamas <cmllamas@google.com>]

On Mon, Jun 22, 2026 at 09:55:29PM +0200, Alice Ryhl wrote:
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -1658,10 +1658,19 @@ static void binder_txn_latency_free(struct binder_transaction *t)
> >
> >  static void binder_free_transaction(struct binder_transaction *t)
> >  {
> > +       struct binder_thread *target_thread;
> >         struct binder_proc *target_proc;
> >
> >         spin_lock(&t->lock);
> >         target_proc = t->to_proc;
> > +       target_thread = t->to_thread;
> > +       /*
> > +        * Pin target_thread to keep target_proc alive. Undelivered
> > +        * transactions with !target_thread are safe, as target_proc
> > +        * can only be the current context there.
> > +        */
> > +       if (target_thread)
> > +               atomic_inc(&target_thread->tmp_ref);
> 
> This is more complicated than the comment suggests, but I think it's
> correct. As far as I can tell, scenarios where to_thread is NULL but
> to_proc is not are also scenarios where the caller ensures that
> to_proc stays alive during this function call.

Right, transactions with !to_thread and a valid to_proc are undelivered,
and as such they can only reach this point from the to_proc's context.

> 
> It's unfortunate that there's no obvious better way of doing this. I'd
> like to just take a refcount on the process, but it's not atomic, and
> you can't take the proc lock protecting it because of lock inversion.

Bingo! I'm using the thread as a proxy because refactoring proc->tmp_ref
to be atomic would be a bigger and thus riskier change.

> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Thanks!