Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Woodhouse, David]

On Thu, 2018-01-18 at 08:52 +0100, Uros Bizjak wrote:
> This puts an extra burden on the developer, which has to use correct
> thunk name in their code. Sure, this can be solved trivially with
> #ifdef __x86_64__, so the issue is minor, but I thought it has to be
> mentioned before the name is set in stone.

Except the developer can mostly use the inline thunk and not have to
worry their pretty little heads about it. And the kernel developers who
*have* chosen to implement their own thunk (and who requested the
thunk-extern variant in the first place) have explicitly asked for the
symbol name to be as it is.

I spent a happy few hours on Sunday night looking at this. Seriously,
in neither the thunk implementation nor the call sites (in explicit
asm) was it easier. It was horrid — just look at the patch I sent out
with the footnote "let's not do this".

In the thunk I still had the actual register name with the 'e' or the
'r' in a macro argument anyway, because I had to deal with the value
from that register. So the thunk macro ended up taking *two* arguments
because its name now didn't match what it had to call the register.

And in the call sites I was putting the target into a register again
using either 'eax' or 'rax' form, then having to branch to the thunk
using the different '_ax' form. It just hurt.



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Wed, Jan 17, 2018 at 7:29 PM, Woodhouse, David  wrote:
> I'm not sure I understand the concern. When compiling a large project for
> -m32 vs. -m64, there must be a million times the compiler has to decide
> whether to emit "r" or "e" before a register name. HJ's patch already does
> this for the thunk symbol. What is the future requirement that I am not
> understanding, and that is so hard?

No, the concern is not with one extra fputc in the compiler.

IIRC, these thunks are also intended to be called from c code. So,
when one compiles this code on 64bit target, the thunk has different
name than when mentioned code is compiled on 32bit target. This puts
an extra burden on the developer, which has to use correct thunk name
in their code. Sure, this can be solved trivially with #ifdef
__x86_64__, so the issue is minor, but I thought it has to be
mentioned before the name is set in stone.

BTW: The names of the registers are ax, bx, di, si, bp, ... and this
is reflected in 32bit PIC thunk names. "e" prefix stands for
"extended", and "r" was added to be consistent with r8 ... r15. The
added pack of registers on 64bit target has different naming rules for
sub-word access, e.g. r8b, r10w, r12d.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Woodhouse, David]

I'm not sure I understand the concern. When compiling a large project for -m32 
vs. -m64, there must be a million times the compiler has to decide whether to 
emit "r" or "e" before a register name. HJ's patch already does this for the 
thunk symbol. What is the future requirement that I am not understanding, and 
that is so hard?

Back to real computer and will stop top-posting HTML soon, I promise!

On 14 Jan 2018 19:22, Uros Bizjak  wrote:

On Sun, Jan 14, 2018 at 6:44 PM, Woodhouse, David  wrote:
> This won't make the list; I'll send a more coherent and less HTML-afflicted
> version later.
>
> The bare 'ax' naming made it painful to instantiate the external thunks for
> 32-bit and 64-bot code because we had to put the e/r back again inside the
> .irp reg ax bx... code.
>
> We could probably have lived with that but it would be painful to change now
> that Linux and Xen patches with the current ABI are all lined up. I
> appreciate they weren't in GCC yet so we get little sympathy but these are
> strange times and we had to move fast.
>
> I'd really like *not* to change it now. Having the thunk name actually
> include the name of the register it's using does seem nicer anyway...

That's unfortunate... I suspect that in the future, one will need
#ifdef __x86_64__ around eventual calls to thunks from c code because
of this decision, since thunks for x86_64 target will have different
names than thunks for x86_32 target. I don't know if the (single?)
case of mixing 32 and 64 bit assembly in the highly specialized part
of the kernel really warrants this decision. Future programmers will
be grateful if kernel people can re-consider their choice in
not-yet-release ABI.

Uros.




Amazon Web Services UK Limited. Registered in England and Wales with 
registration number 08650665 and which has its registered office at 60 Holborn 
Viaduct, London EC1A 2FD, United Kingdom.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 5:43 PM, Uros Bizjak  wrote:
> On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
>> On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
>>> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
 On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:

> Hi Uros,
>
> Can you take a look at my x86 backend changes so that they are ready
> to check in once we have consensus.

 Please finish the talks about the correct approach first. Once the
 consensus is reached, please post the final version of the patches for
 review.

 BTW: I have no detailed insight in these issues, so I'll look mostly
 at the implementation details, probably early next week.
>>>
>>> One general remark is on the usage of -1 as an invalid register
>>
>> This has been rewritten.  The checked in patch no longer does that.
>
> I'm looking directly into current indirect_thunk_name,
> output_indirect_thunk and output_indirect_thunk_function functions in
> i386.c which have plenty of the mentioned checks.

Improved with attached patch.

2018-01-16  Uros Bizjak  

* config/i386/i386.c (indirect_thunk_name): Declare regno
as unsigned int.  Compare regno with INVALID_REGNUM.
(output_indirect_thunk): Ditto.
(output_indirect_thunk_function): Ditto.
(ix86_code_end): Declare regno as unsigned int.  Use INVALID_REGNUM
in the call to output_indirect_thunk_function.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Uros.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ea9c462..7f233d1 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10765,16 +10765,16 @@ static int indirect_thunks_bnd_used;
 /* Fills in the label name that should be used for the indirect thunk.  */
 
 static void
-indirect_thunk_name (char name[32], int regno, bool need_bnd_p,
-bool ret_p)
+indirect_thunk_name (char name[32], unsigned int regno,
+bool need_bnd_p, bool ret_p)
 {
-  if (regno >= 0 && ret_p)
+  if (regno != INVALID_REGNUM && ret_p)
 gcc_unreachable ();
 
   if (USE_HIDDEN_LINKONCE)
 {
   const char *bnd = need_bnd_p ? "_bnd" : "";
-  if (regno >= 0)
+  if (regno != INVALID_REGNUM)
{
  const char *reg_prefix;
  if (LEGACY_INT_REGNO_P (regno))
@@ -10792,7 +10792,7 @@ indirect_thunk_name (char name[32], int regno, bool 
need_bnd_p,
 }
   else
 {
-  if (regno >= 0)
+  if (regno != INVALID_REGNUM)
{
  if (need_bnd_p)
ASM_GENERATE_INTERNAL_LABEL (name, "LITBR", regno);
@@ -10844,7 +10844,7 @@ indirect_thunk_name (char name[32], int regno, bool 
need_bnd_p,
  */
 
 static void
-output_indirect_thunk (bool need_bnd_p, int regno)
+output_indirect_thunk (bool need_bnd_p, unsigned int regno)
 {
   char indirectlabel1[32];
   char indirectlabel2[32];
@@ -10874,7 +10874,7 @@ output_indirect_thunk (bool need_bnd_p, int regno)
 
   ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel2);
 
-  if (regno >= 0)
+  if (regno != INVALID_REGNUM)
 {
   /* MOV.  */
   rtx xops[2];
@@ -10898,12 +10898,12 @@ output_indirect_thunk (bool need_bnd_p, int regno)
 }
 
 /* Output a funtion with a call and return thunk for indirect branch.
-   If BND_P is true, the BND prefix is needed.   If REGNO != -1,  the
-   function address is in REGNO.  Otherwise, the function address is
+   If BND_P is true, the BND prefix is needed.  If REGNO != UNVALID_REGNUM,
+   the function address is in REGNO.  Otherwise, the function address is
on the top of stack.  */
 
 static void
-output_indirect_thunk_function (bool need_bnd_p, int regno)
+output_indirect_thunk_function (bool need_bnd_p, unsigned int regno)
 {
   char name[32];
   tree decl;
@@ -10952,7 +10952,7 @@ output_indirect_thunk_function (bool need_bnd_p, int 
regno)
ASM_OUTPUT_LABEL (asm_out_file, name);
   }
 
-  if (regno < 0)
+  if (regno == INVALID_REGNUM)
 {
   /* Create alias for __x86.return_thunk/__x86.return_thunk_bnd.  */
   char alias[32];
@@ -11026,16 +11026,16 @@ static void
 ix86_code_end (void)
 {
   rtx xops[2];
-  int regno;
+  unsigned int regno;
 
   if (indirect_thunk_needed)
-output_indirect_thunk_function (false, -1);
+output_indirect_thunk_function (false, INVALID_REGNUM);
   if (indirect_thunk_bnd_needed)
-output_indirect_thunk_function (true, -1);
+output_indirect_thunk_function (true, INVALID_REGNUM);
 
   for (regno = FIRST_REX_INT_REG; regno <= LAST_REX_INT_REG; regno++)
 {
-  int i = regno - FIRST_REX_INT_REG + LAST_INT_REG + 1;
+  unsigned int i = regno - FIRST_REX_INT_REG + LAST_INT_REG + 1;
   if ((indirect_thunks_used & (1 << i)))
output_indirect_thunk_function (false, regno);
 

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Rainer Orth]

Hi Richard,

> I'm quite sure Solaris supports comdats, after all it invented ELF ;)

true: gcc/configure.ac has

  # Sun ld has COMDAT group support since Solaris 9, but it doesn't
  # interoperate with GNU as until Solaris 11 build 130, i.e. ld
  # version 1.688.
  #
  # If using Sun as for COMDAT group as emitted by GCC, one needs at
  # least ld version 1.2267.

> I've also seen
> comdats in debugging early LTO issues.  We might run into Solaris as
> issues though.

The Solaris code has been taught to deal with that, so it should
hopefully be hidden from the rest of the compiler.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Rainer Orth]

Hi Jan,

>> It makes the option using thunks unusable though, right?  Can you simply make
>> them hidden on systems without comdat support?  That duplicates them per TU
>> but at least the feature works.  Or those systems should provide the
>> thunks via
>> libgcc.
>> 
>> I agree we can followup with a fix for Solaris given lack of a public
>> testing machine.
>
> My memory is bit dim, but I am convinced I was fixing specific errors for
> comdats
> on Solaris, so I think the toolchain supports them in some sort, just is more
> restrictive/different from GNU implementation.

comdat does work just fine in Solaris 11, but the Solaris 10 linker has
problems with what gcc generates.

> Indeed, i think just producing sorry, unimplemented message is what we should 
> do
> if we can't support retpoline on given target.

Certainly, coupled with an appropriate effective-target keyword to limit
testcases appropriately.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Rainer Orth]

Hi Richard,

>>> Backport is blocked by
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
>>>
>>> There are many test failures due to lack of comdat support in linker on
>>> Solaris.

actually this is lack of hidden .gnu.linkonce support right now.
Currently that's disabled for all but gld; I'm looking to make that
dynamic on newer versions of Solaris 11.

>>> I can limit these tests to Linux.
>>
>> These are testcase issues and shouldn't block backport to GCC 7.
>
> It makes the option using thunks unusable though, right?  Can you simply make
> them hidden on systems without comdat support?  That duplicates them per TU
> but at least the feature works.  Or those systems should provide the thunks 
> via
> libgcc.
>
> I agree we can followup with a fix for Solaris given lack of a public
> testing machine.

I do have both an x86 and sparc machine running Solaris 11 around to
serve as testing machines.  Still checking with legal how best to handle
external access, either locally or integrated into the compile farm.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Tue, Jan 16, 2018 at 12:34 AM, Jan Hubicka  wrote:
>> On Mon, Jan 15, 2018 at 5:53 PM, H.J. Lu  wrote:
>> > On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
>> >> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
>> >>  wrote:
>> >>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
>>  Now my patch set has been checked into trunk.  Here is a patch set
>>  to move struct ix86_frame to machine_function on GCC 7, which is
>>  needed to backport the patch set to GCC 7:
>> 
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
>> 
>>  OK for gcc-7-branch?
>> >>>
>> >>> Yes, backporting is ok - please watch for possible fallout on trunk and 
>> >>> make
>> >>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
>> >>> Wednesday now with the final release about a week later if no issue shows
>> >>> up.
>> >>>
>> >>
>> >> Backport is blocked by
>> >>
>> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
>> >>
>> >> There are many test failures due to lack of comdat support in linker on 
>> >> Solaris.
>> >> I can limit these tests to Linux.
>> >
>> > These are testcase issues and shouldn't block backport to GCC 7.
>>
>> It makes the option using thunks unusable though, right?  Can you simply make
>> them hidden on systems without comdat support?  That duplicates them per TU
>> but at least the feature works.  Or those systems should provide the thunks 
>> via
>> libgcc.
>>
>> I agree we can followup with a fix for Solaris given lack of a public
>> testing machine.
>
> My memory is bit dim, but I am convinced I was fixing specific errors for 
> comdats
> on Solaris, so I think the toolchain supports them in some sort, just is more
> restrictive/different from GNU implementation.
>
> Indeed, i think just producing sorry, unimplemented message is what we should 
> do
> if we can't support retpoline on given target.
>

It still works without comdat.  GCC just generate a local thunk in each object
file.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> On Tue, Jan 16, 2018 at 9:34 AM, Jan Hubicka  wrote:
> >> On Mon, Jan 15, 2018 at 5:53 PM, H.J. Lu  wrote:
> >> > On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
> >> >> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
> >> >>  wrote:
> >> >>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
> >>  Now my patch set has been checked into trunk.  Here is a patch set
> >>  to move struct ix86_frame to machine_function on GCC 7, which is
> >>  needed to backport the patch set to GCC 7:
> >> 
> >>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
> >>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
> >>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
> >> 
> >>  OK for gcc-7-branch?
> >> >>>
> >> >>> Yes, backporting is ok - please watch for possible fallout on trunk 
> >> >>> and make
> >> >>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
> >> >>> Wednesday now with the final release about a week later if no issue 
> >> >>> shows
> >> >>> up.
> >> >>>
> >> >>
> >> >> Backport is blocked by
> >> >>
> >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
> >> >>
> >> >> There are many test failures due to lack of comdat support in linker on 
> >> >> Solaris.
> >> >> I can limit these tests to Linux.
> >> >
> >> > These are testcase issues and shouldn't block backport to GCC 7.
> >>
> >> It makes the option using thunks unusable though, right?  Can you simply 
> >> make
> >> them hidden on systems without comdat support?  That duplicates them per TU
> >> but at least the feature works.  Or those systems should provide the 
> >> thunks via
> >> libgcc.
> >>
> >> I agree we can followup with a fix for Solaris given lack of a public
> >> testing machine.
> >
> > My memory is bit dim, but I am convinced I was fixing specific errors for 
> > comdats
> > on Solaris, so I think the toolchain supports them in some sort, just is 
> > more
> > restrictive/different from GNU implementation.
> >
> > Indeed, i think just producing sorry, unimplemented message is what we 
> > should do
> > if we can't support retpoline on given target.
> 
> I'm quite sure Solaris supports comdats, after all it invented ELF ;)
> I've also seen
> comdats in debugging early LTO issues.  We might run into Solaris as
> issues though.

:)
My recollection is that the thunks in a comdat group needs to come in specific
order after the entry symbol. Probably after - at some point I tried to move the
before (for better code layout) and needed to retreat.

Honza

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Tue, Jan 16, 2018 at 9:34 AM, Jan Hubicka  wrote:
>> On Mon, Jan 15, 2018 at 5:53 PM, H.J. Lu  wrote:
>> > On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
>> >> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
>> >>  wrote:
>> >>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
>>  Now my patch set has been checked into trunk.  Here is a patch set
>>  to move struct ix86_frame to machine_function on GCC 7, which is
>>  needed to backport the patch set to GCC 7:
>> 
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
>>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
>> 
>>  OK for gcc-7-branch?
>> >>>
>> >>> Yes, backporting is ok - please watch for possible fallout on trunk and 
>> >>> make
>> >>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
>> >>> Wednesday now with the final release about a week later if no issue shows
>> >>> up.
>> >>>
>> >>
>> >> Backport is blocked by
>> >>
>> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
>> >>
>> >> There are many test failures due to lack of comdat support in linker on 
>> >> Solaris.
>> >> I can limit these tests to Linux.
>> >
>> > These are testcase issues and shouldn't block backport to GCC 7.
>>
>> It makes the option using thunks unusable though, right?  Can you simply make
>> them hidden on systems without comdat support?  That duplicates them per TU
>> but at least the feature works.  Or those systems should provide the thunks 
>> via
>> libgcc.
>>
>> I agree we can followup with a fix for Solaris given lack of a public
>> testing machine.
>
> My memory is bit dim, but I am convinced I was fixing specific errors for 
> comdats
> on Solaris, so I think the toolchain supports them in some sort, just is more
> restrictive/different from GNU implementation.
>
> Indeed, i think just producing sorry, unimplemented message is what we should 
> do
> if we can't support retpoline on given target.

I'm quite sure Solaris supports comdats, after all it invented ELF ;)
I've also seen
comdats in debugging early LTO issues.  We might run into Solaris as
issues though.

Richard.

> Honza
>>
>> Richard.
>>
>> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83839
>> >>
>> >> Bootstrap failed on Dawning due to lack of ".set" directive in assembler. 
>> >>  I
>> >> uploaded a patch:
>> >>
>> >> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43124
>> >>
>> >> There is no confirmation on it.  Also there may be test failures on 
>> >> Dardwin
>> >> due to difference in assembly output.
>> >
>> > I posted a patch for Darwin build:
>> >
>> > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01347.html
>> >
>> > This needs to be checked into trunk before I can start backport to GCC 7.
>> >
>> > --
>> > H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> On Mon, Jan 15, 2018 at 5:53 PM, H.J. Lu  wrote:
> > On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
> >> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
> >>  wrote:
> >>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
>  Now my patch set has been checked into trunk.  Here is a patch set
>  to move struct ix86_frame to machine_function on GCC 7, which is
>  needed to backport the patch set to GCC 7:
> 
>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
>  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
> 
>  OK for gcc-7-branch?
> >>>
> >>> Yes, backporting is ok - please watch for possible fallout on trunk and 
> >>> make
> >>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
> >>> Wednesday now with the final release about a week later if no issue shows
> >>> up.
> >>>
> >>
> >> Backport is blocked by
> >>
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
> >>
> >> There are many test failures due to lack of comdat support in linker on 
> >> Solaris.
> >> I can limit these tests to Linux.
> >
> > These are testcase issues and shouldn't block backport to GCC 7.
> 
> It makes the option using thunks unusable though, right?  Can you simply make
> them hidden on systems without comdat support?  That duplicates them per TU
> but at least the feature works.  Or those systems should provide the thunks 
> via
> libgcc.
> 
> I agree we can followup with a fix for Solaris given lack of a public
> testing machine.

My memory is bit dim, but I am convinced I was fixing specific errors for 
comdats
on Solaris, so I think the toolchain supports them in some sort, just is more
restrictive/different from GNU implementation.

Indeed, i think just producing sorry, unimplemented message is what we should do
if we can't support retpoline on given target.

Honza
> 
> Richard.
> 
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83839
> >>
> >> Bootstrap failed on Dawning due to lack of ".set" directive in assembler.  
> >> I
> >> uploaded a patch:
> >>
> >> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43124
> >>
> >> There is no confirmation on it.  Also there may be test failures on Dardwin
> >> due to difference in assembly output.
> >
> > I posted a patch for Darwin build:
> >
> > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01347.html
> >
> > This needs to be checked into trunk before I can start backport to GCC 7.
> >
> > --
> > H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Mon, Jan 15, 2018 at 5:53 PM, H.J. Lu  wrote:
> On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
>> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
>>  wrote:
>>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
 Now my patch set has been checked into trunk.  Here is a patch set
 to move struct ix86_frame to machine_function on GCC 7, which is
 needed to backport the patch set to GCC 7:

 https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
 https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
 https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html

 OK for gcc-7-branch?
>>>
>>> Yes, backporting is ok - please watch for possible fallout on trunk and make
>>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
>>> Wednesday now with the final release about a week later if no issue shows
>>> up.
>>>
>>
>> Backport is blocked by
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
>>
>> There are many test failures due to lack of comdat support in linker on 
>> Solaris.
>> I can limit these tests to Linux.
>
> These are testcase issues and shouldn't block backport to GCC 7.

It makes the option using thunks unusable though, right?  Can you simply make
them hidden on systems without comdat support?  That duplicates them per TU
but at least the feature works.  Or those systems should provide the thunks via
libgcc.

I agree we can followup with a fix for Solaris given lack of a public
testing machine.

Richard.

>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83839
>>
>> Bootstrap failed on Dawning due to lack of ".set" directive in assembler.  I
>> uploaded a patch:
>>
>> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43124
>>
>> There is no confirmation on it.  Also there may be test failures on Dardwin
>> due to difference in assembly output.
>
> I posted a patch for Darwin build:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01347.html
>
> This needs to be checked into trunk before I can start backport to GCC 7.
>
> --
> H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Mon, Jan 15, 2018 at 3:38 AM, H.J. Lu  wrote:
> On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
>  wrote:
>> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
>>> Now my patch set has been checked into trunk.  Here is a patch set
>>> to move struct ix86_frame to machine_function on GCC 7, which is
>>> needed to backport the patch set to GCC 7:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
>>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
>>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
>>>
>>> OK for gcc-7-branch?
>>
>> Yes, backporting is ok - please watch for possible fallout on trunk and make
>> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
>> Wednesday now with the final release about a week later if no issue shows
>> up.
>>
>
> Backport is blocked by
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838
>
> There are many test failures due to lack of comdat support in linker on 
> Solaris.
> I can limit these tests to Linux.

These are testcase issues and shouldn't block backport to GCC 7.

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83839
>
> Bootstrap failed on Dawning due to lack of ".set" directive in assembler.  I
> uploaded a patch:
>
> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43124
>
> There is no confirmation on it.  Also there may be test failures on Dardwin
> due to difference in assembly output.

I posted a patch for Darwin build:

https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01347.html

This needs to be checked into trunk before I can start backport to GCC 7.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Mon, Jan 15, 2018 at 12:31 AM, Richard Biener
 wrote:
> On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
>> Now my patch set has been checked into trunk.  Here is a patch set
>> to move struct ix86_frame to machine_function on GCC 7, which is
>> needed to backport the patch set to GCC 7:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
>>
>> OK for gcc-7-branch?
>
> Yes, backporting is ok - please watch for possible fallout on trunk and make
> sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
> Wednesday now with the final release about a week later if no issue shows
> up.
>

Backport is blocked by

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83838

There are many test failures due to lack of comdat support in linker on Solaris.
I can limit these tests to Linux.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83839

Bootstrap failed on Dawning due to lack of ".set" directive in assembler.  I
uploaded a patch:

https://gcc.gnu.org/bugzilla/attachment.cgi?id=43124

There is no confirmation on it.  Also there may be test failures on Dardwin
due to difference in assembly output.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Sun, Jan 14, 2018 at 4:08 PM, H.J. Lu  wrote:
> Now my patch set has been checked into trunk.  Here is a patch set
> to move struct ix86_frame to machine_function on GCC 7, which is
> needed to backport the patch set to GCC 7:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html
>
> OK for gcc-7-branch?

Yes, backporting is ok - please watch for possible fallout on trunk and make
sure to adjust the backport accordingly.  I plan to do GCC 7.3 RC1 on
Wednesday now with the final release about a week later if no issue shows
up.

Thanks for all your work!
Richard.

> Thanks.
>
>
> --
> H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 21:52 +0100, Uros Bizjak wrote:
> 
> Well, you did say that these are strange times ;)
> 
> From the user perspective, it would be more convenient to use the
> thunk names that are the same for 32bit and 64bit targets. If we
> ignore this fact, the difference is only a couple of lines in the
> compiler source which we also can live with. But please discuss my
> proposal also in the kernel community, and weight the benefits and
> drawbacks of each approach before the final decision.
> 
> Please pass the final decision to gcc community, and we'll implement
> it.

I think you watched this happen, but just to be explicitly clear:

We weighed the benefits and tested this, and we concluded that we don't
want it. Let's stick with e.g. __x86_indirect_thunk_rax please.

Thank you for being flexible.

smime.p7s
Description: S/MIME cryptographic signature

RE: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Kumar, Venkataramanan]

Hi 

> -Original Message-
> From: H.J. Lu [mailto:hjl.to...@gmail.com]
> Sent: Sunday, January 14, 2018 7:52 PM
> To: Jan Hubicka <hubi...@ucw.cz>
> Cc: Kumar, Venkataramanan <venkataramanan.ku...@amd.com>; gcc-
> patc...@gcc.gnu.org; Dharmakan, Rohit arul raj
> <rohitarulraj.dharma...@amd.com>; Nagarajan, Muthu kumar raj
> <muthukumarraj.nagara...@amd.com>; Uros Bizjak (ubiz...@gmail.com)
> <ubiz...@gmail.com>
> Subject: Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> 
> On Sun, Jan 14, 2018 at 6:20 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> >> > Hi HJ,
> >> >
> >> > > -Original Message-
> >> > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> >> > > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> >> > > Sent: Sunday, January 14, 2018 9:07 AM
> >> > > To: gcc-patches@gcc.gnu.org
> >> > > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> >> > >
> >> > > This set of patches for GCC 8 mitigates variant #2 of the
> >> > > speculative execution vulnerabilities on x86 processors
> >> > > identified by CVE-2017-5715, aka Spectre.  They convert indirect
> >> > > branches and function returns to call and return thunks to avoid
> speculative execution via indirect call, jmp and ret.
> >> > >
> >> > > H.J. Lu (5):
> >> > >   x86: Add -mindirect-branch=
> >> > >   x86: Add -mfunction-return=
> >> > >   x86: Add -mindirect-branch-register
> >> > >   x86: Add 'V' register operand modifier
> >> > >   x86: Disallow -mindirect-branch=/-mfunction-return= with
> >> > > -mcmodel=large
> >> >
> >> > Current set of patches don't seem to have any option to generate
> "lfence" as the loop filler in "retpoline",  which is required by AMD.
> >> > Can you please clarify the plan. We would like to get this checked-in GCC
> 8.
> >>
> >> Since thunks are output as strings, it is easy to add the option on
> >> the top of patch #1 of the series.  I do not fully understand the
> >> reason for choosing pause over lfence for Intel, but if we need to do
> >> both, we need to have command line option (and possibly attribute).
> >> What would be reasonable name for it?
> >
> > I forgot there is -mindirect-branch-loop for that in the original patchset.
> > So for now we should be happy with having both lfence and pause in
> > there or do we still need it?
> >
> 
> I suggest we leave it out for the time being.

Yes as of now having both "lfence" and "pause" is Ok.   Hope we can add "- 
indirect-branch-loop" option later if required.  

Regards,
Venkat,
> 
> 
> --
> H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Linus Torvalds]

On Sun, Jan 14, 2018 at 3:09 PM, David Woodhouse  wrote:
>
> I think we should stick with what we have now, with the names of the
> thunks actually being the *full* name of the register (rax, eax, etc.)
> that they use.

It that works for the gcc people, then yes, I agree. The mixed
"sometimes full, sometimes not" approach just seems broken.

  Linus

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 15:02 -0800, Linus Torvalds wrote:
> On Sun, Jan 14, 2018 at 2:39 PM, David Woodhouse  > wrote:
> >
> > I'm not convinced we want to do this, but I'll defer to Linus.
> 
> Well, I guess we have no choice, if gcc ends up using the stupid
> names.

At this point, they'll do what we ask. See Uros's earlier message:

> ... the difference is only a couple of lines in the
> compiler source which we also can live with. But please discuss my
> proposal also in the kernel community, and weight the benefits and
> drawbacks of each approach before the final decision.
> 
> Please pass the final decision to gcc community, and we'll implement it.

I think we should stick with what we have now, with the names of the
thunks actually being the *full* name of the register (rax, eax, etc.)
that they use.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Linus Torvalds]

On Sun, Jan 14, 2018 at 2:39 PM, David Woodhouse  wrote:
>
> I'm not convinced we want to do this, but I'll defer to Linus.

Well, I guess we have no choice, if gcc ends up using the stupid names.

And yes, apparently this just made our macros worse instead of
cleaning anything up. Oh well.

I do have one (possible) solution: just export both names. So you'd export

  __x86_indirect_thunk_ax
  __x86_indirect_thunk_rax
..
 __x86_indirect_thunk_8
 __x86_indirect_thunk_r8

as symbols (same code, obviously), and then

 (a) the macros would be simpler

 (b) it just happens to work with even the old gcc patch

But at this point I don't really care.

  Linus

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 14:12 -0800, H.J. Lu wrote:
> Please use this GCC patch instead.

Building now; thanks.

This is the kernel patch I'll test as soon as the compiler is done.
It's slightly less horrid than the "clever" one I sent out earlier, but
does still end up needing those _ASM_AX etc. macros in *addition* to
the bare "ax" that goes in the symbol names.

I'm not convinced we want to do this, but I'll defer to Linus.



From 755f50731a99b0ce0890e478e6a2d6ebd647da15 Mon Sep 17 00:00:00 2001
From: David Woodhouse 
Date: Sun, 14 Jan 2018 22:21:02 +
Subject: [PATCH] x86/retpoline: Switch thunk names to match final GCC patches

At the last minute, they were switched from __x86_indirect_thunk_rax to
__x86_indirect_thunk_ax without the 'r' or 'e' on the register name.

This is not entirely an improvement, IMO.

Signed-off-by: David Woodhouse 
---
 arch/x86/include/asm/asm-prototypes.h | 24 ++--
 arch/x86/lib/retpoline.S  | 41 +--
 2 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/arch/x86/include/asm/asm-prototypes.h 
b/arch/x86/include/asm/asm-prototypes.h
index 0927cdc4f946..df80478fb682 100644
--- a/arch/x86/include/asm/asm-prototypes.h
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -18,19 +18,7 @@ extern void cmpxchg8b_emu(void);
 #endif
 
 #ifdef CONFIG_RETPOLINE
-#ifdef CONFIG_X86_32
-#define INDIRECT_THUNK(reg) extern asmlinkage void __x86_indirect_thunk_e ## 
reg(void);
-#else
-#define INDIRECT_THUNK(reg) extern asmlinkage void __x86_indirect_thunk_r ## 
reg(void);
-INDIRECT_THUNK(8)
-INDIRECT_THUNK(9)
-INDIRECT_THUNK(10)
-INDIRECT_THUNK(11)
-INDIRECT_THUNK(12)
-INDIRECT_THUNK(13)
-INDIRECT_THUNK(14)
-INDIRECT_THUNK(15)
-#endif
+#define INDIRECT_THUNK(reg) extern asmlinkage void __x86_indirect_thunk_ ## 
reg(void);
 INDIRECT_THUNK(ax)
 INDIRECT_THUNK(bx)
 INDIRECT_THUNK(cx)
@@ -39,4 +27,14 @@ INDIRECT_THUNK(si)
 INDIRECT_THUNK(di)
 INDIRECT_THUNK(bp)
 INDIRECT_THUNK(sp)
+#ifdef CONFIG_64BIT
+INDIRECT_THUNK(r8)
+INDIRECT_THUNK(r9)
+INDIRECT_THUNK(r10)
+INDIRECT_THUNK(r11)
+INDIRECT_THUNK(r12)
+INDIRECT_THUNK(r13)
+INDIRECT_THUNK(r14)
+INDIRECT_THUNK(r15)
+#endif /* CONFIG_64BIT */
 #endif /* CONFIG_RETPOLINE */
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index cb45c6cb465f..7da2c9035836 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -8,14 +8,14 @@
 #include 
 #include 
 
-.macro THUNK reg
-   .section .text.__x86.indirect_thunk.\reg
+.macro THUNK reg suffix
+   .section .text.__x86.indirect_thunk.\suffix
 
-ENTRY(__x86_indirect_thunk_\reg)
+ENTRY(__x86_indirect_thunk_\suffix)
    CFI_STARTPROC
    JMP_NOSPEC %\reg
    CFI_ENDPROC
-ENDPROC(__x86_indirect_thunk_\reg)
+ENDPROC(__x86_indirect_thunk_\suffix)
 .endm
 
 /*
@@ -26,23 +26,22 @@ ENDPROC(__x86_indirect_thunk_\reg)
  * the simple and nasty way...
  */
 #define EXPORT_THUNK(reg) EXPORT_SYMBOL(__x86_indirect_thunk_ ## reg)
-#define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)
+#define GENERATE_THUNK(reg, suffix) THUNK reg suffix; EXPORT_THUNK(suffix)
 
-GENERATE_THUNK(_ASM_AX)
-GENERATE_THUNK(_ASM_BX)
-GENERATE_THUNK(_ASM_CX)
-GENERATE_THUNK(_ASM_DX)
-GENERATE_THUNK(_ASM_SI)
-GENERATE_THUNK(_ASM_DI)
-GENERATE_THUNK(_ASM_BP)
-GENERATE_THUNK(_ASM_SP)
+GENERATE_THUNK(_ASM_AX, ax)
+GENERATE_THUNK(_ASM_BX, bx)
+GENERATE_THUNK(_ASM_CX, cx)
+GENERATE_THUNK(_ASM_DX, dx)
+GENERATE_THUNK(_ASM_SI, si)
+GENERATE_THUNK(_ASM_DI, di)
+GENERATE_THUNK(_ASM_BP, bp)
 #ifdef CONFIG_64BIT
-GENERATE_THUNK(r8)
-GENERATE_THUNK(r9)
-GENERATE_THUNK(r10)
-GENERATE_THUNK(r11)
-GENERATE_THUNK(r12)
-GENERATE_THUNK(r13)
-GENERATE_THUNK(r14)
-GENERATE_THUNK(r15)
+GENERATE_THUNK(r8, r8)
+GENERATE_THUNK(r9, r9)
+GENERATE_THUNK(r10, r10)
+GENERATE_THUNK(r11, r11)
+GENERATE_THUNK(r12, r12)
+GENERATE_THUNK(r13, r13)
+GENERATE_THUNK(r14, r14)
+GENERATE_THUNK(r15, r15)
 #endif
-- 
2.14.3


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 2:09 PM, David Woodhouse  wrote:
> On Sun, 2018-01-14 at 14:03 -0800, H.J. Lu wrote:
>>
>> > And *that* was the point at which I asked HJ to just use the proper
>> > bloody register names :)
>>
>> Please let me know if I should make the change to ax,..., r8,..r15.
>
> This is what I'm building my compiler with now, to make that change:
> http://git.infradead.org/users/dwmw2/gcc-retpoline.git/shortlog/refs/heads/retpoline-regnames
>
> At this point I'm inclined to suggest we don't make the change. I'll
> finish and test it anyway. I *can* change my GENERATE_THUNK macro to
> take two arguments — the suffix of the thunk name, and the register
> name to use. That lets me ditch the clever but ugly loop trick.
>
> I *did* want to get this file back to using .irp in the end, by fixing
> up other kernel infrastructure to do things properly. But I can live
> without that too if I must.

Please use this GCC patch instead.

-- 
H.J.
From 223c07edf531eaa05c7fff564ce6b5dc48e6a49b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Sun, 14 Jan 2018 14:04:55 -0800
Subject: [PATCH] x86: Change register names in __x86_indirect_thunk_reg

Change names of the lower 8 integer registers in __x86_indirect_thunk_reg
to ax, dx, cx, bx, si, di and bp.

gcc/

	* config/i386/i386.c (indirect_thunk_name): Don't check
	LEGACY_INT_REGNO_P.
	(print_reg): Use reg_names[regno] for 'V'.
	* doc/extend.texi: Replace "the full integer" with "the integer"
	for 'V'.

gcc/testsuite/

	* gcc.target/i386/indirect-thunk-1.c: Updated.
	* gcc.target/i386/indirect-thunk-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-5.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-6.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-4.c: Likewise.
	* gcc.target/i386/ret-thunk-10.c: Likewise.
	* gcc.target/i386/ret-thunk-11.c: Likewise.
	* gcc.target/i386/ret-thunk-12.c: Likewise.
	* gcc.target/i386/ret-thunk-13.c: Likewise.
	* gcc.target/i386/ret-thunk-14.c: Likewise.
	* gcc.target/i386/ret-thunk-15.c: Likewise.
	* gcc.target/i386/ret-thunk-9.c: Likewise.
---
 gcc/config/i386/i386.c   | 20 
 gcc/doc/extend.texi  |  2 +-
 gcc/testsuite/gcc.target/i386/indirect-thunk-1.c |  2 +-
 gcc/testsuite/gcc.target/i386/indirect-thunk-2.c |  2 +-
 gcc/testsuite/gcc.target/i386/indirect-thunk-3.c |  2 +-
 gcc/testsuite/gcc.target/i386/indirect-thunk-4.c |  2 +-
 gcc/testsuite/gcc.target/i386/indirect-thunk-7.c |  2 +-
 .../gcc.target/i386/indirect-thunk-attr-1.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-attr-2.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-attr-5.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-attr-6.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-attr-7.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-extern-1.c|  2 +-
 .../gcc.target/i386/indirect-thunk-extern-2.c|  2 +-
 .../gcc.target/i386/indirect-thunk-extern-3.c|  2 +-
 .../gcc.target/i386/indirect-thunk-extern-4.c|  2 +-
 .../gcc.target/i386/indirect-thunk-extern-7.c|  2 +-
 .../gcc.target/i386/indirect-thunk-register-1.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-register-3.c  |  2 +-
 .../gcc.target/i386/indirect-thunk-register-4.c  |  3 +--
 gcc/testsuite/gcc.target/i386/ret-thunk-10.c |  4 ++--
 gcc/testsuite/gcc.target/i386/ret-thunk-11.c |  4 ++--
 gcc/testsuite/gcc.target/i386/ret-thunk-12.c |  4 ++--
 gcc/testsuite/gcc.target/i386/ret-thunk-13.c |  2 +-
 gcc/testsuite/gcc.target/i386/ret-thunk-14.c |  2 +-
 gcc/testsuite/gcc.target/i386/ret-thunk-15.c |  2 +-
 gcc/testsuite/gcc.target/i386/ret-thunk-9.c  |  2 +-
 27 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5e4f845a1bd..890bd701cd1 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10787,15 +10787,8 @@ indirect_thunk_name (char name[32], int regno, bool need_bnd_p,
 {
   const char *bnd = need_bnd_p ? "_bnd" : "";
   if (regno >= 0)
-	{
-	  const char *reg_prefix;
-	  if (LEGACY_INT_REGNO_P (regno))
-	reg_prefix = TARGET_64BIT ? 

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 14:03 -0800, H.J. Lu wrote:
> 
> > And *that* was the point at which I asked HJ to just use the proper
> > bloody register names :)
> 
> Please let me know if I should make the change to ax,..., r8,..r15.

This is what I'm building my compiler with now, to make that change:
http://git.infradead.org/users/dwmw2/gcc-retpoline.git/shortlog/refs/heads/retpoline-regnames

At this point I'm inclined to suggest we don't make the change. I'll
finish and test it anyway. I *can* change my GENERATE_THUNK macro to
take two arguments — the suffix of the thunk name, and the register
name to use. That lets me ditch the clever but ugly loop trick.

I *did* want to get this file back to using .irp in the end, by fixing
up other kernel infrastructure to do things properly. But I can live
without that too if I must.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 1:58 PM, David Woodhouse  wrote:
> On Sun, 2018-01-14 at 13:07 -0800, Linus Torvalds wrote:
>> On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  wrote:
>> >
>> > Review on the GCC patches has led to a request that the thunk symbols
>> > be changed from e.g. __x86_indirect_thunk_rax to
>> > __x86_indirect_thunk_ax without the 'r'.
>> Ok. I think that just makes it easier for us, since then the names are
>> independent of 32-vs/64, and we don't need to use the _ASM_XY names.
>>
>> What about r8-r15? I'm assuming 'r' there is used?
>
> Ah yes, *this* is why I hated it... for 'r8' onwards that is indeed the
> register names as well as the suffix of the thunk name. But for the
> legacy registers I have to prepend 'e' or 'r' myself in the macro. So
> it ends up looking like this:
>
>
> .macro THUNK reg
> .section .text.__x86.indirect_thunk.\reg
>
> ENTRY(__x86_indirect_thunk_\reg)
> CFI_STARTPROC
> $done = 0
> .irp xreg r8 r9 r10 r11 r12 r13 r14 r15
> .ifeqs "\reg", "\xreg"
> JMP_NOSPEC %\reg
> $done = 1
> .endif
> .endr
> .if $done != 1
> JMP_NOSPEC %__ASM_REG(\reg)
> .endif
> CFI_ENDPROC
> ENDPROC(__x86_indirect_thunk_\reg)
> .endm
>
> /*
>  * Despite being an assembler file we can't just use .irp here
>  * because __KSYM_DEPS__ only uses the C preprocessor and would
>  * only see one instance of "__x86_indirect_thunk_\reg" rather
>  * than one per register with the correct names. So we do it
>  * the simple and nasty way...
>  */
> #define EXPORT_THUNK(reg) EXPORT_SYMBOL(__x86_indirect_thunk_ ## reg)
> #define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)
>
> GENERATE_THUNK(ax)
> GENERATE_THUNK(bx)
> GENERATE_THUNK(cx)
> GENERATE_THUNK(dx)
> GENERATE_THUNK(si)
> GENERATE_THUNK(di)
> GENERATE_THUNK(bp)
> #ifdef CONFIG_64BIT
> GENERATE_THUNK(r8)
> GENERATE_THUNK(r9)
> GENERATE_THUNK(r10)
> GENERATE_THUNK(r11)
> GENERATE_THUNK(r12)
> GENERATE_THUNK(r13)
> GENERATE_THUNK(r14)
> GENERATE_THUNK(r15)
> #endif
>
>
> And *that* was the point at which I asked HJ to just use the proper
> bloody register names :)

Please let me know if I should make the change to ax,..., r8,..r15.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 13:07 -0800, Linus Torvalds wrote:
> On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  wrote:
> > 
> > Review on the GCC patches has led to a request that the thunk symbols
> > be changed from e.g. __x86_indirect_thunk_rax to
> > __x86_indirect_thunk_ax without the 'r'.
> Ok. I think that just makes it easier for us, since then the names are
> independent of 32-vs/64, and we don't need to use the _ASM_XY names.
> 
> What about r8-r15? I'm assuming 'r' there is used?

Ah yes, *this* is why I hated it... for 'r8' onwards that is indeed the
register names as well as the suffix of the thunk name. But for the
legacy registers I have to prepend 'e' or 'r' myself in the macro. So
it ends up looking like this:


.macro THUNK reg
.section .text.__x86.indirect_thunk.\reg

ENTRY(__x86_indirect_thunk_\reg)
CFI_STARTPROC
$done = 0
.irp xreg r8 r9 r10 r11 r12 r13 r14 r15
.ifeqs "\reg", "\xreg"
JMP_NOSPEC %\reg
$done = 1
.endif
.endr
.if $done != 1
JMP_NOSPEC %__ASM_REG(\reg)
.endif
CFI_ENDPROC
ENDPROC(__x86_indirect_thunk_\reg)
.endm

/*
 * Despite being an assembler file we can't just use .irp here
 * because __KSYM_DEPS__ only uses the C preprocessor and would
 * only see one instance of "__x86_indirect_thunk_\reg" rather
 * than one per register with the correct names. So we do it
 * the simple and nasty way...
 */
#define EXPORT_THUNK(reg) EXPORT_SYMBOL(__x86_indirect_thunk_ ## reg)
#define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)

GENERATE_THUNK(ax)
GENERATE_THUNK(bx)
GENERATE_THUNK(cx)
GENERATE_THUNK(dx)
GENERATE_THUNK(si)
GENERATE_THUNK(di)
GENERATE_THUNK(bp)
#ifdef CONFIG_64BIT
GENERATE_THUNK(r8)
GENERATE_THUNK(r9)
GENERATE_THUNK(r10)
GENERATE_THUNK(r11)
GENERATE_THUNK(r12)
GENERATE_THUNK(r13)
GENERATE_THUNK(r14)
GENERATE_THUNK(r15)
#endif


And *that* was the point at which I asked HJ to just use the proper
bloody register names :)


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 1:07 PM, Linus Torvalds
 wrote:
> On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  wrote:
>> Review on the GCC patches has led to a request that the thunk symbols
>> be changed from e.g. __x86_indirect_thunk_rax to
>> __x86_indirect_thunk_ax without the 'r'.
>
> Ok. I think that just makes it easier for us, since then the names are
> independent of 32-vs/64, and we don't need to use the _ASM_XY names.
>
> What about r8-r15? I'm assuming 'r' there is used?

They will remain r8-r15.

> Mind sending me a tested patch? I'll was indeed planning on generating
> rc8, but I might as well go grocery shopping now instead, and do rc8
> later in the evening.
>
> Linus



-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Thomas Gleixner]

On Sun, 14 Jan 2018, David Woodhouse wrote:

> On Sun, 2018-01-14 at 13:07 -0800, Linus Torvalds wrote:
> > On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  
> > wrote:
> > > Review on the GCC patches has led to a request that the thunk symbols
> > > be changed from e.g. __x86_indirect_thunk_rax to
> > > __x86_indirect_thunk_ax without the 'r'.
> > 
> > Ok. I think that just makes it easier for us, since then the names are
> > independent of 32-vs/64, and we don't need to use the _ASM_XY names.
> > 
> > What about r8-r15? I'm assuming 'r' there is used?
> > 
> > Mind sending me a tested patch? I'll was indeed planning on generating
> > rc8, but I might as well go grocery shopping now instead, and do rc8
> > later in the evening.
> 
> I'll kick off a compiler build now...

Send the patch to me/LKML. I'm queueing the compile time warning removal
and then can add that one on top so Linus can pull the lot.

Thanks,

tglx

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 13:07 -0800, Linus Torvalds wrote:
> On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  wrote:
> > Review on the GCC patches has led to a request that the thunk symbols
> > be changed from e.g. __x86_indirect_thunk_rax to
> > __x86_indirect_thunk_ax without the 'r'.
> 
> Ok. I think that just makes it easier for us, since then the names are
> independent of 32-vs/64, and we don't need to use the _ASM_XY names.
> 
> What about r8-r15? I'm assuming 'r' there is used?
> 
> Mind sending me a tested patch? I'll was indeed planning on generating
> rc8, but I might as well go grocery shopping now instead, and do rc8
> later in the evening.

I'll kick off a compiler build now...

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Linus Torvalds]

On Sun, Jan 14, 2018 at 1:02 PM, David Woodhouse  wrote:
> Review on the GCC patches has led to a request that the thunk symbols
> be changed from e.g. __x86_indirect_thunk_rax to
> __x86_indirect_thunk_ax without the 'r'.

Ok. I think that just makes it easier for us, since then the names are
independent of 32-vs/64, and we don't need to use the _ASM_XY names.

What about r8-r15? I'm assuming 'r' there is used?

Mind sending me a tested patch? I'll was indeed planning on generating
rc8, but I might as well go grocery shopping now instead, and do rc8
later in the evening.

Linus

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 21:52 +0100, Uros Bizjak wrote:
> On Sun, Jan 14, 2018 at 9:34 PM, David Woodhouse  wrote:
> > But sure, right now it isn't that might of a difference for me; my
> > implementation has changed since I made that reqeust. I have no
> > fundamental technical objection to the bare 'ax' naming. We can live
> > with either.
> > 
> > It's just that we've been asking for an agreement on the basics (the
> > command line we use, and the thunk names) for some days now, and this
> > is the first time we've had this discussion, and Linus has just taken
> > the patches.
> > 
> > That's still fine. I know we get no sympathy, and we *can* change the
> > Linux kernel between -rc8 and -final if we must, and change the Xen
> > patches too. I'd just rather not.
> Well, you did say that these are strange times ;)
> 
> From the user perspective, it would be more convenient to use the
> thunk names that are the same for 32bit and 64bit targets. If we
> ignore this fact, the difference is only a couple of lines in the
> compiler source which we also can live with. But please discuss my
> proposal also in the kernel community, and weight the benefits and
> drawbacks of each approach before the final decision.
> 
> Please pass the final decision to gcc community, and we'll implement it.

+Linus, Thomas.

Review on the GCC patches has led to a request that the thunk symbols
be changed from e.g. __x86_indirect_thunk_rax to
__x86_indirect_thunk_ax without the 'r'.

If we're going to change the thunk names, it's best to do it *right*
now before the 4.15-rc8 release.

I genuinely don't care at this point what the thunk names are. It's
just that Linus is probably preparing the -rc8 release as we speak, and
I'd want to do a new compiler build and set of tests if we make the
change. For that reason alone, I'm inclined to answer that we should
leave them as they are.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 9:34 PM, David Woodhouse  wrote:

> Likewise, the CONFIG_TRIM_UNUSED_SYMBOLS mechanism in the kernel passes
> .S files through the preprocessor and looks for EXPORT_SYMBOL, so it
> wasn't working well with my .irp-based implementation like the one in
> Xen. So I've swapped it out for this one for now.
>
> Again, I was hoping to clean that up and make it do something saner,
> and then this could switch back too.
>
> But sure, right now it isn't that might of a difference for me; my
> implementation has changed since I made that reqeust. I have no
> fundamental technical objection to the bare 'ax' naming. We can live
> with either.
>
> It's just that we've been asking for an agreement on the basics (the
> command line we use, and the thunk names) for some days now, and this
> is the first time we've had this discussion, and Linus has just taken
> the patches.
>
> That's still fine. I know we get no sympathy, and we *can* change the
> Linux kernel between -rc8 and -final if we must, and change the Xen
> patches too. I'd just rather not.

Well, you did say that these are strange times ;)

>From the user perspective, it would be more convenient to use the
thunk names that are the same for 32bit and 64bit targets. If we
ignore this fact, the difference is only a couple of lines in the
compiler source which we also can live with. But please discuss my
proposal also in the kernel community, and weight the benefits and
drawbacks of each approach before the final decision.

Please pass the final decision to gcc community, and we'll implement it.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-14 at 21:21 +0100, Uros Bizjak wrote:
> A quick look through latest x86/pti update [1] shows:
> 
> +#ifdef CONFIG_X86_32
> +#define INDIRECT_THUNK(reg) extern asmlinkage void
> __x86_indirect_thunk_e ## reg(void);
> +#else
> +#define INDIRECT_THUNK(reg) extern asmlinkage void
> __x86_indirect_thunk_r ## reg(void);
> +INDIRECT_THUNK(8)
> +INDIRECT_THUNK(9)
> +INDIRECT_THUNK(10)
> +INDIRECT_THUNK(11)
> +INDIRECT_THUNK(12)
> +INDIRECT_THUNK(13)
> +INDIRECT_THUNK(14)
> +INDIRECT_THUNK(15)
> +#endif
> +INDIRECT_THUNK(ax)
> +INDIRECT_THUNK(bx)
> +INDIRECT_THUNK(cx)
> +INDIRECT_THUNK(dx)
> +INDIRECT_THUNK(si)
> +INDIRECT_THUNK(di)
> +INDIRECT_THUNK(bp)
> +INDIRECT_THUNK(sp)

Yeah, that one is purely for the CONFIG_MODVERSIONS system, which I'm
hoping to fix properly by not having to have fake (and clearly
incorrect) C prototypes for the thunks which aren't actually C
functions. It's intended to go away.


> and:
> 
> +/*
> + * Despite being an assembler file we can't just use .irp here
> + * because __KSYM_DEPS__ only uses the C preprocessor and would
> + * only see one instance of "__x86_indirect_thunk_\reg" rather
> + * than one per register with the correct names. So we do it
> + * the simple and nasty way...
> + */
> +#define EXPORT_THUNK(reg) EXPORT_SYMBOL(__x86_indirect_thunk_ ## reg)
> +#define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)
> +
> +GENERATE_THUNK(_ASM_AX)
> +GENERATE_THUNK(_ASM_BX)
> +GENERATE_THUNK(_ASM_CX)
> +GENERATE_THUNK(_ASM_DX)
> +GENERATE_THUNK(_ASM_SI)
> +GENERATE_THUNK(_ASM_DI)
> +GENERATE_THUNK(_ASM_BP)
> +GENERATE_THUNK(_ASM_SP)
> +#ifdef CONFIG_64BIT
> +GENERATE_THUNK(r8)
> +GENERATE_THUNK(r9)
> +GENERATE_THUNK(r10)
> +GENERATE_THUNK(r11)
> +GENERATE_THUNK(r12)
> +GENERATE_THUNK(r13)
> +GENERATE_THUNK(r14)
> +GENERATE_THUNK(r15)
> 
> I have a feeling that using e.g. __x86_indirect_thunk_ax would be more
> convenient in both cases.

Likewise, the CONFIG_TRIM_UNUSED_SYMBOLS mechanism in the kernel passes
.S files through the preprocessor and looks for EXPORT_SYMBOL, so it
wasn't working well with my .irp-based implementation like the one in
Xen. So I've swapped it out for this one for now.

Again, I was hoping to clean that up and make it do something saner,
and then this could switch back too.

But sure, right now it isn't that might of a difference for me; my
implementation has changed since I made that reqeust. I have no
fundamental technical objection to the bare 'ax' naming. We can live
with either.

It's just that we've been asking for an agreement on the basics (the
command line we use, and the thunk names) for some days now, and this
is the first time we've had this discussion, and Linus has just taken
the patches. 

That's still fine. I know we get no sympathy, and we *can* change the
Linux kernel between -rc8 and -final if we must, and change the Xen
patches too. I'd just rather not.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 7:22 PM, Uros Bizjak  wrote:
> On Sun, Jan 14, 2018 at 6:44 PM, Woodhouse, David  wrote:
>> This won't make the list; I'll send a more coherent and less HTML-afflicted
>> version later.
>>
>> The bare 'ax' naming made it painful to instantiate the external thunks for
>> 32-bit and 64-bot code because we had to put the e/r back again inside the
>> .irp reg ax bx... code.
>>
>> We could probably have lived with that but it would be painful to change now
>> that Linux and Xen patches with the current ABI are all lined up. I
>> appreciate they weren't in GCC yet so we get little sympathy but these are
>> strange times and we had to move fast.
>>
>> I'd really like *not* to change it now. Having the thunk name actually
>> include the name of the register it's using does seem nicer anyway...
>
> That's unfortunate... I suspect that in the future, one will need
> #ifdef __x86_64__ around eventual calls to thunks from c code because
> of this decision, since thunks for x86_64 target will have different
> names than thunks for x86_32 target. I don't know if the (single?)
> case of mixing 32 and 64 bit assembly in the highly specialized part
> of the kernel really warrants this decision. Future programmers will
> be grateful if kernel people can re-consider their choice in
> not-yet-release ABI.

A quick look through latest x86/pti update [1] shows:

+#ifdef CONFIG_X86_32
+#define INDIRECT_THUNK(reg) extern asmlinkage void
__x86_indirect_thunk_e ## reg(void);
+#else
+#define INDIRECT_THUNK(reg) extern asmlinkage void
__x86_indirect_thunk_r ## reg(void);
+INDIRECT_THUNK(8)
+INDIRECT_THUNK(9)
+INDIRECT_THUNK(10)
+INDIRECT_THUNK(11)
+INDIRECT_THUNK(12)
+INDIRECT_THUNK(13)
+INDIRECT_THUNK(14)
+INDIRECT_THUNK(15)
+#endif
+INDIRECT_THUNK(ax)
+INDIRECT_THUNK(bx)
+INDIRECT_THUNK(cx)
+INDIRECT_THUNK(dx)
+INDIRECT_THUNK(si)
+INDIRECT_THUNK(di)
+INDIRECT_THUNK(bp)
+INDIRECT_THUNK(sp)

and:

+/*
+ * Despite being an assembler file we can't just use .irp here
+ * because __KSYM_DEPS__ only uses the C preprocessor and would
+ * only see one instance of "__x86_indirect_thunk_\reg" rather
+ * than one per register with the correct names. So we do it
+ * the simple and nasty way...
+ */
+#define EXPORT_THUNK(reg) EXPORT_SYMBOL(__x86_indirect_thunk_ ## reg)
+#define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)
+
+GENERATE_THUNK(_ASM_AX)
+GENERATE_THUNK(_ASM_BX)
+GENERATE_THUNK(_ASM_CX)
+GENERATE_THUNK(_ASM_DX)
+GENERATE_THUNK(_ASM_SI)
+GENERATE_THUNK(_ASM_DI)
+GENERATE_THUNK(_ASM_BP)
+GENERATE_THUNK(_ASM_SP)
+#ifdef CONFIG_64BIT
+GENERATE_THUNK(r8)
+GENERATE_THUNK(r9)
+GENERATE_THUNK(r10)
+GENERATE_THUNK(r11)
+GENERATE_THUNK(r12)
+GENERATE_THUNK(r13)
+GENERATE_THUNK(r14)
+GENERATE_THUNK(r15)

I have a feeling that using e.g. __x86_indirect_thunk_ax would be more
convenient in both cases.

[1] https://www.spinics.net/lists/kernel/msg2697606.html

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 6:44 PM, Woodhouse, David  wrote:
> This won't make the list; I'll send a more coherent and less HTML-afflicted
> version later.
>
> The bare 'ax' naming made it painful to instantiate the external thunks for
> 32-bit and 64-bot code because we had to put the e/r back again inside the
> .irp reg ax bx... code.
>
> We could probably have lived with that but it would be painful to change now
> that Linux and Xen patches with the current ABI are all lined up. I
> appreciate they weren't in GCC yet so we get little sympathy but these are
> strange times and we had to move fast.
>
> I'd really like *not* to change it now. Having the thunk name actually
> include the name of the register it's using does seem nicer anyway...

That's unfortunate... I suspect that in the future, one will need
#ifdef __x86_64__ around eventual calls to thunks from c code because
of this decision, since thunks for x86_64 target will have different
names than thunks for x86_32 target. I don't know if the (single?)
case of mixing 32 and 64 bit assembly in the highly specialized part
of the kernel really warrants this decision. Future programmers will
be grateful if kernel people can re-consider their choice in
not-yet-release ABI.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Woodhouse, David]

This won't make the list; I'll send a more coherent and less HTML-afflicted 
version later.

The bare 'ax' naming made it painful to instantiate the external thunks for 
32-bit and 64-bot code because we had to put the e/r back again inside the .irp 
reg ax bx... code.

We could probably have lived with that but it would be painful to change now 
that Linux and Xen patches with the current ABI are all lined up. I appreciate 
they weren't in GCC yet so we get little sympathy but these are strange times 
and we had to move fast.

I'd really like *not* to change it now. Having the thunk name actually include 
the name of the register it's using does seem nicer anyway...

On 14 Jan 2018 17:58, "H.J. Lu"  wrote:

On Sun, Jan 14, 2018 at 8:45 AM, Jakub Jelinek  wrote:
> On Sun, Jan 14, 2018 at 08:41:54AM -0800, H.J. Lu wrote:
>> They are used in asm statements in kernel:
>>
>> extern void (*func_p) (void);
>>
>> void
>> foo (void)
>> {
>>   asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));
>
> Well, using it just with a single register classes wouldn't make much sense,
> then you can just use "call __x86_indirect_thunk_rax"
> or "call __x86_indirect_thunk_eax" depending on __x86_64__, you wouldn't
> need to extend anything.
> But supposedly if you use it with "r" or "q" or similar class this will be
> different.
>

I believe "r" is allowed.


--
H.J.




Amazon Web Services UK Limited. Registered in England and Wales with 
registration number 08650665 and which has its registered office at 60 Holborn 
Viaduct, London EC1A 2FD, United Kingdom.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 8:45 AM, Jakub Jelinek  wrote:
> On Sun, Jan 14, 2018 at 08:41:54AM -0800, H.J. Lu wrote:
>> They are used in asm statements in kernel:
>>
>> extern void (*func_p) (void);
>>
>> void
>> foo (void)
>> {
>>   asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));
>
> Well, using it just with a single register classes wouldn't make much sense,
> then you can just use "call __x86_indirect_thunk_rax"
> or "call __x86_indirect_thunk_eax" depending on __x86_64__, you wouldn't
> need to extend anything.
> But supposedly if you use it with "r" or "q" or similar class this will be
> different.
>

I believe "r" is allowed.


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 8:48 AM, Uros Bizjak  wrote:
> On Sun, Jan 14, 2018 at 5:41 PM, H.J. Lu  wrote:
>> On Sun, Jan 14, 2018 at 8:39 AM, Uros Bizjak  wrote:
>>> On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
 On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
>> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>>
>>> Hi Uros,
>>>
>>> Can you take a look at my x86 backend changes so that they are ready
>>> to check in once we have consensus.
>>
>> Please finish the talks about the correct approach first. Once the
>> consensus is reached, please post the final version of the patches for
>> review.
>>
>> BTW: I have no detailed insight in these issues, so I'll look mostly
>> at the implementation details, probably early next week.
>
> One general remark is on the usage of -1 as an invalid register

 This has been rewritten.  The checked in patch no longer does that.
>>>
>>> Another issue:
>>>
>>> +static void
>>> +indirect_thunk_name (char name[32], int regno, bool need_bnd_p)
>>> +{
>>> +  if (USE_HIDDEN_LINKONCE)
>>> +{
>>> +  const char *bnd = need_bnd_p ? "_bnd" : "";
>>> +  if (regno >= 0)
>>> +{
>>> +  const char *reg_prefix;
>>> +  if (LEGACY_INT_REGNO_P (regno))
>>> +reg_prefix = TARGET_64BIT ? "r" : "e";
>>> +  else
>>> +reg_prefix = "";
>>> +  sprintf (name, "__x86_indirect_thunk%s_%s%s",
>>> +   bnd, reg_prefix, reg_names[regno]);
>>> +}
>>> +  else
>>> +sprintf (name, "__x86_indirect_thunk%s", bnd);
>>> +}
>>>
>>> What is the benefit of reg_prefix? Can't we just live with e.g.:
>>>
>>> __x86_indirect_thunk_ax
>>>
>>> which is the true register name and is valid for 32bit and 64bit targets.
>>
>> They are used in asm statements in kernel:
>>
>> extern void (*func_p) (void);
>>
>> void
>> foo (void)
>> {
>>   asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));
>> }
>>
>> it generates:
>>
>> foo:
>> movq func_p(%rip), %rax
>> call __x86_indirect_thunk_rax
>> ret
>
> Please fix %V to output reg_name instead.
>

David, please comment.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 5:41 PM, H.J. Lu  wrote:
> On Sun, Jan 14, 2018 at 8:39 AM, Uros Bizjak  wrote:
>> On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
>>> On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
 On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>
>> Hi Uros,
>>
>> Can you take a look at my x86 backend changes so that they are ready
>> to check in once we have consensus.
>
> Please finish the talks about the correct approach first. Once the
> consensus is reached, please post the final version of the patches for
> review.
>
> BTW: I have no detailed insight in these issues, so I'll look mostly
> at the implementation details, probably early next week.

 One general remark is on the usage of -1 as an invalid register
>>>
>>> This has been rewritten.  The checked in patch no longer does that.
>>
>> Another issue:
>>
>> +static void
>> +indirect_thunk_name (char name[32], int regno, bool need_bnd_p)
>> +{
>> +  if (USE_HIDDEN_LINKONCE)
>> +{
>> +  const char *bnd = need_bnd_p ? "_bnd" : "";
>> +  if (regno >= 0)
>> +{
>> +  const char *reg_prefix;
>> +  if (LEGACY_INT_REGNO_P (regno))
>> +reg_prefix = TARGET_64BIT ? "r" : "e";
>> +  else
>> +reg_prefix = "";
>> +  sprintf (name, "__x86_indirect_thunk%s_%s%s",
>> +   bnd, reg_prefix, reg_names[regno]);
>> +}
>> +  else
>> +sprintf (name, "__x86_indirect_thunk%s", bnd);
>> +}
>>
>> What is the benefit of reg_prefix? Can't we just live with e.g.:
>>
>> __x86_indirect_thunk_ax
>>
>> which is the true register name and is valid for 32bit and 64bit targets.
>
> They are used in asm statements in kernel:
>
> extern void (*func_p) (void);
>
> void
> foo (void)
> {
>   asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));
> }
>
> it generates:
>
> foo:
> movq func_p(%rip), %rax
> call __x86_indirect_thunk_rax
> ret

Please fix %V to output reg_name instead.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jakub Jelinek]

On Sun, Jan 14, 2018 at 08:41:54AM -0800, H.J. Lu wrote:
> They are used in asm statements in kernel:
> 
> extern void (*func_p) (void);
> 
> void
> foo (void)
> {
>   asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));

Well, using it just with a single register classes wouldn't make much sense,
then you can just use "call __x86_indirect_thunk_rax"
or "call __x86_indirect_thunk_eax" depending on __x86_64__, you wouldn't
need to extend anything.
But supposedly if you use it with "r" or "q" or similar class this will be
different.

Jakub

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
> On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
>> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
>>> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>>>
 Hi Uros,

 Can you take a look at my x86 backend changes so that they are ready
 to check in once we have consensus.
>>>
>>> Please finish the talks about the correct approach first. Once the
>>> consensus is reached, please post the final version of the patches for
>>> review.
>>>
>>> BTW: I have no detailed insight in these issues, so I'll look mostly
>>> at the implementation details, probably early next week.
>>
>> One general remark is on the usage of -1 as an invalid register
>
> This has been rewritten.  The checked in patch no longer does that.

I'm looking directly into current indirect_thunk_name,
output_indirect_thunk and output_indirect_thunk_function functions in
i386.c which have plenty of the mentioned checks.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 8:39 AM, Uros Bizjak  wrote:
> On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
>> On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
>>> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
 On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:

> Hi Uros,
>
> Can you take a look at my x86 backend changes so that they are ready
> to check in once we have consensus.

 Please finish the talks about the correct approach first. Once the
 consensus is reached, please post the final version of the patches for
 review.

 BTW: I have no detailed insight in these issues, so I'll look mostly
 at the implementation details, probably early next week.
>>>
>>> One general remark is on the usage of -1 as an invalid register
>>
>> This has been rewritten.  The checked in patch no longer does that.
>
> Another issue:
>
> +static void
> +indirect_thunk_name (char name[32], int regno, bool need_bnd_p)
> +{
> +  if (USE_HIDDEN_LINKONCE)
> +{
> +  const char *bnd = need_bnd_p ? "_bnd" : "";
> +  if (regno >= 0)
> +{
> +  const char *reg_prefix;
> +  if (LEGACY_INT_REGNO_P (regno))
> +reg_prefix = TARGET_64BIT ? "r" : "e";
> +  else
> +reg_prefix = "";
> +  sprintf (name, "__x86_indirect_thunk%s_%s%s",
> +   bnd, reg_prefix, reg_names[regno]);
> +}
> +  else
> +sprintf (name, "__x86_indirect_thunk%s", bnd);
> +}
>
> What is the benefit of reg_prefix? Can't we just live with e.g.:
>
> __x86_indirect_thunk_ax
>
> which is the true register name and is valid for 32bit and 64bit targets.

They are used in asm statements in kernel:

extern void (*func_p) (void);

void
foo (void)
{
  asm ("call __x86_indirect_thunk_%V0" : : "a" (func_p));
}

it generates:

foo:
movq func_p(%rip), %rax
call __x86_indirect_thunk_rax
ret


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 5:35 PM, H.J. Lu  wrote:
> On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
>> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
>>> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>>>
 Hi Uros,

 Can you take a look at my x86 backend changes so that they are ready
 to check in once we have consensus.
>>>
>>> Please finish the talks about the correct approach first. Once the
>>> consensus is reached, please post the final version of the patches for
>>> review.
>>>
>>> BTW: I have no detailed insight in these issues, so I'll look mostly
>>> at the implementation details, probably early next week.
>>
>> One general remark is on the usage of -1 as an invalid register
>
> This has been rewritten.  The checked in patch no longer does that.

Another issue:

+static void
+indirect_thunk_name (char name[32], int regno, bool need_bnd_p)
+{
+  if (USE_HIDDEN_LINKONCE)
+{
+  const char *bnd = need_bnd_p ? "_bnd" : "";
+  if (regno >= 0)
+{
+  const char *reg_prefix;
+  if (LEGACY_INT_REGNO_P (regno))
+reg_prefix = TARGET_64BIT ? "r" : "e";
+  else
+reg_prefix = "";
+  sprintf (name, "__x86_indirect_thunk%s_%s%s",
+   bnd, reg_prefix, reg_names[regno]);
+}
+  else
+sprintf (name, "__x86_indirect_thunk%s", bnd);
+}

What is the benefit of reg_prefix? Can't we just live with e.g.:

__x86_indirect_thunk_ax

which is the true register name and is valid for 32bit and 64bit targets.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 8:19 AM, Uros Bizjak  wrote:
> On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
>> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>>
>>> Hi Uros,
>>>
>>> Can you take a look at my x86 backend changes so that they are ready
>>> to check in once we have consensus.
>>
>> Please finish the talks about the correct approach first. Once the
>> consensus is reached, please post the final version of the patches for
>> review.
>>
>> BTW: I have no detailed insight in these issues, so I'll look mostly
>> at the implementation details, probably early next week.
>
> One general remark is on the usage of -1 as an invalid register

This has been rewritten.  The checked in patch no longer does that.

> number. We have INVALID_REGNUM definition for this, and many tests,
> like:
>
>   if (regno >= 0)
>
> could become much more informative:
>
>   if (regno != INVALID_REGNUM)



-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Fri, Jan 12, 2018 at 9:01 AM, Uros Bizjak  wrote:
> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>
>> Hi Uros,
>>
>> Can you take a look at my x86 backend changes so that they are ready
>> to check in once we have consensus.
>
> Please finish the talks about the correct approach first. Once the
> consensus is reached, please post the final version of the patches for
> review.
>
> BTW: I have no detailed insight in these issues, so I'll look mostly
> at the implementation details, probably early next week.

One general remark is on the usage of -1 as an invalid register
number. We have INVALID_REGNUM definition for this, and many tests,
like:

  if (regno >= 0)

could become much more informative:

  if (regno != INVALID_REGNUM)

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

Now my patch set has been checked into trunk.  Here is a patch set
to move struct ix86_frame to machine_function on GCC 7, which is
needed to backport the patch set to GCC 7:

https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01239.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01240.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01241.html

OK for gcc-7-branch?

Thanks.


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 6:20 AM, Jan Hubicka  wrote:
>> > Hi HJ,
>> >
>> > > -Original Message-
>> > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
>> > > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
>> > > Sent: Sunday, January 14, 2018 9:07 AM
>> > > To: gcc-patches@gcc.gnu.org
>> > > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
>> > >
>> > > This set of patches for GCC 8 mitigates variant #2 of the speculative
>> > > execution vulnerabilities on x86 processors identified by CVE-2017-5715, 
>> > > aka
>> > > Spectre.  They convert indirect branches and function returns to call and
>> > > return thunks to avoid speculative execution via indirect call, jmp and 
>> > > ret.
>> > >
>> > > H.J. Lu (5):
>> > >   x86: Add -mindirect-branch=
>> > >   x86: Add -mfunction-return=
>> > >   x86: Add -mindirect-branch-register
>> > >   x86: Add 'V' register operand modifier
>> > >   x86: Disallow -mindirect-branch=/-mfunction-return= with
>> > > -mcmodel=large
>> >
>> > Current set of patches don't seem to have any option to generate "lfence" 
>> > as the loop filler in "retpoline",  which is required by AMD.
>> > Can you please clarify the plan. We would like to get this checked-in GCC 
>> > 8.
>>
>> Since thunks are output as strings, it is easy to add the option
>> on the top of patch #1 of the series.  I do not fully understand
>> the reason for choosing pause over lfence for Intel, but if we need
>> to do both, we need to have command line option (and possibly attribute).
>> What would be reasonable name for it?
>
> I forgot there is -mindirect-branch-loop for that in the original patchset.
> So for now we should be happy with having both lfence and pause in there
> or do we still need it?
>

I suggest we leave it out for the time being.


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> > Hi HJ, 
> > 
> > > -Original Message-
> > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> > > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> > > Sent: Sunday, January 14, 2018 9:07 AM
> > > To: gcc-patches@gcc.gnu.org
> > > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> > > 
> > > This set of patches for GCC 8 mitigates variant #2 of the speculative
> > > execution vulnerabilities on x86 processors identified by CVE-2017-5715, 
> > > aka
> > > Spectre.  They convert indirect branches and function returns to call and
> > > return thunks to avoid speculative execution via indirect call, jmp and 
> > > ret.
> > > 
> > > H.J. Lu (5):
> > >   x86: Add -mindirect-branch=
> > >   x86: Add -mfunction-return=
> > >   x86: Add -mindirect-branch-register
> > >   x86: Add 'V' register operand modifier
> > >   x86: Disallow -mindirect-branch=/-mfunction-return= with
> > > -mcmodel=large
> > 
> > Current set of patches don't seem to have any option to generate "lfence" 
> > as the loop filler in "retpoline",  which is required by AMD.
> > Can you please clarify the plan. We would like to get this checked-in GCC 
> > 8.   
> 
> Since thunks are output as strings, it is easy to add the option
> on the top of patch #1 of the series.  I do not fully understand
> the reason for choosing pause over lfence for Intel, but if we need
> to do both, we need to have command line option (and possibly attribute).
> What would be reasonable name for it?

I forgot there is -mindirect-branch-loop for that in the original patchset.
So for now we should be happy with having both lfence and pause in there
or do we still need it?

Honza
> 
> Honza

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 14, 2018 at 4:42 AM, Jan Hubicka  wrote:
>> On Sun, Jan 14, 2018 at 11:40 AM, Jan Hubicka  wrote:
>> >> Hi HJ,
>> >>
>> >> > -Original Message-
>> >> > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
>> >> > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
>> >> > Sent: Sunday, January 14, 2018 9:07 AM
>> >> > To: gcc-patches@gcc.gnu.org
>> >> > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
>> >> >
>> >> > This set of patches for GCC 8 mitigates variant #2 of the speculative
>> >> > execution vulnerabilities on x86 processors identified by 
>> >> > CVE-2017-5715, aka
>> >> > Spectre.  They convert indirect branches and function returns to call 
>> >> > and
>> >> > return thunks to avoid speculative execution via indirect call, jmp and 
>> >> > ret.
>> >> >
>> >> > H.J. Lu (5):
>> >> >   x86: Add -mindirect-branch=
>> >> >   x86: Add -mfunction-return=
>> >> >   x86: Add -mindirect-branch-register
>> >> >   x86: Add 'V' register operand modifier
>> >> >   x86: Disallow -mindirect-branch=/-mfunction-return= with
>> >> > -mcmodel=large
>> >>
>> >> Current set of patches don't seem to have any option to generate "lfence" 
>> >> as the loop filler in "retpoline",  which is required by AMD.
>> >> Can you please clarify the plan. We would like to get this checked-in GCC 
>> >> 8.
>> >
>> > Since thunks are output as strings, it is easy to add the option
>> > on the top of patch #1 of the series.  I do not fully understand
>> > the reason for choosing pause over lfence for Intel, but if we need
>> > to do both, we need to have command line option (and possibly attribute).
>> > What would be reasonable name for it?
>>
>> Looking at the kernel patch [1], the loop filler should be
>> "pause;lfence" sequence, and should be universally accepted for Intel
>> and AMD targets.
>>
>> [1] https://www.spinics.net/lists/kernel/msg2697507.html
>
> Yep, I would say we should go with pause;lfence now and see if we want to add 
> argument
> eventually.
> HJ, does it sound OK?

Yes, I am checking a patch to default to "pause; lfence".


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> On Sun, Jan 14, 2018 at 11:40 AM, Jan Hubicka  wrote:
> >> Hi HJ,
> >>
> >> > -Original Message-
> >> > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> >> > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> >> > Sent: Sunday, January 14, 2018 9:07 AM
> >> > To: gcc-patches@gcc.gnu.org
> >> > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> >> >
> >> > This set of patches for GCC 8 mitigates variant #2 of the speculative
> >> > execution vulnerabilities on x86 processors identified by CVE-2017-5715, 
> >> > aka
> >> > Spectre.  They convert indirect branches and function returns to call and
> >> > return thunks to avoid speculative execution via indirect call, jmp and 
> >> > ret.
> >> >
> >> > H.J. Lu (5):
> >> >   x86: Add -mindirect-branch=
> >> >   x86: Add -mfunction-return=
> >> >   x86: Add -mindirect-branch-register
> >> >   x86: Add 'V' register operand modifier
> >> >   x86: Disallow -mindirect-branch=/-mfunction-return= with
> >> > -mcmodel=large
> >>
> >> Current set of patches don't seem to have any option to generate "lfence" 
> >> as the loop filler in "retpoline",  which is required by AMD.
> >> Can you please clarify the plan. We would like to get this checked-in GCC 
> >> 8.
> >
> > Since thunks are output as strings, it is easy to add the option
> > on the top of patch #1 of the series.  I do not fully understand
> > the reason for choosing pause over lfence for Intel, but if we need
> > to do both, we need to have command line option (and possibly attribute).
> > What would be reasonable name for it?
> 
> Looking at the kernel patch [1], the loop filler should be
> "pause;lfence" sequence, and should be universally accepted for Intel
> and AMD targets.
> 
> [1] https://www.spinics.net/lists/kernel/msg2697507.html

Yep, I would say we should go with pause;lfence now and see if we want to add 
argument
eventually.
HJ, does it sound OK?

Honza
> 
> Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Sun, Jan 14, 2018 at 11:40 AM, Jan Hubicka  wrote:
>> Hi HJ,
>>
>> > -Original Message-
>> > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
>> > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
>> > Sent: Sunday, January 14, 2018 9:07 AM
>> > To: gcc-patches@gcc.gnu.org
>> > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
>> >
>> > This set of patches for GCC 8 mitigates variant #2 of the speculative
>> > execution vulnerabilities on x86 processors identified by CVE-2017-5715, 
>> > aka
>> > Spectre.  They convert indirect branches and function returns to call and
>> > return thunks to avoid speculative execution via indirect call, jmp and 
>> > ret.
>> >
>> > H.J. Lu (5):
>> >   x86: Add -mindirect-branch=
>> >   x86: Add -mfunction-return=
>> >   x86: Add -mindirect-branch-register
>> >   x86: Add 'V' register operand modifier
>> >   x86: Disallow -mindirect-branch=/-mfunction-return= with
>> > -mcmodel=large
>>
>> Current set of patches don't seem to have any option to generate "lfence" as 
>> the loop filler in "retpoline",  which is required by AMD.
>> Can you please clarify the plan. We would like to get this checked-in GCC 8.
>
> Since thunks are output as strings, it is easy to add the option
> on the top of patch #1 of the series.  I do not fully understand
> the reason for choosing pause over lfence for Intel, but if we need
> to do both, we need to have command line option (and possibly attribute).
> What would be reasonable name for it?

Looking at the kernel patch [1], the loop filler should be
"pause;lfence" sequence, and should be universally accepted for Intel
and AMD targets.

[1] https://www.spinics.net/lists/kernel/msg2697507.html

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> Hi HJ, 
> 
> > -Original Message-
> > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> > ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> > Sent: Sunday, January 14, 2018 9:07 AM
> > To: gcc-patches@gcc.gnu.org
> > Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> > 
> > This set of patches for GCC 8 mitigates variant #2 of the speculative
> > execution vulnerabilities on x86 processors identified by CVE-2017-5715, aka
> > Spectre.  They convert indirect branches and function returns to call and
> > return thunks to avoid speculative execution via indirect call, jmp and ret.
> > 
> > H.J. Lu (5):
> >   x86: Add -mindirect-branch=
> >   x86: Add -mfunction-return=
> >   x86: Add -mindirect-branch-register
> >   x86: Add 'V' register operand modifier
> >   x86: Disallow -mindirect-branch=/-mfunction-return= with
> > -mcmodel=large
> 
> Current set of patches don't seem to have any option to generate "lfence" as 
> the loop filler in "retpoline",  which is required by AMD.
> Can you please clarify the plan. We would like to get this checked-in GCC 8.  
>  

Since thunks are output as strings, it is easy to add the option
on the top of patch #1 of the series.  I do not fully understand
the reason for choosing pause over lfence for Intel, but if we need
to do both, we need to have command line option (and possibly attribute).
What would be reasonable name for it?

Honza

RE: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Kumar, Venkataramanan]

Hi HJ, 

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> Sent: Sunday, January 14, 2018 9:07 AM
> To: gcc-patches@gcc.gnu.org
> Subject: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre
> 
> This set of patches for GCC 8 mitigates variant #2 of the speculative
> execution vulnerabilities on x86 processors identified by CVE-2017-5715, aka
> Spectre.  They convert indirect branches and function returns to call and
> return thunks to avoid speculative execution via indirect call, jmp and ret.
> 
> H.J. Lu (5):
>   x86: Add -mindirect-branch=
>   x86: Add -mfunction-return=
>   x86: Add -mindirect-branch-register
>   x86: Add 'V' register operand modifier
>   x86: Disallow -mindirect-branch=/-mfunction-return= with
> -mcmodel=large

Current set of patches don't seem to have any option to generate "lfence" as 
the loop filler in "retpoline",  which is required by AMD.
Can you please clarify the plan. We would like to get this checked-in GCC 8.   

> 
>  gcc/config/i386/constraints.md |  12 +-
>  gcc/config/i386/i386-opts.h|  13 +
>  gcc/config/i386/i386-protos.h  |   2 +
>  gcc/config/i386/i386.c | 822 
> -
>  gcc/config/i386/i386.h |  10 +
>  gcc/config/i386/i386.md|  69 +-
>  gcc/config/i386/i386.opt   |  28 +
>  gcc/config/i386/predicates.md  |  21 +-
>  gcc/doc/extend.texi|  22 +
>  gcc/doc/invoke.texi|  41 +-
>  gcc/testsuite/gcc.target/i386/indirect-thunk-1.c   |  19 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-10.c  |   7 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-2.c   |  19 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-3.c   |  20 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-4.c   |  20 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-5.c   |  16 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-6.c   |  17 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-7.c   |  43 ++
>  gcc/testsuite/gcc.target/i386/indirect-thunk-8.c   |   7 +
>  gcc/testsuite/gcc.target/i386/indirect-thunk-9.c   |   7 +
>  .../gcc.target/i386/indirect-thunk-attr-1.c|  22 +
>  .../gcc.target/i386/indirect-thunk-attr-10.c   |   9 +
>  .../gcc.target/i386/indirect-thunk-attr-11.c   |   9 +
>  .../gcc.target/i386/indirect-thunk-attr-2.c|  20 +
>  .../gcc.target/i386/indirect-thunk-attr-3.c|  21 +
>  .../gcc.target/i386/indirect-thunk-attr-4.c|  20 +
>  .../gcc.target/i386/indirect-thunk-attr-5.c|  22 +
>  .../gcc.target/i386/indirect-thunk-attr-6.c|  21 +
>  .../gcc.target/i386/indirect-thunk-attr-7.c|  44 ++
>  .../gcc.target/i386/indirect-thunk-attr-8.c|  41 +
>  .../gcc.target/i386/indirect-thunk-attr-9.c|   9 +
>  .../gcc.target/i386/indirect-thunk-bnd-1.c |  19 +
>  .../gcc.target/i386/indirect-thunk-bnd-2.c |  20 +
>  .../gcc.target/i386/indirect-thunk-bnd-3.c |  18 +
>  .../gcc.target/i386/indirect-thunk-bnd-4.c |  19 +
>  .../gcc.target/i386/indirect-thunk-extern-1.c  |  19 +
>  .../gcc.target/i386/indirect-thunk-extern-2.c  |  19 +
>  .../gcc.target/i386/indirect-thunk-extern-3.c  |  20 +
>  .../gcc.target/i386/indirect-thunk-extern-4.c  |  20 +
>  .../gcc.target/i386/indirect-thunk-extern-5.c  |  16 +
>  .../gcc.target/i386/indirect-thunk-extern-6.c  |  17 +
>  .../gcc.target/i386/indirect-thunk-extern-7.c  |  43 ++
>  .../gcc.target/i386/indirect-thunk-inline-1.c  |  18 +
>  .../gcc.target/i386/indirect-thunk-inline-2.c  |  18 +
>  .../gcc.target/i386/indirect-thunk-inline-3.c  |  19 +
>  .../gcc.target/i386/indirect-thunk-inline-4.c  |  19 +
>  .../gcc.target/i386/indirect-thunk-inline-5.c  |  15 +
>  .../gcc.target/i386/indirect-thunk-inline-6.c  |  16 +
>  .../gcc.target/i386/indirect-thunk-inline-7.c  |  42 ++
>  .../gcc.target/i386/indirect-thunk-register-1.c|  22 +
>  .../gcc.target/i386/indirect-thunk-register-2.c|  20 +
>  .../gcc.target/i386/indirect-thunk-register-3.c|  19 +
>  .../gcc.target/i386/indirect-thunk-register-4.c|  13 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-1.c|  12 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-10.c   |  22 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-11.c   |  22 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-12.c   |  21 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-13.c   |  21 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-14.c   |  21 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-15.c   |  21 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-16.c   |  18 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-17.c   |   7 +
>  gcc/testsuite/gcc.target/i386/ret-thunk-18.c   |   8 +
>  

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sat, 2018-01-13 at 13:01 +0100, Florian Weimer wrote:
> * David Woodhouse:
> 
> > 
> > > 
> > > Do you assume that you will eventually apply run-time patching to
> > > thunks (in case they aren't needed)?
> > "eventually"? We've been doing it for weeks. We are desperate to
> > release the kernel when can we have agreement on at *least* the
> > command line option and the name of the thunk? Internal implementation
> > details we really don't care about, but those we do.
>
> Thanks.  Are you content with patching just the thunk, or would you
> prefer patching the control transfer to it, too?

As things stand, GCC will put the target into a register and then call
the thunk.

We currently patch the thunk to turn it into a straight 'jmp *%\reg'
instead of the full retpoline (or perhaps 'lfence; jmp *\reg' on AMD).

This means we have the direct unconditional branch instruction emitted
by GCC, directly followed by a branch-to-register. And presumably you
aren't talking about deep magic here, just turning that 'call
__x86_indirect_branch_\reg' back into a 'call *\reg' inline in the C
code? Probably not even eliminating the load into the register in the
first place, for targets which actually came from memory?

So right now, for this specific use case, we are content with just
patching the thunk. It's not like that unconditional direct branch to
the thunk is hard for the CPU to predict. ;)

Now, if we're talking about the general case of wanting to runtime-
patch code emitted by GCC, there are much better use cases. Like
letting you emit 'movbe;nop;nop;nop' in appropriate cases, but telling
us where they are so we can hotpatch them into a 'mov;bswap' on CPUs
that need it. And other similar cases which are *much* more interesting
than the retpoline thing.¹

If we get that generic capability, then by all means we might as well
do the retpoline thunks too in future. But we're not losing sleep over
it right now.

What we're losing sleep over right now is the fact that we haven't even
got an agreement that the command line options, and the thunk symbol
name, are OK to use. Regardless of how you *implement* those, we really
want to commit the kernel code to use it within the next 24 hours...


¹ I actually just had a *really* nasty thought about that. If I start
the C file with asm(".include movbe-hack.h") and movbe-hack.h contains
a '.macro movbe' which does the normal Linux ALTERNATIVE thing... no,
you're going to come and hurt me, aren't you... 

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Florian Weimer]

* David Woodhouse:

>> Do you assume that you will eventually apply run-time patching to
>> thunks (in case they aren't needed)?
>
> "eventually"? We've been doing it for weeks. We are desperate to
> release the kernel when can we have agreement on at *least* the
> command line option and the name of the thunk? Internal implementation
> details we really don't care about, but those we do.

Thanks.  Are you content with patching just the thunk, or would you
prefer patching the control transfer to it, too?

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Fri, Jan 12, 2018 at 6:50 AM, Jan Hubicka  wrote:
>> On Fri, Jan 12, 2018 at 12:01 AM, Uros Bizjak  wrote:
>> > On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>> >
>> >> Hi Uros,
>> >>
>> >> Can you take a look at my x86 backend changes so that they are ready
>> >> to check in once we have consensus.
>> >
>> > Please finish the talks about the correct approach first. Once the
>> > consensus is reached, please post the final version of the patches for
>> > review.
>>
>> A  new set of patches are posted at
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01041.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01044.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01045.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01043.html
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01042.html
>>
>> I will submit an additional patch to disallow
>> -mindirect-branch=/-mfunction-return=
>> with -mshstk.
>>
>> > BTW: I have no detailed insight in these issues, so I'll look mostly
>> > at the implementation details, probably early next week.
>> >
>>
>> Kernel teams are waiting for the GCC 8 upstream patches.  They
>> have been using my GCC 7 backports for weeks now.   Jan, can
>> you review my patches before Uros has time next week?
>
> I have already read the original series, so I can take a look at the

Thanks.

> updated ones. Did we get some concensus on how much we want to do
> in middle-end?

I believe so.  Most of people, including Jeff, now think that my x86 specific
approach is the way to go.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jan Hubicka]

> On Fri, Jan 12, 2018 at 12:01 AM, Uros Bizjak  wrote:
> > On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
> >
> >> Hi Uros,
> >>
> >> Can you take a look at my x86 backend changes so that they are ready
> >> to check in once we have consensus.
> >
> > Please finish the talks about the correct approach first. Once the
> > consensus is reached, please post the final version of the patches for
> > review.
> 
> A  new set of patches are posted at
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01041.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01044.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01045.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01043.html
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01042.html
> 
> I will submit an additional patch to disallow
> -mindirect-branch=/-mfunction-return=
> with -mshstk.
> 
> > BTW: I have no detailed insight in these issues, so I'll look mostly
> > at the implementation details, probably early next week.
> >
> 
> Kernel teams are waiting for the GCC 8 upstream patches.  They
> have been using my GCC 7 backports for weeks now.   Jan, can
> you review my patches before Uros has time next week?

I have already read the original series, so I can take a look at the
updated ones. Did we get some concensus on how much we want to do
in middle-end?

Honza
> 
> Thanks.
> 
> 
> -- 
> H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Fri, Jan 12, 2018 at 12:01 AM, Uros Bizjak  wrote:
> On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:
>
>> Hi Uros,
>>
>> Can you take a look at my x86 backend changes so that they are ready
>> to check in once we have consensus.
>
> Please finish the talks about the correct approach first. Once the
> consensus is reached, please post the final version of the patches for
> review.

A  new set of patches are posted at

https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01041.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01044.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01045.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01043.html
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01042.html

I will submit an additional patch to disallow
-mindirect-branch=/-mfunction-return=
with -mshstk.

> BTW: I have no detailed insight in these issues, so I'll look mostly
> at the implementation details, probably early next week.
>

Kernel teams are waiting for the GCC 8 upstream patches.  They
have been using my GCC 7 backports for weeks now.   Jan, can
you review my patches before Uros has time next week?

Thanks.


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Uros Bizjak]

On Thu, Jan 11, 2018 at 2:28 PM, H.J. Lu  wrote:

> Hi Uros,
>
> Can you take a look at my x86 backend changes so that they are ready
> to check in once we have consensus.

Please finish the talks about the correct approach first. Once the
consensus is reached, please post the final version of the patches for
review.

BTW: I have no detailed insight in these issues, so I'll look mostly
at the implementation details, probably early next week.

Uros.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/11/2018 04:40 PM, Joseph Myers wrote:
> On Thu, 11 Jan 2018, Jeff Law wrote:
> 
>>> Well, given retpolines are largely kernel relevant right now we don't
>>> need to care here.
>> That's still TBD as far as I can tell.  I certainly hope we don't have
>> to go retpolines in user space, at least not in the general case.  I'm
>> holding out hope that the kernel folks are going to save the day.
> 
> I'd presume that just about any userspace process could have sensitive 
> data in its address space (e.g. cp, if it happens to be copying it at the 
> time).  
Yup.


> Is the expectation that the kernel will use IBRS/IBPB/STIBP 
> globally to shield processes from branch prediction state created by other 
> processes?  (As far as I can tell, microcode enabling IBRS/IBPB/STIBP is 
> only available for Ivy Bridge-EX and later at present, though I can't 
> locate any official Intel status information on microcode updates for 
> Spectre that have been released or are planned.)
I'm not sure of all the details of how it's supposed to work (assuming
it can) nor how much may or may not be covered by NDAs.  So it's
probably best to just stick with my statement that I hope that the
kernel folks can save the day here.

jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Joseph Myers]

On Thu, 11 Jan 2018, Jeff Law wrote:

> > Well, given retpolines are largely kernel relevant right now we don't
> > need to care here.
> That's still TBD as far as I can tell.  I certainly hope we don't have
> to go retpolines in user space, at least not in the general case.  I'm
> holding out hope that the kernel folks are going to save the day.

I'd presume that just about any userspace process could have sensitive 
data in its address space (e.g. cp, if it happens to be copying it at the 
time).  Is the expectation that the kernel will use IBRS/IBPB/STIBP 
globally to shield processes from branch prediction state created by other 
processes?  (As far as I can tell, microcode enabling IBRS/IBPB/STIBP is 
only available for Ivy Bridge-EX and later at present, though I can't 
locate any official Intel status information on microcode updates for 
Spectre that have been released or are planned.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Thu, 2018-01-11 at 22:17 +0100, Florian Weimer wrote:
> * David Woodhouse:
> 
> > 
> > On Sun, 2018-01-07 at 16:36 -0700, Jeff Law wrote:
> > > 
> > > 
> > > My fundamental problem with this patchkit is that it is 100% x86/x86_64
> > > specific.
> > > 
> > > ISTM we want a target independent mechanism (ie, new standard patterns,
> > > options, etc) then an x86/x86_64 implementation using that target
> > > independent framework (ie, the actual implementation of those new
> > > standard patterns).
> > From the kernel point of view, I'm not too worried about GCC internal
> > implementation details. What would be really useful to agree in short
> > order is the command-line options that invoke this behaviour, and the
> > ABI for the thunks.
>
> Do you assume that you will eventually apply run-time patching to
> thunks (in case they aren't needed)?

"eventually"? We've been doing it for weeks. We are desperate to
release the kernel when can we have agreement on at *least* the
command line option and the name of the thunk? Internal implementation
details we really don't care about, but those we do.

http://git.infradead.org/users/dwmw2/linux-retpoline.git


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Florian Weimer]

* David Woodhouse:

> On Sun, 2018-01-07 at 16:36 -0700, Jeff Law wrote:
>> 
>> My fundamental problem with this patchkit is that it is 100% x86/x86_64
>> specific.
>> 
>> ISTM we want a target independent mechanism (ie, new standard patterns,
>> options, etc) then an x86/x86_64 implementation using that target
>> independent framework (ie, the actual implementation of those new
>> standard patterns).
>
> From the kernel point of view, I'm not too worried about GCC internal
> implementation details. What would be really useful to agree in short
> order is the command-line options that invoke this behaviour, and the
> ABI for the thunks.

Do you assume that you will eventually apply run-time patching to
thunks (in case they aren't needed)?

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/11/2018 03:16 AM, Richard Biener wrote:
> On Thu, Jan 11, 2018 at 1:18 AM, Jeff Law  wrote:
>> On 01/10/2018 06:14 AM, Jakub Jelinek wrote:
>>> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
 On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
 wrote:
>> It's really just a couple of new primitives to emit a jump as a call and
>> one to slam in a new return address.  Given those I think you can do the
>> entire implementation as RTL at expansion time and you've got a damn
>> good shot at protecting most architectures from these kinds of attacks.
>
> I think that you're a bit optimistic here and that implementing a generic 
> and
> robust framework at the RTL level might require some time.  Given the 
> time and
> (back-)portability constraints, it might be wiser to rush into 
> architecture-
> specific countermeasures than to rush into an half-backed RTL framework.

 Let me also say that while it might be nice to commonize code introducing 
 these
 mitigations as late as possible to not disrupt optimization is important.  
 So I
 don't see a very strong motivation in trying very hard to make this more
 middle-endish, apart from maybe sharing helper functions where possible.
>>>
>>> That and perhaps a common option to handle the cases that are common to
>>> multiple backends (i.e. move some options from -m* namespace to -f*).
>>> I'd say the decision about the options and ABI of what we emit is more
>>> important than where we actually emit it, we can easily change where we do
>>> that over time, but not the options nor the ABI.
>> From a UI standpoint, I think the decision has already been made as LLVM
>> has already thrown -mretpolines into their tree.   Sigh.
> 
> Well, given retpolines are largely kernel relevant right now we don't
> need to care here.
That's still TBD as far as I can tell.  I certainly hope we don't have
to go retpolines in user space, at least not in the general case.  I'm
holding out hope that the kernel folks are going to save the day.


> 
>> So I think the one thing we ought to seriously consider is at least
>> reserving -mretpoline for this style of mitigation of spectre v2.  ALl
>> target's don't have to implementation this style mitigation, but if they
>> do, they use -mretpoline.
> 
> And I'd also like people not to bikeshed too much on this given we're
> in the situation
> of having exploitable kernels around for which we need a cooperating
> compiler.  So
> during the time we bikeshed this (rather than reviewing the actual
> patches) we have
> to "backport" the current non-upstream state anyway to deliver fixed
> kernels to our
> customer.
Believe me, after dealing with stack clash, I'm fully aware of the
constraints folks dealling with mitigation are under.

Jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/11/2018 03:04 AM, Alan Modra wrote:
> On Wed, Jan 10, 2018 at 05:13:36PM -0700, Jeff Law wrote:
>> On 01/08/2018 07:23 AM, Alan Modra wrote:
>>> On Sun, Jan 07, 2018 at 04:36:20PM -0700, Jeff Law wrote:
 On 01/07/2018 03:58 PM, H.J. Lu wrote:
> This set of patches for GCC 8 mitigates variant #2 of the speculative 
> execution
> vulnerabilities on x86 processors identified by CVE-2017-5715, aka 
> Spectre.
>>> [snip]
 My fundamental problem with this patchkit is that it is 100% x86/x86_64
 specific.
>>>
>>> It's possible that x86 needs spectre variant 2 mitigation that isn't
>>> necessary on other modern processors like ARM and PowerPC, so let's
>>> not rush into general solutions designed around x86..
>> >From what I know about variant 2 mitigation it's going to be needed on a
>> variety of chip families, not just the Intel architecture.
> 
> Yes.  I was thinking that it might be possible ignore variant 2
> attacks if there were no gadgets available anywhere in the victim
> address space, which is true enough but difficult to achieve.  That
> led me to think that indirect branches didn't matter, until someone
> pointed out that the indirect branch attack could be chained.  If you
> have the first part of a gadget, the read of interesting memory,
> followed by an indirect branch, that indirect branch can be spoofed
> into code that uses the interesting value in a way that affects cache
> state.
Note that even though variant 2 potentially applies to many
architectures and I believe we could do a fairly generic retpoline
implementation that would provide a high level of protection across many
targets that may not be the best choice.

As I mentioned in a message last night, some of the processor vendors
seem to want to go a different direction.  I'll summarize those as "add
this magic instruction to the indirect call/jump sequence".  They're
typically just a single insn and fairly trivial to implement.  They may
require using a different branch instruction as well.  But again,
trivial to implement.

Given that divergence I think we really want to look closely at HJ's
code for x86/x86_64 and let the other processor vendors work towards
their own mitigations -- ultimately with everyone doing their own thing
in their target files :(

Jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Thu, Jan 11, 2018 at 2:16 AM, Richard Biener
 wrote:
> On Thu, Jan 11, 2018 at 1:18 AM, Jeff Law  wrote:
>> On 01/10/2018 06:14 AM, Jakub Jelinek wrote:
>>> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
 On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
 wrote:
>> It's really just a couple of new primitives to emit a jump as a call and
>> one to slam in a new return address.  Given those I think you can do the
>> entire implementation as RTL at expansion time and you've got a damn
>> good shot at protecting most architectures from these kinds of attacks.
>
> I think that you're a bit optimistic here and that implementing a generic 
> and
> robust framework at the RTL level might require some time.  Given the 
> time and
> (back-)portability constraints, it might be wiser to rush into 
> architecture-
> specific countermeasures than to rush into an half-backed RTL framework.

 Let me also say that while it might be nice to commonize code introducing 
 these
 mitigations as late as possible to not disrupt optimization is important.  
 So I
 don't see a very strong motivation in trying very hard to make this more
 middle-endish, apart from maybe sharing helper functions where possible.
>>>
>>> That and perhaps a common option to handle the cases that are common to
>>> multiple backends (i.e. move some options from -m* namespace to -f*).
>>> I'd say the decision about the options and ABI of what we emit is more
>>> important than where we actually emit it, we can easily change where we do
>>> that over time, but not the options nor the ABI.
>> From a UI standpoint, I think the decision has already been made as LLVM
>> has already thrown -mretpolines into their tree.   Sigh.
>
> Well, given retpolines are largely kernel relevant right now we don't
> need to care here.
>
>> So I think the one thing we ought to seriously consider is at least
>> reserving -mretpoline for this style of mitigation of spectre v2.  ALl
>> target's don't have to implementation this style mitigation, but if they
>> do, they use -mretpoline.
>
> And I'd also like people not to bikeshed too much on this given we're
> in the situation
> of having exploitable kernels around for which we need a cooperating
> compiler.  So
> during the time we bikeshed this (rather than reviewing the actual
> patches) we have
> to "backport" the current non-upstream state anyway to deliver fixed
> kernels to our
> customer.
>
> Yes, if this were a "normal feature" we could continue discussing and
> trying to design
> sth nice and shiny.  But this isn't a normal feature.
>
> So please - I'd also like to get this into a released compiler (aka
> 7.3) as soon as possible
> (given a RC for 7.3 was scheduled to be early this week).

Hi Uros,

Can you take a look at my x86 backend changes so that they are ready
to check in once we have consensus.

Thanks.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Mon, Jan 8, 2018 at 3:37 AM, H.J. Lu  wrote:
> On Sun, Jan 7, 2018 at 8:07 PM, Sandra Loosemore
>  wrote:
>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>>>
>>> This set of patches for GCC 8 mitigates variant #2 of the speculative
>>> execution
>>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka
>>> Spectre.  They
>>> convert indirect branches to call and return thunks to avoid speculative
>>> execution
>>> via indirect call and jmp.
>>
>>
>> I have a general documentation issue with all the new command-line options
>> and attributes added by this patch set:  the documentation is very
>> implementor-speaky and doesn't explain what user-level problem they're
>> trying to solve.
>
> Do you have any suggestions?
>
>> E.g. to take just one example
>>
>>> +@item function_return("@var{choice}")
>>> +@cindex @code{function_return} function attribute, x86
>>> +On x86 targets, the @code{function_return} attribute causes the compiler
>>> +to convert function return with @var{choice}.  @samp{keep} keeps function
>>> +return unmodified.  @samp{thunk} converts function return to call and
>>> +return thunk.  @samp{thunk-inline} converts function return to inlined
>>> +call and return thunk.  @samp{thunk-extern} converts function return to
>>> +external call and return thunk provided in a separate object file.
>>
>>
>> Why would you want to mess with call and return code generation in this way?
>> The documentation doesn't say.
>>
>> For thunk-extern, is the programmer supposed to provide the thunk?  How
>> would you go about implementing the missing bit of code?  What should it do?
>> I'm compiler implementor and I wouldn't even know how to use this feature by
>> reading the manual; how would an ordinary application programmer who isn't
>> familiar with GCC internals know how to use it?

That is the best I can do.  My GCC documentation describes what my
patches generate.  The usage guidance should come from Intel white paper.
After it is published, I will submit a GCC patch to refer it.  It will
be very nice
for Intel white paper to recommend what compiler options should be used.

> This option was requested by Linux kernel people.  Linux kernel may
> choose different thunks at kernel load-time.  If a program doesn't know
> how to write external thunk, he/she shouldn't it.
>
>> If the goal here is to tell GCC to produce code that is protected against
>> the Spectre vulnerability, perhaps simplify this to adding just one option
>> that controls all the things you've given separate options and attributes
>> for?
>
> -mindirect-branch=thunk does the job.  Other options/choices are for
> fine tuning.
>
> Thanks.
>
> --
> H.J.



-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Thu, Jan 11, 2018 at 1:18 AM, Jeff Law  wrote:
> On 01/10/2018 06:14 AM, Jakub Jelinek wrote:
>> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
>>> On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
>>> wrote:
> It's really just a couple of new primitives to emit a jump as a call and
> one to slam in a new return address.  Given those I think you can do the
> entire implementation as RTL at expansion time and you've got a damn
> good shot at protecting most architectures from these kinds of attacks.

 I think that you're a bit optimistic here and that implementing a generic 
 and
 robust framework at the RTL level might require some time.  Given the time 
 and
 (back-)portability constraints, it might be wiser to rush into 
 architecture-
 specific countermeasures than to rush into an half-backed RTL framework.
>>>
>>> Let me also say that while it might be nice to commonize code introducing 
>>> these
>>> mitigations as late as possible to not disrupt optimization is important.  
>>> So I
>>> don't see a very strong motivation in trying very hard to make this more
>>> middle-endish, apart from maybe sharing helper functions where possible.
>>
>> That and perhaps a common option to handle the cases that are common to
>> multiple backends (i.e. move some options from -m* namespace to -f*).
>> I'd say the decision about the options and ABI of what we emit is more
>> important than where we actually emit it, we can easily change where we do
>> that over time, but not the options nor the ABI.
> From a UI standpoint, I think the decision has already been made as LLVM
> has already thrown -mretpolines into their tree.   Sigh.

Well, given retpolines are largely kernel relevant right now we don't
need to care here.

> So I think the one thing we ought to seriously consider is at least
> reserving -mretpoline for this style of mitigation of spectre v2.  ALl
> target's don't have to implementation this style mitigation, but if they
> do, they use -mretpoline.

And I'd also like people not to bikeshed too much on this given we're
in the situation
of having exploitable kernels around for which we need a cooperating
compiler.  So
during the time we bikeshed this (rather than reviewing the actual
patches) we have
to "backport" the current non-upstream state anyway to deliver fixed
kernels to our
customer.

Yes, if this were a "normal feature" we could continue discussing and
trying to design
sth nice and shiny.  But this isn't a normal feature.

So please - I'd also like to get this into a released compiler (aka
7.3) as soon as possible
(given a RC for 7.3 was scheduled to be early this week).

Thanks,
Richard.

>
> Jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Wed, Jan 10, 2018 at 2:52 PM, H.J. Lu  wrote:
> On Wed, Jan 10, 2018 at 5:14 AM, Jakub Jelinek  wrote:
>> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
>>> On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
>>> wrote:
>>> >> It's really just a couple of new primitives to emit a jump as a call and
>>> >> one to slam in a new return address.  Given those I think you can do the
>>> >> entire implementation as RTL at expansion time and you've got a damn
>>> >> good shot at protecting most architectures from these kinds of attacks.
>>> >
>>> > I think that you're a bit optimistic here and that implementing a generic 
>>> > and
>>> > robust framework at the RTL level might require some time.  Given the 
>>> > time and
>>> > (back-)portability constraints, it might be wiser to rush into 
>>> > architecture-
>>> > specific countermeasures than to rush into an half-backed RTL framework.
>>>
>>> Let me also say that while it might be nice to commonize code introducing 
>>> these
>>> mitigations as late as possible to not disrupt optimization is important.  
>>> So I
>>> don't see a very strong motivation in trying very hard to make this more
>>> middle-endish, apart from maybe sharing helper functions where possible.
>>
>> That and perhaps a common option to handle the cases that are common to
>> multiple backends (i.e. move some options from -m* namespace to -f*).
>> I'd say the decision about the options and ABI of what we emit is more
>> important than where we actually emit it, we can easily change where we do
>> that over time, but not the options nor the ABI.
>>
>
> My x86 mitigations are specific to x86 processors.  I don't know if
> these options
> are relevant to other processors.  However, it is a good to have a common
> option to enable mitigations, which can be built on top of processor specific
> options.  For example, -fmitigate-spectre may simply imply
>
> -mindirect-branch=thunk -mindirect-branch-register
>
> For kernel, they may want to use
>
> -mindirect-branch=thunk-extern -mindirect-branch-register
>
> instead.

Yes, that's a good idea (common -fFOO umbrella option mapping to target bits).

And of course targets can follow x86 in their -mindirect-branch-foo
flag namings (if semantic matches).

Richard.

> --
> H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Alan Modra]

On Wed, Jan 10, 2018 at 05:13:36PM -0700, Jeff Law wrote:
> On 01/08/2018 07:23 AM, Alan Modra wrote:
> > On Sun, Jan 07, 2018 at 04:36:20PM -0700, Jeff Law wrote:
> >> On 01/07/2018 03:58 PM, H.J. Lu wrote:
> >>> This set of patches for GCC 8 mitigates variant #2 of the speculative 
> >>> execution
> >>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka 
> >>> Spectre.
> > [snip]
> >> My fundamental problem with this patchkit is that it is 100% x86/x86_64
> >> specific.
> > 
> > It's possible that x86 needs spectre variant 2 mitigation that isn't
> > necessary on other modern processors like ARM and PowerPC, so let's
> > not rush into general solutions designed around x86..
> >From what I know about variant 2 mitigation it's going to be needed on a
> variety of chip families, not just the Intel architecture.

Yes.  I was thinking that it might be possible ignore variant 2
attacks if there were no gadgets available anywhere in the victim
address space, which is true enough but difficult to achieve.  That
led me to think that indirect branches didn't matter, until someone
pointed out that the indirect branch attack could be chained.  If you
have the first part of a gadget, the read of interesting memory,
followed by an indirect branch, that indirect branch can be spoofed
into code that uses the interesting value in a way that affects cache
state.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/10/2018 06:14 AM, Jakub Jelinek wrote:
> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
>> On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
>> wrote:
 It's really just a couple of new primitives to emit a jump as a call and
 one to slam in a new return address.  Given those I think you can do the
 entire implementation as RTL at expansion time and you've got a damn
 good shot at protecting most architectures from these kinds of attacks.
>>>
>>> I think that you're a bit optimistic here and that implementing a generic 
>>> and
>>> robust framework at the RTL level might require some time.  Given the time 
>>> and
>>> (back-)portability constraints, it might be wiser to rush into architecture-
>>> specific countermeasures than to rush into an half-backed RTL framework.
>>
>> Let me also say that while it might be nice to commonize code introducing 
>> these
>> mitigations as late as possible to not disrupt optimization is important.  
>> So I
>> don't see a very strong motivation in trying very hard to make this more
>> middle-endish, apart from maybe sharing helper functions where possible.
> 
> That and perhaps a common option to handle the cases that are common to
> multiple backends (i.e. move some options from -m* namespace to -f*).
> I'd say the decision about the options and ABI of what we emit is more
> important than where we actually emit it, we can easily change where we do
> that over time, but not the options nor the ABI.
>From a UI standpoint, I think the decision has already been made as LLVM
has already thrown -mretpolines into their tree.   Sigh.

So I think the one thing we ought to seriously consider is at least
reserving -mretpoline for this style of mitigation of spectre v2.  ALl
target's don't have to implementation this style mitigation, but if they
do, they use -mretpoline.

Jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/08/2018 07:23 AM, Alan Modra wrote:
> On Sun, Jan 07, 2018 at 04:36:20PM -0700, Jeff Law wrote:
>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>>> This set of patches for GCC 8 mitigates variant #2 of the speculative 
>>> execution
>>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.
> [snip]
>> My fundamental problem with this patchkit is that it is 100% x86/x86_64
>> specific.
> 
> It's possible that x86 needs spectre variant 2 mitigation that isn't
> necessary on other modern processors like ARM and PowerPC, so let's
> not rush into general solutions designed around x86..
>From what I know about variant 2 mitigation it's going to be needed on a
variety of chip families, not just the Intel architecture.

However, I'm seeing signals that other chips vendors are looking towards
approaches that don't use retpolines.  So even though I think we could
build them fairly easy for most targets out of simple primitives, it may
not be the best use of our time.

> 
> Here's a quick overview of Spectre.  For more, see
> https://spectreattack.com/spectre.pdf
> https://googleprojectzero.blogspot.com.au/2018/01/reading-privileged-memory-with-side.html
> https://developer.arm.com/-/media/Files/pdf/Cache_Speculation_Side-channels.pdf
Yup.  Already familiar with this stuff :-)

> 
> However, x86 has the additional problem of variable length
> instructions.  Gadgets might be hiding in code when executed at an
> offset from the start of the "real" instructions.  Which is why x86 is
> more at risk from this attack than other processors, and why x86 needs
> something like the posted variant 2 mitigation, slowing down all
> indirect branches.
> 
True, but largely beside the point.   I'm not aware of anyone serious
looking at mating ROP with Spectre at this point, though it is certainly
possible.  The bad guys don't need to work that hard at this time.


Jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Wed, Jan 10, 2018 at 5:14 AM, Jakub Jelinek  wrote:
> On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
>> On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  
>> wrote:
>> >> It's really just a couple of new primitives to emit a jump as a call and
>> >> one to slam in a new return address.  Given those I think you can do the
>> >> entire implementation as RTL at expansion time and you've got a damn
>> >> good shot at protecting most architectures from these kinds of attacks.
>> >
>> > I think that you're a bit optimistic here and that implementing a generic 
>> > and
>> > robust framework at the RTL level might require some time.  Given the time 
>> > and
>> > (back-)portability constraints, it might be wiser to rush into 
>> > architecture-
>> > specific countermeasures than to rush into an half-backed RTL framework.
>>
>> Let me also say that while it might be nice to commonize code introducing 
>> these
>> mitigations as late as possible to not disrupt optimization is important.  
>> So I
>> don't see a very strong motivation in trying very hard to make this more
>> middle-endish, apart from maybe sharing helper functions where possible.
>
> That and perhaps a common option to handle the cases that are common to
> multiple backends (i.e. move some options from -m* namespace to -f*).
> I'd say the decision about the options and ABI of what we emit is more
> important than where we actually emit it, we can easily change where we do
> that over time, but not the options nor the ABI.
>

My x86 mitigations are specific to x86 processors.  I don't know if
these options
are relevant to other processors.  However, it is a good to have a common
option to enable mitigations, which can be built on top of processor specific
options.  For example, -fmitigate-spectre may simply imply

-mindirect-branch=thunk -mindirect-branch-register

For kernel, they may want to use

-mindirect-branch=thunk-extern -mindirect-branch-register

instead.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jakub Jelinek]

On Wed, Jan 10, 2018 at 02:08:48PM +0100, Richard Biener wrote:
> On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  wrote:
> >> It's really just a couple of new primitives to emit a jump as a call and
> >> one to slam in a new return address.  Given those I think you can do the
> >> entire implementation as RTL at expansion time and you've got a damn
> >> good shot at protecting most architectures from these kinds of attacks.
> >
> > I think that you're a bit optimistic here and that implementing a generic 
> > and
> > robust framework at the RTL level might require some time.  Given the time 
> > and
> > (back-)portability constraints, it might be wiser to rush into architecture-
> > specific countermeasures than to rush into an half-backed RTL framework.
> 
> Let me also say that while it might be nice to commonize code introducing 
> these
> mitigations as late as possible to not disrupt optimization is important.  So 
> I
> don't see a very strong motivation in trying very hard to make this more
> middle-endish, apart from maybe sharing helper functions where possible.

That and perhaps a common option to handle the cases that are common to
multiple backends (i.e. move some options from -m* namespace to -f*).
I'd say the decision about the options and ABI of what we emit is more
important than where we actually emit it, we can easily change where we do
that over time, but not the options nor the ABI.

Jakub

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Richard Biener]

On Wed, Jan 10, 2018 at 11:18 AM, Eric Botcazou  wrote:
>> It's really just a couple of new primitives to emit a jump as a call and
>> one to slam in a new return address.  Given those I think you can do the
>> entire implementation as RTL at expansion time and you've got a damn
>> good shot at protecting most architectures from these kinds of attacks.
>
> I think that you're a bit optimistic here and that implementing a generic and
> robust framework at the RTL level might require some time.  Given the time and
> (back-)portability constraints, it might be wiser to rush into architecture-
> specific countermeasures than to rush into an half-backed RTL framework.

Let me also say that while it might be nice to commonize code introducing these
mitigations as late as possible to not disrupt optimization is important.  So I
don't see a very strong motivation in trying very hard to make this more
middle-endish, apart from maybe sharing helper functions where possible.

Richard.

> --
> Eric Botcazou

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Wed, Jan 10, 2018 at 2:18 AM, Eric Botcazou  wrote:
>> It's really just a couple of new primitives to emit a jump as a call and
>> one to slam in a new return address.  Given those I think you can do the
>> entire implementation as RTL at expansion time and you've got a damn
>> good shot at protecting most architectures from these kinds of attacks.
>
> I think that you're a bit optimistic here and that implementing a generic and
> robust framework at the RTL level might require some time.  Given the time and
> (back-)portability constraints, it might be wiser to rush into architecture-
> specific countermeasures than to rush into an half-backed RTL framework.
>

We have tried to implement this in target-independent IR with a different
compiler.  We run into a couple issues:

1. Some optimizations aren't performed since optimizers don't understand
our code sequences.
2. Some passes insert instructions between our code sequences, which
leads to invalid codes.

All of them can be resolved, given enough time.  I don't know how long
it will take to make generic RTL approach as robust as x86 backend
specific implementation, which just converts indirect branch and return
to different functional equivalent code sequences.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Eric Botcazou]

> It's really just a couple of new primitives to emit a jump as a call and
> one to slam in a new return address.  Given those I think you can do the
> entire implementation as RTL at expansion time and you've got a damn
> good shot at protecting most architectures from these kinds of attacks.

I think that you're a bit optimistic here and that implementing a generic and 
robust framework at the RTL level might require some time.  Given the time and 
(back-)portability constraints, it might be wiser to rush into architecture-
specific countermeasures than to rush into an half-backed RTL framework.

-- 
Eric Botcazou

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Tue, Jan 9, 2018 at 10:52 AM, Jeff Law  wrote:
> On 01/07/2018 05:29 PM, H.J. Lu wrote:
>> On Sun, Jan 7, 2018 at 3:36 PM, Jeff Law  wrote:
>>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
 This set of patches for GCC 8 mitigates variant #2 of the speculative 
 execution
 vulnerabilities on x86 processors identified by CVE-2017-5715, aka 
 Spectre.  They
 convert indirect branches to call and return thunks to avoid speculative 
 execution
 via indirect call and jmp.


 H.J. Lu (5):
   x86: Add -mindirect-branch=
   x86: Add -mindirect-branch-loop=
   x86: Add -mfunction-return=
   x86: Add -mindirect-branch-register
   x86: Add 'V' register operand modifier

  gcc/config/i386/constraints.md |  12 +-
  gcc/config/i386/i386-opts.h|  14 +
  gcc/config/i386/i386-protos.h  |   2 +
  gcc/config/i386/i386.c | 655 
 -
  gcc/config/i386/i386.h |  10 +
  gcc/config/i386/i386.md|  51 +-
  gcc/config/i386/i386.opt   |  45 ++
  gcc/config/i386/predicates.md  |  21 +-
  gcc/doc/extend.texi|  22 +
  gcc/doc/invoke.texi|  37 +-
>>> My fundamental problem with this patchkit is that it is 100% x86/x86_64
>>> specific.
>>>
>>> ISTM we want a target independent mechanism (ie, new standard patterns,
>>> options, etc) then an x86/x86_64 implementation using that target
>>> independent framework (ie, the actual implementation of those new
>>> standard patterns).
>>>
>>
>> My patch set is implemented with some constraints:
>>
>> 1. They need to be backportable to GCC 7/6/5/4.x.
>> 2. They should work with all compiler optimizations.
>> 3. They need to generate code sequences are x86 specific, which can't be
>> changed in any shape or form.  And the generated codes are quite opposite
>> to what a good optimizing compiler should generate.
>>
>> Given that these conditions, I kept existing indirect call, jump and
>> return patterns.
>> I generated different code sequences for these patterns during the final pass
>> when generating assembly codes.
>>
>> I guess that I could add a late target independent RTL pass to convert
>> indirect call, jump and return patterns to something else.  But I am not sure
>> if that is what you are looking for.
> I don't see how those constraints are incompatible with doing most of
> this work at a higher level in the compiler.  You just surround the
> resulting RTL bits with appropriate barriers to prevent them from
> getting mucked up by the optimizers.  Actually I think you're going to
> need one barrier in the middle of the sequence to keep bbro at bay
> within the sequence as a whole.
>
> It's really just a couple of new primitives to emit a jump as a call and
> one to slam in a new return address.  Given those I think you can do the
> entire implementation as RTL at expansion time and you've got a damn
> good shot at protecting most architectures from these kinds of attacks.

Doing this at RTL expansion time may not work well with RTL optimizing
paases nor IRA/LRA.  We may wind up undoing all kinds of optimizations
as well code sequences.


-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/08/2018 03:01 AM, Martin Liška wrote:
> On 01/08/2018 01:29 AM, H.J. Lu wrote:
>> 1. They need to be backportable to GCC 7/6/5/4.x.
> 
> I must admit this is very important constrain. To be honest, we're planning
> to backport the patchset to GCC 4.3.
Red Hat would likely be backporting a ways as well.  But I don't think
doing this at expand rather than in the targets would affect
backportability all that much, nor do I think that backporting to 10
year old compilers like SuSE and Red Hat do should drive the design
decisions :-)

jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/07/2018 05:29 PM, H.J. Lu wrote:
> On Sun, Jan 7, 2018 at 3:36 PM, Jeff Law  wrote:
>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>>> This set of patches for GCC 8 mitigates variant #2 of the speculative 
>>> execution
>>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre. 
>>>  They
>>> convert indirect branches to call and return thunks to avoid speculative 
>>> execution
>>> via indirect call and jmp.
>>>
>>>
>>> H.J. Lu (5):
>>>   x86: Add -mindirect-branch=
>>>   x86: Add -mindirect-branch-loop=
>>>   x86: Add -mfunction-return=
>>>   x86: Add -mindirect-branch-register
>>>   x86: Add 'V' register operand modifier
>>>
>>>  gcc/config/i386/constraints.md |  12 +-
>>>  gcc/config/i386/i386-opts.h|  14 +
>>>  gcc/config/i386/i386-protos.h  |   2 +
>>>  gcc/config/i386/i386.c | 655 
>>> -
>>>  gcc/config/i386/i386.h |  10 +
>>>  gcc/config/i386/i386.md|  51 +-
>>>  gcc/config/i386/i386.opt   |  45 ++
>>>  gcc/config/i386/predicates.md  |  21 +-
>>>  gcc/doc/extend.texi|  22 +
>>>  gcc/doc/invoke.texi|  37 +-
>> My fundamental problem with this patchkit is that it is 100% x86/x86_64
>> specific.
>>
>> ISTM we want a target independent mechanism (ie, new standard patterns,
>> options, etc) then an x86/x86_64 implementation using that target
>> independent framework (ie, the actual implementation of those new
>> standard patterns).
>>
> 
> My patch set is implemented with some constraints:
> 
> 1. They need to be backportable to GCC 7/6/5/4.x.
> 2. They should work with all compiler optimizations.
> 3. They need to generate code sequences are x86 specific, which can't be
> changed in any shape or form.  And the generated codes are quite opposite
> to what a good optimizing compiler should generate.
> 
> Given that these conditions, I kept existing indirect call, jump and
> return patterns.
> I generated different code sequences for these patterns during the final pass
> when generating assembly codes.
> 
> I guess that I could add a late target independent RTL pass to convert
> indirect call, jump and return patterns to something else.  But I am not sure
> if that is what you are looking for.
I don't see how those constraints are incompatible with doing most of
this work at a higher level in the compiler.  You just surround the
resulting RTL bits with appropriate barriers to prevent them from
getting mucked up by the optimizers.  Actually I think you're going to
need one barrier in the middle of the sequence to keep bbro at bay
within the sequence as a whole.

It's really just a couple of new primitives to emit a jump as a call and
one to slam in a new return address.  Given those I think you can do the
entire implementation as RTL at expansion time and you've got a damn
good shot at protecting most architectures from these kinds of attacks.

jeff

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Sun, 2018-01-07 at 16:36 -0700, Jeff Law wrote:
> 
> My fundamental problem with this patchkit is that it is 100% x86/x86_64
> specific.
> 
> ISTM we want a target independent mechanism (ie, new standard patterns,
> options, etc) then an x86/x86_64 implementation using that target
> independent framework (ie, the actual implementation of those new
> standard patterns).

From the kernel point of view, I'm not too worried about GCC internal
implementation details. What would be really useful to agree in short
order is the command-line options that invoke this behaviour, and the
ABI for the thunks. 

Once that's done, we can push the patches to Linus and people can build
safe kernels, and we can build with HJ's existing patch set for the
time being. And you can bikeshed the rest to your collective hearts'
content... :)

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[David Woodhouse]

On Mon, 2018-01-08 at 09:27 +0100, Florian Weimer wrote:
> * H. J. Lu:
> 
> > 
> > This set of patches for GCC 8 mitigates variant #2 of the
> > speculative execution vulnerabilities on x86 processors identified
> > by CVE-2017-5715, aka Spectre.  They convert indirect branches to
> > call and return thunks to avoid speculative execution via indirect
> > call and jmp.
> Would it make sense to add a mode which relies on an empty return
> stack cache?  Or will CPUs use the regular branch predictor if the
> return stack is empty?
> 
> With an empty return stack cache and no branch predictor, a simple
> PUSH/RET sequence cannot be predicted, so the complex CALL sequence
> with a speculation barrier is not needed.

Some CPUs will use the regular branch predictor if the RSB is empty.
Others just round-robin the RSB and will use the *oldest* entry if they
underflow.



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Florian Weimer]

* Sandra Loosemore:

> I have a general documentation issue with all the new command-line 
> options and attributes added by this patch set:  the documentation is 
> very implementor-speaky and doesn't explain what user-level problem 
> they're trying to solve.

Agreed.  Ideally, the documentation would also list the CPU
models/model groups where it is known to have the desired effect, and
if firmware updates are needed.

For some users, it may be useful to be able to advertise that they
have built their binaries with hardening, but another group of users
is interested in hardening which actually works to stop all potential
exploits.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Florian Weimer]

* H. J. Lu:

> This set of patches for GCC 8 mitigates variant #2 of the
> speculative execution vulnerabilities on x86 processors identified
> by CVE-2017-5715, aka Spectre.  They convert indirect branches to
> call and return thunks to avoid speculative execution via indirect
> call and jmp.

Would it make sense to add a mode which relies on an empty return
stack cache?  Or will CPUs use the regular branch predictor if the
return stack is empty?

With an empty return stack cache and no branch predictor, a simple
PUSH/RET sequence cannot be predicted, so the complex CALL sequence
with a speculation barrier is not needed.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Michael Matz]

Hi,

On Mon, 8 Jan 2018, Jakub Jelinek wrote:

> On Mon, Jan 08, 2018 at 07:00:11AM -0800, H.J. Lu wrote:
> > See:
> > 
> > https://sourceware.org/ml/binutils/2017-11/msg00369.html
> 
> Program Headers:
>   Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   LOAD   0x00 0x 0x 0x00200 0x00200 R   0x20
>   LOAD   0x000fd0 0x00200fd0 0x00200fd0 0x0002b 0x0002b R E 0x20
>   LOAD   0x001000 0x00201000 0x00201000 0x00058 0x00058 R   0x20
>   LOAD   0x200f80 0x00400f80 0x00400f80 0x000a0 0x000a0 RW  0x20
>   DYNAMIC0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 RW  0x4
>   GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
>   GNU_RELRO  0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 R   0x1
> 
> Uh, 3 read-only LOADs instead of 2?  Shouldn't then all the read-only
> non-executable sections be emitted together, so that you have a R, then R E,
> then RW PT_LOADs?

See also my subthread starting at H.J. first version of the set:
  https://sourceware.org/ml/binutils/2017-11/msg00218.html
where some of the issues are hashed through.


Ciao,
Michael.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jakub Jelinek]

On Mon, Jan 08, 2018 at 08:17:27AM -0800, H.J. Lu wrote:
> On Mon, Jan 8, 2018 at 7:06 AM, Jakub Jelinek  wrote:
> > On Mon, Jan 08, 2018 at 07:00:11AM -0800, H.J. Lu wrote:
> >> See:
> >>
> >> https://sourceware.org/ml/binutils/2017-11/msg00369.html
> >
> > Program Headers:
> >   Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> >   LOAD   0x00 0x 0x 0x00200 0x00200 R   0x20
> >   LOAD   0x000fd0 0x00200fd0 0x00200fd0 0x0002b 0x0002b R E 0x20
> >   LOAD   0x001000 0x00201000 0x00201000 0x00058 0x00058 R   0x20
> >   LOAD   0x200f80 0x00400f80 0x00400f80 0x000a0 0x000a0 RW  0x20
> >   DYNAMIC0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 RW  0x4
> >   GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
> >   GNU_RELRO  0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 R   0x1
> >
> > Uh, 3 read-only LOADs instead of 2?  Shouldn't then all the read-only
> > non-executable sections be emitted together, so that you have a R, then R E,
> > then RW PT_LOADs?
> 
> It is done on purpose since the second RO segment will be merged with the 
> RELRO
> segment at load time:

That doesn't look like an advantage over not introducing it.

> Elf file type is EXEC (Executable file)
> Entry point 0x401ea0
> There are 11 program headers, starting at offset 52
> 
> Program Headers:
>   Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR   0x34 0x00400034 0x00400034 0x00160 0x00160 R   0x4
>   INTERP 0x000194 0x00400194 0x00400194 0x0001a 0x0001a R   0x1
>   [Requesting program interpreter: /libx32/ld-linux-x32.so.2]
>   LOAD   0x00 0x0040 0x0040 0x0037c 0x0037c R   0x1000
>   LOAD   0x000e68 0x00401e68 0x00401e68 0x00195 0x00195 R E 0x1000
>   LOAD   0x001000 0x00402000 0x00402000 0x00124 0x00124 R   0x1000
>   LOAD   0x001ef0 0x00402ef0 0x00402ef0 0x00134 0x00138 RW  0x1000
>   DYNAMIC0x001ef8 0x00402ef8 0x00402ef8 0x000f8 0x000f8 RW  0x4
>   NOTE   0x0001b0 0x004001b0 0x004001b0 0x00044 0x00044 R   0x4
>   GNU_EH_FRAME   0x001008 0x00402008 0x00402008 0x00034 0x00034 R   0x4
>   GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
>   GNU_RELRO  0x001ef0 0x00402ef0 0x00402ef0 0x00110 0x00110 R   0x1

   PHDR   0x34 0x00400034 0x00400034 0x00160 0x00160 R   0x4
   INTERP 0x000194 0x00400194 0x00400194 0x0001a 0x0001a R   0x1
   [Requesting program interpreter: /libx32/ld-linux-x32.so.2]
   LOAD   0x00 0x0040 0x0040 0x004a0 0x004a0 R   0x1000
   LOAD   0x000e68 0x00401e68 0x00401e68 0x00195 0x00195 R E 0x1000
   LOAD   0x001ef0 0x00402ef0 0x00402ef0 0x00134 0x00138 RW  0x1000
   DYNAMIC0x001ef8 0x00402ef8 0x00402ef8 0x000f8 0x000f8 RW  0x4
   NOTE   0x0001b0 0x004001b0 0x004001b0 0x00044 0x00044 R   0x4
   GNU_EH_FRAME   0x001008 0x00402008 0x00402008 0x00034 0x00034 R   0x4
   GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
   GNU_RELRO  0x001ef0 0x00402ef0 0x00402ef0 0x00110 0x00110 R   0x1

you could even more the second PT_LOAD earlier to make the gaps on disk
smaller.

Jakub

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Mon, Jan 8, 2018 at 7:06 AM, Jakub Jelinek  wrote:
> On Mon, Jan 08, 2018 at 07:00:11AM -0800, H.J. Lu wrote:
>> See:
>>
>> https://sourceware.org/ml/binutils/2017-11/msg00369.html
>
> Program Headers:
>   Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   LOAD   0x00 0x 0x 0x00200 0x00200 R   0x20
>   LOAD   0x000fd0 0x00200fd0 0x00200fd0 0x0002b 0x0002b R E 0x20
>   LOAD   0x001000 0x00201000 0x00201000 0x00058 0x00058 R   0x20
>   LOAD   0x200f80 0x00400f80 0x00400f80 0x000a0 0x000a0 RW  0x20
>   DYNAMIC0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 RW  0x4
>   GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
>   GNU_RELRO  0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 R   0x1
>
> Uh, 3 read-only LOADs instead of 2?  Shouldn't then all the read-only
> non-executable sections be emitted together, so that you have a R, then R E,
> then RW PT_LOADs?

It is done on purpose since the second RO segment will be merged with the RELRO
segment at load time:

Elf file type is EXEC (Executable file)
Entry point 0x401ea0
There are 11 program headers, starting at offset 52

Program Headers:
  Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR   0x34 0x00400034 0x00400034 0x00160 0x00160 R   0x4
  INTERP 0x000194 0x00400194 0x00400194 0x0001a 0x0001a R   0x1
  [Requesting program interpreter: /libx32/ld-linux-x32.so.2]
  LOAD   0x00 0x0040 0x0040 0x0037c 0x0037c R   0x1000
  LOAD   0x000e68 0x00401e68 0x00401e68 0x00195 0x00195 R E 0x1000
  LOAD   0x001000 0x00402000 0x00402000 0x00124 0x00124 R   0x1000
  LOAD   0x001ef0 0x00402ef0 0x00402ef0 0x00134 0x00138 RW  0x1000
  DYNAMIC0x001ef8 0x00402ef8 0x00402ef8 0x000f8 0x000f8 RW  0x4
  NOTE   0x0001b0 0x004001b0 0x004001b0 0x00044 0x00044 R   0x4
  GNU_EH_FRAME   0x001008 0x00402008 0x00402008 0x00034 0x00034 R   0x4
  GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
  GNU_RELRO  0x001ef0 0x00402ef0 0x00402ef0 0x00110 0x00110 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00
   01 .interp
   02 .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym
.dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt
   03 .init .plt .text .fini
   04 .rodata .eh_frame_hdr .eh_frame
   05 .init_array .fini_array .dynamic .got .got.plt .data .bss
   06 .dynamic
   07 .note.ABI-tag .note.gnu.build-id
   08 .eh_frame_hdr
   09
   10 .init_array .fini_array .dynamic .got



-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jakub Jelinek]

On Mon, Jan 08, 2018 at 07:00:11AM -0800, H.J. Lu wrote:
> See:
> 
> https://sourceware.org/ml/binutils/2017-11/msg00369.html

Program Headers:
  Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD   0x00 0x 0x 0x00200 0x00200 R   0x20
  LOAD   0x000fd0 0x00200fd0 0x00200fd0 0x0002b 0x0002b R E 0x20
  LOAD   0x001000 0x00201000 0x00201000 0x00058 0x00058 R   0x20
  LOAD   0x200f80 0x00400f80 0x00400f80 0x000a0 0x000a0 RW  0x20
  DYNAMIC0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 RW  0x4
  GNU_STACK  0x00 0x 0x 0x0 0x0 RW  0x10
  GNU_RELRO  0x200f80 0x00400f80 0x00400f80 0x00080 0x00080 R   0x1

Uh, 3 read-only LOADs instead of 2?  Shouldn't then all the read-only
non-executable sections be emitted together, so that you have a R, then R E,
then RW PT_LOADs?

Jakub

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Mon, Jan 8, 2018 at 6:23 AM, Alan Modra  wrote:
> On Sun, Jan 07, 2018 at 04:36:20PM -0700, Jeff Law wrote:
>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>> > This set of patches for GCC 8 mitigates variant #2 of the speculative 
>> > execution
>> > vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.
> [snip]
>> My fundamental problem with this patchkit is that it is 100% x86/x86_64
>> specific.
>
> It's possible that x86 needs spectre variant 2 mitigation that isn't
> necessary on other modern processors like ARM and PowerPC, so let's
> not rush into general solutions designed around x86..
>
> Here's a quick overview of Spectre.  For more, see
> https://spectreattack.com/spectre.pdf
> https://googleprojectzero.blogspot.com.au/2018/01/reading-privileged-memory-with-side.html
> https://developer.arm.com/-/media/Files/pdf/Cache_Speculation_Side-channels.pdf
>
> The simplest example of ideal "gadget" code that can be exploited by
> an attacker who can control the value of x, perhaps as a parameter to
> some service provided by the victim is:
>
> char *array1, *array2;
> y = array2[array1[x] * cache_line_size];
>
> The idea being that with the appropriate x, array1[x] can load any
> byte of interest in the victim, with the array2 load evicting a cache
> line detectable by the attacker.  The value of the byte of interest
> can then be inferred by which cache line was affected.
>
> Typical code of course checks user input.
>
> if (x < array1_size)
>   y = array2[array1[x] * cache_line_size];
>
> Spectre variant 1 preloads the branch predictor to make the condition
> predict as true.  Then when the out-of-range value of x is given,
> speculative execution runs the gadget code affecting the cache.  Even
> though this speculative execution is rolled back, the cache remains
> affected..
>
> Spectre variant 2 preloads the branch target predictor for indirect
> branches so that some indirect branch in the victim, eg. a PLT call,
> speculatively executes gadget code found somewhere in the victim.
>
>
> So, to mitigate Spectre variant 1, ensure that speculative execution
> doesn't get as far as the array2 load.  You could do that by modifying
> the above code to
>
> if (x < array1_size)
>   {
> /* speculation barrier goes here */
> y = array2[array1[x] * cache_line_size];
>   }
>
> But you could also do
>
> if (x < array1_size)
>   {
> tmp = array1[x] * cache_line_size;
> /* speculation barrier goes here */
> y = array2[tmp];
>   }
>
> This has the advantage of killing variant 2 attacks for this gadget
> too.  If you ensure there are no gadgets anywhere, then variant 2
> attacks are not possible.  Besides compiler changes to prevent gadgets
> being emitted you also need compiler and linker changes to not emit
> read-only data in executable segments, because data might just happen
> to be a gadget when executed.

See:

https://sourceware.org/ml/binutils/2017-11/msg00369.html

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Alan Modra]

On Sun, Jan 07, 2018 at 04:36:20PM -0700, Jeff Law wrote:
> On 01/07/2018 03:58 PM, H.J. Lu wrote:
> > This set of patches for GCC 8 mitigates variant #2 of the speculative 
> > execution
> > vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.
[snip]
> My fundamental problem with this patchkit is that it is 100% x86/x86_64
> specific.

It's possible that x86 needs spectre variant 2 mitigation that isn't
necessary on other modern processors like ARM and PowerPC, so let's
not rush into general solutions designed around x86..

Here's a quick overview of Spectre.  For more, see
https://spectreattack.com/spectre.pdf
https://googleprojectzero.blogspot.com.au/2018/01/reading-privileged-memory-with-side.html
https://developer.arm.com/-/media/Files/pdf/Cache_Speculation_Side-channels.pdf

The simplest example of ideal "gadget" code that can be exploited by
an attacker who can control the value of x, perhaps as a parameter to
some service provided by the victim is:

char *array1, *array2;
y = array2[array1[x] * cache_line_size];

The idea being that with the appropriate x, array1[x] can load any
byte of interest in the victim, with the array2 load evicting a cache
line detectable by the attacker.  The value of the byte of interest
can then be inferred by which cache line was affected.

Typical code of course checks user input.

if (x < array1_size)
  y = array2[array1[x] * cache_line_size];

Spectre variant 1 preloads the branch predictor to make the condition
predict as true.  Then when the out-of-range value of x is given,
speculative execution runs the gadget code affecting the cache.  Even
though this speculative execution is rolled back, the cache remains
affected..

Spectre variant 2 preloads the branch target predictor for indirect
branches so that some indirect branch in the victim, eg. a PLT call,
speculatively executes gadget code found somewhere in the victim.


So, to mitigate Spectre variant 1, ensure that speculative execution
doesn't get as far as the array2 load.  You could do that by modifying
the above code to

if (x < array1_size)
  {
/* speculation barrier goes here */
y = array2[array1[x] * cache_line_size];
  }

But you could also do

if (x < array1_size)
  {
tmp = array1[x] * cache_line_size;
/* speculation barrier goes here */
y = array2[tmp];
  }

This has the advantage of killing variant 2 attacks for this gadget
too.  If you ensure there are no gadgets anywhere, then variant 2
attacks are not possible.  Besides compiler changes to prevent gadgets
being emitted you also need compiler and linker changes to not emit
read-only data in executable segments, because data might just happen
to be a gadget when executed.

However, x86 has the additional problem of variable length
instructions.  Gadgets might be hiding in code when executed at an
offset from the start of the "real" instructions.  Which is why x86 is
more at risk from this attack than other processors, and why x86 needs
something like the posted variant 2 mitigation, slowing down all
indirect branches.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 7, 2018 at 10:55 PM, Markus Trippelsdorf
 wrote:
> On 2018.01.07 at 21:07 -0700, Sandra Loosemore wrote:
>> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>> > This set of patches for GCC 8 mitigates variant #2 of the speculative 
>> > execution
>> > vulnerabilities on x86 processors identified by CVE-2017-5715, aka 
>> > Spectre.  They
>> > convert indirect branches to call and return thunks to avoid speculative 
>> > execution
>> > via indirect call and jmp.
>>
>> I have a general documentation issue with all the new command-line
>> options and attributes added by this patch set:  the documentation is
>> very implementor-speaky and doesn't explain what user-level problem
>> they're trying to solve.
>>
>> E.g. to take just one example
>>
>> > +@item function_return("@var{choice}")
>> > +@cindex @code{function_return} function attribute, x86
>> > +On x86 targets, the @code{function_return} attribute causes the compiler
>> > +to convert function return with @var{choice}.  @samp{keep} keeps function
>> > +return unmodified.  @samp{thunk} converts function return to call and
>> > +return thunk.  @samp{thunk-inline} converts function return to inlined
>> > +call and return thunk.  @samp{thunk-extern} converts function return to
>> > +external call and return thunk provided in a separate object file.
>>
>> Why would you want to mess with call and return code generation in this
>> way?  The documentation doesn't say.
>>
>> For thunk-extern, is the programmer supposed to provide the thunk?  How
>> would you go about implementing the missing bit of code?  What should it
>> do?  I'm compiler implementor and I wouldn't even know how to use this
>> feature by reading the manual; how would an ordinary application
>> programmer who isn't familiar with GCC internals know how to use it?
>>
>> If the goal here is to tell GCC to produce code that is protected
>> against the Spectre vulnerability, perhaps simplify this to adding just
>> one option that controls all the things you've given separate options
>> and attributes for?
>
> Also it would be good to coordinate with the LLVM guys: They currently
> use -mretpoline and -mretpoline_external_thunk.
> I agree with Sandra that a single master option like -mretpoline would
> be better.

Our current goal is to compile Linux kernel.  We won't change the generated
codes.  We will change the command options only if we add a late generic RTL
pass.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 7, 2018 at 8:07 PM, Sandra Loosemore
 wrote:
> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>>
>> This set of patches for GCC 8 mitigates variant #2 of the speculative
>> execution
>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka
>> Spectre.  They
>> convert indirect branches to call and return thunks to avoid speculative
>> execution
>> via indirect call and jmp.
>
>
> I have a general documentation issue with all the new command-line options
> and attributes added by this patch set:  the documentation is very
> implementor-speaky and doesn't explain what user-level problem they're
> trying to solve.

Do you have any suggestions?

> E.g. to take just one example
>
>> +@item function_return("@var{choice}")
>> +@cindex @code{function_return} function attribute, x86
>> +On x86 targets, the @code{function_return} attribute causes the compiler
>> +to convert function return with @var{choice}.  @samp{keep} keeps function
>> +return unmodified.  @samp{thunk} converts function return to call and
>> +return thunk.  @samp{thunk-inline} converts function return to inlined
>> +call and return thunk.  @samp{thunk-extern} converts function return to
>> +external call and return thunk provided in a separate object file.
>
>
> Why would you want to mess with call and return code generation in this way?
> The documentation doesn't say.
>
> For thunk-extern, is the programmer supposed to provide the thunk?  How
> would you go about implementing the missing bit of code?  What should it do?
> I'm compiler implementor and I wouldn't even know how to use this feature by
> reading the manual; how would an ordinary application programmer who isn't
> familiar with GCC internals know how to use it?

This option was requested by Linux kernel people.  Linux kernel may
choose different thunks at kernel load-time.  If a program doesn't know
how to write external thunk, he/she shouldn't it.

> If the goal here is to tell GCC to produce code that is protected against
> the Spectre vulnerability, perhaps simplify this to adding just one option
> that controls all the things you've given separate options and attributes
> for?

-mindirect-branch=thunk does the job.  Other options/choices are for
fine tuning.

Thanks.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Martin Liška]

On 01/08/2018 01:29 AM, H.J. Lu wrote:
> 1. They need to be backportable to GCC 7/6/5/4.x.

I must admit this is very important constrain. To be honest, we're planning
to backport the patchset to GCC 4.3.

Martin

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Markus Trippelsdorf]

On 2018.01.07 at 21:07 -0700, Sandra Loosemore wrote:
> On 01/07/2018 03:58 PM, H.J. Lu wrote:
> > This set of patches for GCC 8 mitigates variant #2 of the speculative 
> > execution
> > vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre. 
> >  They
> > convert indirect branches to call and return thunks to avoid speculative 
> > execution
> > via indirect call and jmp.
> 
> I have a general documentation issue with all the new command-line 
> options and attributes added by this patch set:  the documentation is 
> very implementor-speaky and doesn't explain what user-level problem 
> they're trying to solve.
> 
> E.g. to take just one example
> 
> > +@item function_return("@var{choice}")
> > +@cindex @code{function_return} function attribute, x86
> > +On x86 targets, the @code{function_return} attribute causes the compiler
> > +to convert function return with @var{choice}.  @samp{keep} keeps function
> > +return unmodified.  @samp{thunk} converts function return to call and
> > +return thunk.  @samp{thunk-inline} converts function return to inlined
> > +call and return thunk.  @samp{thunk-extern} converts function return to
> > +external call and return thunk provided in a separate object file.
> 
> Why would you want to mess with call and return code generation in this 
> way?  The documentation doesn't say.
> 
> For thunk-extern, is the programmer supposed to provide the thunk?  How 
> would you go about implementing the missing bit of code?  What should it 
> do?  I'm compiler implementor and I wouldn't even know how to use this 
> feature by reading the manual; how would an ordinary application 
> programmer who isn't familiar with GCC internals know how to use it?
> 
> If the goal here is to tell GCC to produce code that is protected 
> against the Spectre vulnerability, perhaps simplify this to adding just 
> one option that controls all the things you've given separate options 
> and attributes for?

Also it would be good to coordinate with the LLVM guys: They currently
use -mretpoline and -mretpoline_external_thunk. 
I agree with Sandra that a single master option like -mretpoline would
be better.

-- 
Markus

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Sandra Loosemore]

On 01/07/2018 03:58 PM, H.J. Lu wrote:

This set of patches for GCC 8 mitigates variant #2 of the speculative execution
vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.  
They
convert indirect branches to call and return thunks to avoid speculative 
execution
via indirect call and jmp.


I have a general documentation issue with all the new command-line 
options and attributes added by this patch set:  the documentation is 
very implementor-speaky and doesn't explain what user-level problem 
they're trying to solve.


E.g. to take just one example


+@item function_return("@var{choice}")
+@cindex @code{function_return} function attribute, x86
+On x86 targets, the @code{function_return} attribute causes the compiler
+to convert function return with @var{choice}.  @samp{keep} keeps function
+return unmodified.  @samp{thunk} converts function return to call and
+return thunk.  @samp{thunk-inline} converts function return to inlined
+call and return thunk.  @samp{thunk-extern} converts function return to
+external call and return thunk provided in a separate object file.


Why would you want to mess with call and return code generation in this 
way?  The documentation doesn't say.


For thunk-extern, is the programmer supposed to provide the thunk?  How 
would you go about implementing the missing bit of code?  What should it 
do?  I'm compiler implementor and I wouldn't even know how to use this 
feature by reading the manual; how would an ordinary application 
programmer who isn't familiar with GCC internals know how to use it?


If the goal here is to tell GCC to produce code that is protected 
against the Spectre vulnerability, perhaps simplify this to adding just 
one option that controls all the things you've given separate options 
and attributes for?


-Sandra

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[H.J. Lu]

On Sun, Jan 7, 2018 at 3:36 PM, Jeff Law  wrote:
> On 01/07/2018 03:58 PM, H.J. Lu wrote:
>> This set of patches for GCC 8 mitigates variant #2 of the speculative 
>> execution
>> vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.  
>> They
>> convert indirect branches to call and return thunks to avoid speculative 
>> execution
>> via indirect call and jmp.
>>
>>
>> H.J. Lu (5):
>>   x86: Add -mindirect-branch=
>>   x86: Add -mindirect-branch-loop=
>>   x86: Add -mfunction-return=
>>   x86: Add -mindirect-branch-register
>>   x86: Add 'V' register operand modifier
>>
>>  gcc/config/i386/constraints.md |  12 +-
>>  gcc/config/i386/i386-opts.h|  14 +
>>  gcc/config/i386/i386-protos.h  |   2 +
>>  gcc/config/i386/i386.c | 655 
>> -
>>  gcc/config/i386/i386.h |  10 +
>>  gcc/config/i386/i386.md|  51 +-
>>  gcc/config/i386/i386.opt   |  45 ++
>>  gcc/config/i386/predicates.md  |  21 +-
>>  gcc/doc/extend.texi|  22 +
>>  gcc/doc/invoke.texi|  37 +-
> My fundamental problem with this patchkit is that it is 100% x86/x86_64
> specific.
>
> ISTM we want a target independent mechanism (ie, new standard patterns,
> options, etc) then an x86/x86_64 implementation using that target
> independent framework (ie, the actual implementation of those new
> standard patterns).
>

My patch set is implemented with some constraints:

1. They need to be backportable to GCC 7/6/5/4.x.
2. They should work with all compiler optimizations.
3. They need to generate code sequences are x86 specific, which can't be
changed in any shape or form.  And the generated codes are quite opposite
to what a good optimizing compiler should generate.

Given that these conditions, I kept existing indirect call, jump and
return patterns.
I generated different code sequences for these patterns during the final pass
when generating assembly codes.

I guess that I could add a late target independent RTL pass to convert
indirect call, jump and return patterns to something else.  But I am not sure
if that is what you are looking for.

-- 
H.J.

Re: [PATCH 0/5] x86: CVE-2017-5715, aka Spectre

[Jeff Law]

On 01/07/2018 03:58 PM, H.J. Lu wrote:
> This set of patches for GCC 8 mitigates variant #2 of the speculative 
> execution
> vulnerabilities on x86 processors identified by CVE-2017-5715, aka Spectre.  
> They
> convert indirect branches to call and return thunks to avoid speculative 
> execution
> via indirect call and jmp.
> 
> 
> H.J. Lu (5):
>   x86: Add -mindirect-branch=
>   x86: Add -mindirect-branch-loop=
>   x86: Add -mfunction-return=
>   x86: Add -mindirect-branch-register
>   x86: Add 'V' register operand modifier
> 
>  gcc/config/i386/constraints.md |  12 +-
>  gcc/config/i386/i386-opts.h|  14 +
>  gcc/config/i386/i386-protos.h  |   2 +
>  gcc/config/i386/i386.c | 655 
> -
>  gcc/config/i386/i386.h |  10 +
>  gcc/config/i386/i386.md|  51 +-
>  gcc/config/i386/i386.opt   |  45 ++
>  gcc/config/i386/predicates.md  |  21 +-
>  gcc/doc/extend.texi|  22 +
>  gcc/doc/invoke.texi|  37 +-
My fundamental problem with this patchkit is that it is 100% x86/x86_64
specific.

ISTM we want a target independent mechanism (ie, new standard patterns,
options, etc) then an x86/x86_64 implementation using that target
independent framework (ie, the actual implementation of those new
standard patterns).

jeff