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;
MIME-Version: 1.0
References: <20200218094815.233387-1-glider@google.com> <202002190916.EFA74B50C@keescook>
In-Reply-To: <202002190916.EFA74B50C@keescook>
From:   Alexander Potapenko <glider@google.com>
Date:   Wed, 19 Feb 2020 18:56:38 +0100
Message-ID: <CAG_fn=X-BeOooHDCKczm+KzWDBp_TY5e2VTnUxiqbHpipoF-sg@mail.gmail.com>
Subject: Re: [PATCH] lib/test_stackinit: move a local outside the switch statement
To:     Kees Cook <keescook@chromium.org>
Cc:     Jann Horn <jannh@google.com>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 =E2=80=98send_sigio_to_task=E2=80=99:
> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-un=
reachable]
>    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=
=3Dkspp/gcc-plugin/stackinit&id=3D35ed32e16e13a86370e4b70991db8d5f771ba898

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 =3D &var;
> >               target_size =3D sizeof(var);
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >
>
> --
> Kees Cook


--=20
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Stra=C3=9Fe, 33
80636 M=C3=BCnchen

Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg