Re: [PATCH 1/2] gcc-plugins: structleak: Generalize to all variable types

[Kees Cook <keescook@chromium.org>]

On Mon, Mar 11, 2019 at 4:05 PM Alexander Popov <alex.popov@linux.com> wrote:
> Hello Kees, hello everyone,
>
> On 12.02.2019 21:04, Kees Cook wrote:
> > Building with CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL should give the
> > kernel complete initialization coverage of all stack variables passed
> > by reference, including padding (see lib/test_stackinit.c).
>
> I would like to note that new STRUCTLEAK_BYREF_ALL initializes *less* stack
> variables than STACKINIT, that was introduced earlier:
> https://www.openwall.com/lists/kernel-hardening/2019/01/23/3
>
> Citing the patches:
> - the STACKINIT plugin "attempts to perform unconditional initialization of all
> stack variables";
> - the STRUCTLEAK_BYREF_ALL feature "gives the kernel complete initialization
> coverage of all stack variables passed by reference".

That's true, yes. STACKINIT was a port of Florian's patch to gcc which
looked only for missing initialization. However, this collides with
warnings about missing initialization. :)

So, given the case that the kernel builds with -Wuninitialized and
-Wmaybe-uninitialized, the preferred method of dealing with
non-by-reference missed initializations is to fix the code itself.
(i.e. kernel builds are expected to build without warnings.) It's only
the byref cases that there is no warning (and most authors refuse to
initialize such cases). Have there been security flaws where gcc
failed to warn a missed initialization that wasn't a byref case?

> I.e. stack variables not passed by reference are not initialized by
> STRUCTLEAK_BYREF_ALL.

Correct.

> Kees, what do you think about adding such cases to your lib/test_stackinit.c?
> This simple example demonstrates the idea:
>
>
> diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
> index 13115b6..f9ef313 100644
> --- a/lib/test_stackinit.c
> +++ b/lib/test_stackinit.c
> @@ -320,9 +320,18 @@ static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill,
>  DEFINE_TEST_DRIVER(switch_1_none, uint64_t, SCALAR);
>  DEFINE_TEST_DRIVER(switch_2_none, uint64_t, SCALAR);
>
> +struct x {
> +       int x1;
> +       int x2;
> +       int x3;
> +};
> +
>  static int __init test_stackinit_init(void)
>  {
>         unsigned int failures = 0;
> +       struct x _x;
> +
> +       printk("uninitialized struct fields sum: %d\n", _x.x1 + _x.x2 + _x.x3);

This would trip the build warnings:

In file included from ./include/linux/kernel.h:15:0,
                 from lib/test_stackinit.c:9:
lib/test_stackinit.c: In function ‘test_stackinit_init’:
./include/linux/printk.h:309:2: warning: ‘__x.x1’ is used
uninitialized in this function [-Wuninitialized]
  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
  ^~~~~~

but those could be silenced for this object specifically if we really
wanted to add it. I think it'd be fine to add this to the test, but
it's a known state, though, so I hadn't bothered with it.

-- 
Kees Cook