On Wed, Feb 26, 2020 at 03:58:41PM +0100, Alexander Potapenko wrote: > On Tue, Feb 25, 2020 at 4:24 PM Alexander Potapenko <glider@xxxxxxxxxx> wrote: > > > > On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > > > > > On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@xxxxxxxxxx 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") > > This one: > > ============================ > @match@ > type T; > identifier var; > statement stmt; > fresh identifier var_noinit = var##" __no_initialize"; > @@ > -T var; > +T var_noinit; > ... > if (copy_from_user(&var,..., sizeof(var))) stmt > ============================ > is better, because it: > - uses a "fresh identifier" hack instead of dealing with attributes > (which aren't supported by spatch 1.0.4) > - seems to be idempotent because of that hack. > > I'll regenerate the binder patch using that script and will look into > enhancing it and committing it to scripts/coccinelle. Cool; sounds good! -- Kees Cook _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
