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

[Alexander Potapenko]

On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook at chromium.org> wrote:
>
> On Mon, Feb 24, 2020 at 04:35:00PM +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:
> >
> >       struct binder_transaction_data tr;
> >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> >               return -EFAULT;
> >
> > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > redundant locals initialization that the compiler fails to remove.
> > To work around this problem till Clang can deal with it, we apply
> > __do_not_initialize to local Binder structures.
>
> It should be possible to write a Coccinelle script to identify all these
> cases. (i.e. a single path from struct declaration to copy_from_user())
> and apply the changes automatically. This script could be checked into
> scripts/coccinelle/ to help keep these markings in sync...

The following script:

=================================
@local_inited_by_cfu@
attribute name __no_initialize;
identifier var;
type T;
statement stmt;
@@
T var
+__no_initialize
;
if (copy_from_user(&var,..., sizeof(var))) stmt
=================================

seems to do the job, but this transformation isn't idempotent: it
inserts __no_initialize every time Coccinelle is invoked.
It's hard to work around this problem, as attributes may only be parts
of +-lines (i.e. I cannot write a rule that matches "T var
__no_initialize")
Linux kernel coccinelle scripts don't really contain good examples of
adding attributes, and probably the spatch version used by most
maintainers doesn't support them :(