Message-ID: <20191009151044.t2jo3mo4acjtyhez@wittgenstein>
Date:   Wed, 9 Oct 2019 17:10:45 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Todd Kjos <tkjos@...roid.com>, jannh@...gle.com, arve@...roid.com,
        christian@...uner.io, devel@...verdev.osuosl.org,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        maco@...roid.com, tkjos@...gle.com,
        Hridya Valsaraju <hridya@...gle.com>
Subject: Re: [PATCH] binder: prevent UAF read in
 print_binder_transaction_log_entry()

On Wed, Oct 09, 2019 at 10:55:58AM -0400, Joel Fernandes wrote:
> On Wed, Oct 09, 2019 at 04:29:11PM +0200, Christian Brauner wrote:
> > On Wed, Oct 09, 2019 at 10:21:29AM -0400, Joel Fernandes wrote:
> > > On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> > > > On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > > > > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > > > > When a binder transaction is initiated on a binder device coming from a
> > > > > > binderfs instance, a pointer to the name of the binder device is stashed
> > > > > > in the binder_transaction_log_entry's context_name member. Later on it
> > > > > > is used to print the name in print_binder_transaction_log_entry(). By
> > > > > > the time print_binder_transaction_log_entry() accesses context_name
> > > > > > binderfs_evict_inode() might have already freed the associated memory
> > > > > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > > > > the name of the binder device instead of stashing a pointer to it.
> > > > > > 
> > > > > > Reported-by: Jann Horn <jannh@...gle.com>
> > > > > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > > > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > > > > Cc: Joel Fernandes <joel@...lfernandes.org>
> > > > > > Cc: Todd Kjos <tkjos@...roid.com>
> > > > > > Cc: Hridya Valsaraju <hridya@...gle.com>
> > > > > > Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> > > > > > ---
> > > > > >  drivers/android/binder.c          | 4 +++-
> > > > > >  drivers/android/binder_internal.h | 2 +-
> > > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > > index c0a491277aca..5b9ac2122e89 100644
> > > > > > --- a/drivers/android/binder.c
> > > > > > +++ b/drivers/android/binder.c
> > > > > > @@ -57,6 +57,7 @@
> > > > > >  #include <linux/sched/signal.h>
> > > > > >  #include <linux/sched/mm.h>
> > > > > >  #include <linux/seq_file.h>
> > > > > > +#include <linux/string.h>
> > > > > >  #include <linux/uaccess.h>
> > > > > >  #include <linux/pid_namespace.h>
> > > > > >  #include <linux/security.h>
> > > > > > @@ -66,6 +67,7 @@
> > > > > >  #include <linux/task_work.h>
> > > > > >  
> > > > > >  #include <uapi/linux/android/binder.h>
> > > > > > +#include <uapi/linux/android/binderfs.h>
> > > > > >  
> > > > > >  #include <asm/cacheflush.h>
> > > > > >  
> > > > > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > > > > >  	e->target_handle = tr->target.handle;
> > > > > >  	e->data_size = tr->data_size;
> > > > > >  	e->offsets_size = tr->offsets_size;
> > > > > > -	e->context_name = proc->context->name;
> > > > > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > > > > 
> > > > > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > fit within the MAX.
> > > > 
> > > > I know but I don't think it's worth special-casing non-binderfs devices.
> > > > First, non-binderfs devices can only be created through a KCONFIG option
> > > > determined at compile time. For stock Android the names are the same for
> > > > all vendors afaik.
> > > 
> > > I am just talking about the name of weirdly named macro here.
> > 
> > You might miss context here: It's named that way because currently only
> > binderfs binder devices are bound to that limit. That's a point I made
> > further below in my previous mail. Non-binderfs devices are not subject
> > to that restriction and when we tried to make them subject to the same
> > it as rejected.
> 
> I know that. I am saying the memcpy is happening for regular binder devices
> as well but the macro has BINDERFS in the name. That's all. It is not a
> significant eye sore. But is a bit odd.

Right, and I told you that we _can't_ rename it to BINDER_MAX because
that check only happens for binderfs devices since you were suggesting
this. If you want to rename to get rid of the this being somehow
apparently odd then you need to introduce that check for non-binderfs
devices too. Or just rename the macro in a follow-up patch. I don't care.

> 
> > <snip>
> > 
> > > 
> > > > Fifth, I already tried to push for validation of non-binderfs binder
> > > > devices a while back when I wrote binderfs and was told that it's not
> > > > needed. Hrydia tried the same and we decided the same thing. So you get
> > > > to be the next person to send a patch. :)
> > > 
> > > I don't follow why we are talking about non-binderfs validation. I am just
> > 
> > Because above you said
> > 
> > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > fit within the MAX.
> > 
> > which to me reads like you want generic checks for _all_ binder devices
> > not just for the ones from binderfs.
> 
> No I am not talking about the checks at all, I am talking about the unwanted
> mem copy you are doing for regular binder devices now. Before binderfs this
> would not have happened, but now for regular binder devices we have to do the
> extra mem copies which were avoided before. That was what I was talking about.

I'm sorry but I did not get this at all from:
"So probably there should be a BINDER_MAX_NAME (and associated checks
for whether non BINDERFS names fit within the MAX." 

> 
> But this discussing is getting to bike shedding at this point, and since the
> overhead is likely small, I am Ok with the change (though I don't like very
> much the additional memcpy and the associated space wastage in the
> transaction buffer for regular binder devices).

Feel free to send a follow-up patch handling both separately.

Thanks!
Christian

Powered by blists - more mailing lists