Message-ID: <20191009145558.GA96813@google.com>
Date:   Wed, 9 Oct 2019 10:55:58 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
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 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.

> <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.

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).

thanks!

 - Joel

> 
> (Btw, I didn't read your comments as pointing it out the patch is buggy.
> I mostly wanted to provide context why we ended up with the
> binderfs-specific restriction. Maybe the list sounded like a complaint
> but it wasn't meant to. :))
> 
> Thanks!
> Christian

Powered by blists - more mailing lists