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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Feb 2020 18:56:38 +0100
From:   Alexander Potapenko <glider@...gle.com>
To:     Kees Cook <keescook@...omium.org>
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 6:36 PM Kees Cook <keescook@...omium.org> wrote:
>
> On Tue, Feb 18, 2020 at 10:48:15AM +0100, glider@...gle.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@...omium.org>
> > Signed-off-by: Alexander Potapenko <glider@...gle.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