linux-kernel - Re: [PATCH] x86: Fix early boot crash on gcc-10, next try

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]

Message-ID: <20200425150440.GA470719@rani.riverdale.lan>
Date:   Sat, 25 Apr 2020 11:04:40 -0400
From:   Arvind Sankar <nivedita@...m.mit.edu>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Arvind Sankar <nivedita@...m.mit.edu>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Michael Matz <matz@...e.de>, Jakub Jelinek <jakub@...hat.com>,
        Sergei Trofimovich <slyfox@...too.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        Kees Cook <keescook@...omium.org>,
        Martin Liška <mliska@...e.cz>,
        Frédéric Pierret (fepitre) 
        <frederic.pierret@...es-os.org>, boris.ostrovsky@...cle.com,
        jgross@...e.com, linuxppc-dev@...ts.ozlabs.org,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH] x86: Fix early boot crash on gcc-10, next try

On Sat, Apr 25, 2020 at 10:57:59AM +0200, Borislav Petkov wrote:
> On Fri, Apr 24, 2020 at 09:46:57PM -0400, Arvind Sankar wrote:
> > The comment above boot_init_stack_canary's definition should be updated
> > to note that it needs to be called from a function that, in addition to
> > not returning, either has stackprotector disabled or avoids ending in a
> > tail call.
> 
> How's that?
> 
> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
> index 91e29b6a86a5..237a54f60d6b 100644
> --- a/arch/x86/include/asm/stackprotector.h
> +++ b/arch/x86/include/asm/stackprotector.h
> @@ -55,8 +55,12 @@
>  /*
>   * Initialize the stackprotector canary value.
>   *
> - * NOTE: this must only be called from functions that never return,
> - * and it must always be inlined.
> + * NOTE: this must only be called from functions that never return, it must
> + * always be inlined and it should be called from a compilation unit for
> + * which stack protector is disabled.
> + *
> + * Alternatively, the caller should not end with a function call which gets
> + * tail-call optimized as that would lead to checking a modified canary value.
>   */
>  static __always_inline void boot_init_stack_canary(void)
>  {

I'd put the clause about stack protector being disabled and the
tail-call one together, to make clear that you still need the never
return and always inline bits. Also, this function is implemented by
multiple arch's and they all have similar comments -- might be better to
consolidate the comment in the generic (dummy) one in
include/linux/stackprotector.h laying out the restrictions that arch
implementations should follow?

> 
> > There are also other calls that likely need to be fixed as well -- in
> > init/main.c, arch/x86/xen/smp_pv.c, and there is a powerpc version of
> > start_secondary in arch/powerpc/kernel/smp.c which may also be affected.
> 
> Yes, there was an attempt to fix former:
> 
> https://lkml.kernel.org/r/20200413123535.10884-1-frederic.pierret@qubes-os.org

There's also the one in init/main.c which is used by multiple
architectures. On x86 at least, the call to arch_call_rest_init at the
end of start_kernel does not get tail-call optimized by gcc-10, but I
don't see anything that actually prevents that from happening. We should
add the asm("") there as well I think, unless the compiler guys see
something about this function that will always prevent the optimization?

Cc'ing PPC list for powerpc start_secondary.