Re: [PATCH] binder: fix descriptor lookup for context manager

[Carlos Llamas <cmllamas@google.com>]

On Mon, Jul 22, 2024 at 12:57:31PM +0200, Alice Ryhl wrote:
> On Tue, Jul 16, 2024 at 7:40 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Mon, Jul 15, 2024 at 9:29 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >         /* 0 is reserved for the context manager */
> > > -       if (node == proc->context->binder_context_mgr_node) {
> > > -               *desc = 0;
> > > -               return 0;
> > > -       }
> > > +       offset = (node == proc->context->binder_context_mgr_node) ? 0 : 1;
> >
> > If context manager doesn't need to be bit 0 anymore, then why do we
> > bother to prefer bit 0? Does it matter?
> >
> > It would simplify the code below if the offset is always 0 since you
> > wouldn't need an offset at all.
> 
> Userspace assumes that sending a message to handle 0 means that the
> current context manager receives it. If we assign anything that is not
> the context manager to bit 0, then libbinder will send ctxmgr messages
> to random other processes. I don't think libbinder handles the case
> where context manager is restarted well at all. Most likely, if we hit
> this condition in real life, processes that had a non-zero refcount to
> the context manager will lose the ability to interact with ctxmgr
> until they are restarted.

Using handle 0 for transaction will always reach the current context
manager. This is hardcoded so it works regardless of the descriptor
assigned to any references.

Things get complicated when doing refcount operations though. It seems
that some commands will reach the new context manager and others will
reach the old dead node. Odd.

This needs to the fixed. I'll look into it.

> 
> I think this patch just needs to make sure that this scenario doesn't
> lead to a UAF in the kernel. Ensuring that userspace handles it
> gracefully is another matter.
> 

Right, the main concern in this patch is handling the BUG() assert.

Thanks,
--
Carlos Llamas