Re: gcc-10: kernel stack is corrupted and fails to boot

From: Borislav Petkov <bp@suse.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Arvind Sankar <nivedita@alum.mit.edu>,
	Kalle Valo <kvalo@codeaurora.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	the arch/x86 maintainers <x86@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: gcc-10: kernel stack is corrupted and fails to boot
Date: Thu, 14 May 2020 01:36:16 +0200	[thread overview]
Message-ID: <20200513233616.GD6733@zn.tnic> (raw)
In-Reply-To: <CAHk-=wgybuOF+Jp2XYWqM7Xn1CW6szWQw_FgVoFh5jx_4YoCVw@mail.gmail.com>

On Wed, May 13, 2020 at 04:13:53PM -0700, Linus Torvalds wrote:
> The check itself doesn't seem worth it. If your worry is that an empty
> asm() can be optimized away, then don't use an empty asm!

gcc guys said we should use that since the first attempt using

__attribute__((optimize("-fno-stack-protector")))

didn't work because, well, that attribute turned out to be "not suitable in
production code". :)

Full thread here:

https://lore.kernel.org/lkml/20200314164451.346497-1-slyfox@gentoo.org/

> In other words, the only reason for that check seems to be a worry
> that simply isn't worth having.

Yes, that was me asking for a way to check whether any future gccs would
violate that. But if they'd do that, they would break a lot of code
depending on it.

> In fact, I think the check is wrong anyway, since the main thing I can
> see that would do a tailcall despite the empty asm is link-time
> optimizations that that check doesn't even check for!
> 
> So everything I see there just screams "the check is bogus" to me. The
> check doesn't work, and if it were to work it only means that the
> prevent_tail_call_optimization() thing is too fragile.

So I did test it trivially by removing the asm("") and then it would
tailcall optimize. But we didn't think about LTO so hm, that would
probably break it.

> Just put a full memory barrier in there, with an actual "mfence"
> instruction or whatever, so that you know that the check is pointless,
> and so that you know that a link-time optimizer can't turn the
> call+return into a tailcall.

Right, the intention here was to have it arch-agnostic in
include/linux/compiler.h because powerpc might need it too soon:

arch/powerpc/kernel/smp.c:1296: boot_init_stack_canary();

Looking at them, they do have an mb() too so how about this then
instead?

#define prevent_tail_call_optimization()	mb()

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg