linux-kernel - Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)

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: <CAKwvOdmKaYYxf7vjvPf2vbn-Ly+4=JZ_zf+OcjYOkWCkgyU_kA@mail.gmail.com>
Date: Fri, 9 Feb 2024 10:55:42 -0800
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, 
	"Andrew Pinski (QUIC)" <quic_apinski@...cinc.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Masahiro Yamada <masahiroy@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11
 (and earlier)

On Fri, Feb 9, 2024 at 10:43 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Fri, Feb 09, 2024, Linus Torvalds wrote:
> > On Fri, 9 Feb 2024 at 09:14, Andrew Pinski (QUIC)
> > <quic_apinski@...cinc.com> wrote:
> > >
> > > So the exact versions of GCC where this is/was fixed are:
> > > 12.4.0 (not released yet)
> > > 13.2.0
> > > 14.1.0 (not released yet)
> >
> > Looking at the patch that the bugzilla says is the fix, it *looks*
> > like it's just the "mark volatile" that is missing.
> >
> > But Sean says that  even if we mark "asm goto" as volatile manually,
> > it still fails.
> >
> > So there seems to be something else going on in addition to just the volatile.
>
> Aha!  Yeah, there's a second bug that set things up so that the "not implicitly
> volatile" bug could rear its head.  (And now I feel less bad for not suspecting
> the compiler sooner, because it didn't occur to me that gcc could possibly think
> the asm blob had no used outputs).
>
> With "volatile" forced, gcc generates code for the asm blob, but doesn't actually
> consume the output of the VMREAD.  As a result, the optimization pass sees the
> unused output and throws it away because the blob isn't treated as volatile.
>
>    vmread %rax,%rax       <= output register is unused
>    jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
>    xor    %r12d,%r12d     <= one of the "return 0" statements
>    mov    %r12,0xf0(%rbx) <= store the output
>
> > Side note: the reason we have that "asm_volatile_goto()" define in the
> > kernel is that we *used* to have a _different_ workaround for a gcc
> > bug in this area:
> >
> >  /*
> >   * GCC 'asm goto' miscompiles certain code sequences:
> >   *
> >   *   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670
> >   *
> >   * Work it around via a compiler barrier quirk suggested by Jakub Jelinek.
> >   *
> >   * (asm goto is automatically volatile - the naming reflects this.)
> >   */
> >  #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
> >
> > and looking at that (old) bugzilla there seems to be a lot of "seems
> > to be fixed", but it's not entirely clear.
> >
> > We've removed that workaround in commit 43c249ea0b1e ("compiler-gcc.h:

Interesting, I thought I had proposed removing that earlier and Linus
had yelled about doing so.
https://lore.kernel.org/lkml/20180907222109.163802-1-ndesaulniers@google.com/
seems like in ~2018 I was trying to, but can't seem to find the thread
where Linus pushed back.

> > remove ancient workaround for gcc PR 58670"), I'm wondering if maybe
> > that removal was a bit optimistic.
>
> FWIW, reverting that does restore correct behavior on gcc-11.

And also pessimizes all asm gotos (for gcc) including ones that don't
contain output as described in 43c249ea0b1e.  The version checks would
at least not pessimize those.

>
> Note, this is 100% reproducible across multiple systems, though AFAICT it's
> somewhat dependent on the .config.  Holler if anyone wants the .config.



-- 
Thanks,
~Nick Desaulniers