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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202002191347.B689E5B43A@keescook>
Date:   Wed, 19 Feb 2020 13:58:05 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Alexander Potapenko <glider@...gle.com>
Cc:     Jann Horn <jannh@...gle.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lib/test_stackinit: move a local outside the switch
 statement

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@...omium.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