Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 19 Feb 2020 13:58:05 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Alexander Potapenko <glider@google.com>
Cc:     Jann Horn <jannh@google.com>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] lib/test_stackinit: move a local outside the switch
 statement
Message-ID: <202002191347.B689E5B43A@keescook>
References: <20200218094815.233387-1-glider@google.com>
 <202002190916.EFA74B50C@keescook>
 <CAG_fn=X-BeOooHDCKczm+KzWDBp_TY5e2VTnUxiqbHpipoF-sg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAG_fn=X-BeOooHDCKczm+KzWDBp_TY5e2VTnUxiqbHpipoF-sg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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