[PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()

Alexander Potapenko glider at google.com
Tue Mar 3 09:14:18 UTC 2020


On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe at perches.com> wrote:
>
> On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe at perches.com> wrote:
> > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe at perches.com> wrote:
> > > > > On Mon, 2020-03-02 at 14:04 +0100, glider at google.com wrote:
> > > > > > Certain copy_from_user() invocations in binder.c are known to
> > > > > > unconditionally initialize locals before their first use, like e.g. in
> > > > > > the following case:
> > > > > []
> > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > []
> > > > > > @@ -3788,7 +3788,7 @@ static int binder_thread_write(struct binder_proc *proc,
> > > > > >
> > > > > >               case BC_TRANSACTION_SG:
> > > > > >               case BC_REPLY_SG: {
> > > > > > -                     struct binder_transaction_data_sg tr;
> > > > > > +                     struct binder_transaction_data_sg tr __no_initialize;
> > > > > >
> > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > >
> > > > > I fail to see any value in marking tr with __no_initialize
> > > > > when it's immediately written to by copy_from_user.
> > > >
> > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > the following code:
> > > >
> > > >   struct binder_transaction_data_sg tr;
> > > >   memset(&tr, 0xAA, sizeof(tr));
> > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > >
> > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > prevent the compiler from emitting the redundant initialization.
> > >
> > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > Correct.
> >
> > > This marking would likely need to be done for nearly all
> > > 3000+ copy_from_user entries.
> > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > cases that we encounter, but in the long-term a compiler solution must
> > supersede them.
> >
> > > Why not try to get something done on the compiler side
> > > to mark the function itself rather than the uses?
> > This is being worked on in the meantime as well (see
> > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > Do you have any particular requisitions about how this should look on
> > the source level?
>
> I presume something like the below when appropriate for
> automatic variables when not already initialized or modified.
> ---
>  include/linux/uaccess.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 8a215c..3e034b5 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
>  #endif
>
>  static __always_inline unsigned long __must_check
> -copy_from_user(void *to, const void __user *from, unsigned long n)
> +copy_from_user(void __no_initialize *to, const void __user *from,
> +              unsigned long n)

Shall this __no_initialize attribute denote that the whole object
passed to it is initialized?
Or do we need to encode the length as well, as Jann suggests?
It's also interesting what should happen if *to is pointing _inside_ a
local object - presumably it's unsafe to disable initialization for
the whole object.


More information about the devel mailing list