[PATCH] lib/test_stackinit: move a local outside the switch statement

[PATCH] lib/test_stackinit: move a local outside the switch statement

[glider]

Right now CONFIG_INIT_STACK_ALL is unable to initialize locals declared
in switch statements, see http://llvm.org/PR44916.
Move the variable declaration outside the switch in lib/test_stackinit.c
to prevent potential test failures until this is sorted out.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 lib/test_stackinit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
index 2d7d257a430e..41e2a6e0cdaa 100644
--- a/lib/test_stackinit.c
+++ b/lib/test_stackinit.c
@@ -282,9 +282,9 @@ DEFINE_TEST(user, struct test_user, STRUCT, none);
  */
 static int noinline __leaf_switch_none(int path, bool fill)
 {
-	switch (path) {
-		uint64_t var;
+	uint64_t var;
 
+	switch (path) {
 	case 1:
 		target_start = &var;
 		target_size = sizeof(var);
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH] lib/test_stackinit: move a local outside the switch statement

[Kees Cook]

On Tue, Feb 18, 2020 at 10:48:15AM +0100, glider@google.com wrote:
> Right now CONFIG_INIT_STACK_ALL is unable to initialize locals declared
> in switch statements, see http://llvm.org/PR44916.
> Move the variable declaration outside the switch in lib/test_stackinit.c
> to prevent potential test failures until this is sorted out.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Er, no. This test is specifically to catch that case. (i.e. the proposed
GCC version of this feature misses this case too.) We absolutely want
this test to continue to fail until it's fixed:

[   65.546670] test_stackinit: switch_1_none FAIL (uninit bytes: 8)
[   65.547478] test_stackinit: switch_2_none FAIL (uninit bytes: 8)

What would be nice is if Clang could at least _warn_ about these
conditions. GCC does this in the same situation:

fs/fcntl.c: In function ‘send_sigio_to_task’:
fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
   siginfo_t si;
             ^~

I have a patch to fix all the switch statement variables, though:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackinit&id=35ed32e16e13a86370e4b70991db8d5f771ba898

-Kees

> ---
>  lib/test_stackinit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
> index 2d7d257a430e..41e2a6e0cdaa 100644
> --- a/lib/test_stackinit.c
> +++ b/lib/test_stackinit.c
> @@ -282,9 +282,9 @@ DEFINE_TEST(user, struct test_user, STRUCT, none);
>   */
>  static int noinline __leaf_switch_none(int path, bool fill)
>  {
> -	switch (path) {
> -		uint64_t var;
> +	uint64_t var;
>  
> +	switch (path) {
>  	case 1:
>  		target_start = &var;
>  		target_size = sizeof(var);
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 

-- 
Kees Cook

Re: [PATCH] lib/test_stackinit: move a local outside the switch statement

[Alexander Potapenko]

On Wed, Feb 19, 2020 at 6:36 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Feb 18, 2020 at 10:48:15AM +0100, glider@google.com wrote:
> > Right now CONFIG_INIT_STACK_ALL is unable to initialize locals declared
> > in switch statements, see http://llvm.org/PR44916.
> > Move the variable declaration outside the switch in lib/test_stackinit.c
> > to prevent potential test failures until this is sorted out.
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
>
> Er, no. This test is specifically to catch that case. (i.e. the proposed
> GCC version of this feature misses this case too.) We absolutely want
> this test to continue to fail until it's fixed:
>
> [   65.546670] test_stackinit: switch_1_none FAIL (uninit bytes: 8)
> [   65.547478] test_stackinit: switch_2_none FAIL (uninit bytes: 8)
>
> What would be nice is if Clang could at least _warn_ about these
> conditions. GCC does this in the same situation:
>
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
>    siginfo_t si;
>              ^~

Is this really a bug from the C Standard point of view?
Initializing the switch-local variable or executing other code after
the switch would indeed be a problem, but isn't it correct to declare
an uninitialized local as long as it's initialized in the branches
that use it?

> I have a patch to fix all the switch statement variables, though:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackinit&id=35ed32e16e13a86370e4b70991db8d5f771ba898

Am I understanding right that these warnings only show up in the
instrumented build?
According to the GCC manual:

   -Wswitch-unreachable does not warn if the statement between the
controlling expression and the first case label is just a declaration

> -Kees
>
> > ---
> >  lib/test_stackinit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
> > index 2d7d257a430e..41e2a6e0cdaa 100644
> > --- a/lib/test_stackinit.c
> > +++ b/lib/test_stackinit.c
> > @@ -282,9 +282,9 @@ DEFINE_TEST(user, struct test_user, STRUCT, none);
> >   */
> >  static int noinline __leaf_switch_none(int path, bool fill)
> >  {
> > -     switch (path) {
> > -             uint64_t var;
> > +     uint64_t var;
> >
> > +     switch (path) {
> >       case 1:
> >               target_start = &var;
> >               target_size = sizeof(var);
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >
>
> --
> Kees Cook



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] lib/test_stackinit: move a local outside the switch statement

[Kees Cook]

On Wed, Feb 19, 2020 at 06:56:38PM +0100, Alexander Potapenko wrote:
> On Wed, Feb 19, 2020 at 6:36 PM Kees Cook <keescook@chromium.org> wrote:
> Am I understanding right that these warnings only show up in the
> instrumented build?

Correct.

> According to the GCC manual:
> 
>    -Wswitch-unreachable does not warn if the statement between the
> controlling expression and the first case label is just a declaration

Right, just a declaration is okay. An initializer is not handled:

        switch (argc) {
                int foo = 0;
        case 0:
...

foo.c:6:7: warning: statement will never be executed
[-Wswitch-unreachable]
    6 |   int foo = 0;
      |       ^~~

The problem I had with the "simple" stackinit GCC plugin was that it
didn't handle padding. What I don't understand is why structleak (with
seemingly the same initialization) _does_ initialize padding:


structleak:

        PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
...

        /* build the initializer expression */
        type = TREE_TYPE(var);
        if (AGGREGATE_TYPE_P(type))
                initializer = build_constructor(type, NULL);
        else
                initializer = fold_convert(type, integer_zero_node);

        /* build the initializer stmt */
        init_stmt = gimple_build_assign(var, initializer);
        gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
        gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
        update_stmt(init_stmt);

vs stackinit:

        register_callback(plugin_name, PLUGIN_FINISH_DECL, finish_decl, NULL);
...

        type = TREE_TYPE (decl);
        if (AGGREGATE_TYPE_P (type))
                DECL_INITIAL (decl) = build_constructor (type, NULL);
        else
                DECL_INITIAL (decl) = fold_convert (type, integer_zero_node);

I assume the difference is due to either pass ordering or the former's
basic block splitting. I haven't had time to dig in and figure it out.

-- 
Kees Cook