Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking

[Jeff Law]

On 07/23/2018 08:33 AM, Richard Earnshaw (lists) wrote:
> [sorry, missed this mail somehow]
> 
> On 11/07/18 22:01, Jeff Law wrote:
>> On 07/09/2018 10:38 AM, Richard Earnshaw wrote:
>>> This patch is the main part of the speculation tracking code.  It adds
>>> a new target-specific pass that is run just before the final branch
>>> reorg pass (so that it can clean up any new edge insertions we make).
>>> The pass is only run with -mtrack-speculation is passed on the command
>>> line.
>>>
>>> One thing that did come to light as part of this was that the stack pointer
>>> register was not being permitted in comparision instructions.  We rely on
>>> that for moving the tracking state between SP and the scratch register at
>>> function call boundaries.
>> Note that the sp in comparison instructions issue came up with the
>> improvements to stack-clash that Tamar, Richard S. and you worked on.
>>
> 
> I can certainly lift that part into a separate patch.
Your call.  It was mostly an observation that the change was clearly
needed elsewhere.  I'm certainly comfortable letting that hunk go in
with whichever kit is approved first :-)

> 
>>
>>>
>>> * config/aarch64/aarch64-speculation.cc: New file.
>>> * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before
>>> pass_reorder_blocks.
>>> * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add
>>> prototype.
>>> * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix
>>> X14 and X15 when tracking speculation.
>>> * config/aarch64/aarch64.md (register name constants): Add
>>> SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM.
>>> (unspec): Add UNSPEC_SPECULATION_TRACKER.
>>> (speculation_barrier): New insn attribute.
>>> (cmp): Allow SP in comparisons.
>>> (speculation_tracker): New insn.
>>> (speculation_barrier): Add speculation_barrier attribute.
>>> * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o.
>>> * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs.
>>> * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation.
>>> ---
>>>  gcc/config.gcc|   2 +-
>>>  gcc/config/aarch64/aarch64-passes.def |   1 +
>>>  gcc/config/aarch64/aarch64-protos.h   |   3 +-
>>>  gcc/config/aarch64/aarch64-speculation.cc | 494 
>>> ++
>>>  gcc/config/aarch64/aarch64.c  |  13 +
>>>  gcc/config/aarch64/aarch64.md |  30 +-
>>>  gcc/config/aarch64/t-aarch64  |  10 +
>>>  gcc/doc/invoke.texi   |  10 +-
>>>  8 files changed, 558 insertions(+), 5 deletions(-)
>>>  create mode 100644 gcc/config/aarch64/aarch64-speculation.cc
>> Given the consensus forming about using these kind of masking
>> instructions being the preferred way to mitigate (as opposed to lfence
>> barriers and the like) I have to ask your opinions about making the bulk
>> of this a general pass rather than one specific to the aarch backend.
>> I'd hate to end up duplicating all this stuff across multiple architectures.
>>
>> I think it all looks pretty reasonable though.
>>
>> jeff
>>
> 
> 
> It would be nice to make this more generic, but I'm not sure how easy
> that would be.  Some of the analysis is surely the same, but deployment
> of the mitigation itself is perhaps more complex.  At this point in
> time, I think I'd prefer to go with the target-specific implementation
> and then look to generalize it as a follow-up.  There may be some more
> optimizations to add later as well.
ACK.  I suspect it's mostly the analysis side that we'll want to share.
I don't mind giving you the advantage of going first and letting it live
in the aarch64 backend.  Second implementation can extract the analysis
bits :-)

So IMHO, this can go forward whenever you want to push it.

Jeff

Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking

[Richard Earnshaw (lists)]

[sorry, missed this mail somehow]

On 11/07/18 22:01, Jeff Law wrote:
> On 07/09/2018 10:38 AM, Richard Earnshaw wrote:
>> This patch is the main part of the speculation tracking code.  It adds
>> a new target-specific pass that is run just before the final branch
>> reorg pass (so that it can clean up any new edge insertions we make).
>> The pass is only run with -mtrack-speculation is passed on the command
>> line.
>>
>> One thing that did come to light as part of this was that the stack pointer
>> register was not being permitted in comparision instructions.  We rely on
>> that for moving the tracking state between SP and the scratch register at
>> function call boundaries.
> Note that the sp in comparison instructions issue came up with the
> improvements to stack-clash that Tamar, Richard S. and you worked on.
> 

I can certainly lift that part into a separate patch.

> 
>>
>>  * config/aarch64/aarch64-speculation.cc: New file.
>>  * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before
>>  pass_reorder_blocks.
>>  * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add
>>  prototype.
>>  * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix
>>  X14 and X15 when tracking speculation.
>>  * config/aarch64/aarch64.md (register name constants): Add
>>  SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM.
>>  (unspec): Add UNSPEC_SPECULATION_TRACKER.
>>  (speculation_barrier): New insn attribute.
>>  (cmp): Allow SP in comparisons.
>>  (speculation_tracker): New insn.
>>  (speculation_barrier): Add speculation_barrier attribute.
>>  * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o.
>>  * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs.
>>  * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation.
>> ---
>>  gcc/config.gcc|   2 +-
>>  gcc/config/aarch64/aarch64-passes.def |   1 +
>>  gcc/config/aarch64/aarch64-protos.h   |   3 +-
>>  gcc/config/aarch64/aarch64-speculation.cc | 494 
>> ++
>>  gcc/config/aarch64/aarch64.c  |  13 +
>>  gcc/config/aarch64/aarch64.md |  30 +-
>>  gcc/config/aarch64/t-aarch64  |  10 +
>>  gcc/doc/invoke.texi   |  10 +-
>>  8 files changed, 558 insertions(+), 5 deletions(-)
>>  create mode 100644 gcc/config/aarch64/aarch64-speculation.cc
> Given the consensus forming about using these kind of masking
> instructions being the preferred way to mitigate (as opposed to lfence
> barriers and the like) I have to ask your opinions about making the bulk
> of this a general pass rather than one specific to the aarch backend.
> I'd hate to end up duplicating all this stuff across multiple architectures.
> 
> I think it all looks pretty reasonable though.
> 
> jeff
> 


It would be nice to make this more generic, but I'm not sure how easy
that would be.  Some of the analysis is surely the same, but deployment
of the mitigation itself is perhaps more complex.  At this point in
time, I think I'd prefer to go with the target-specific implementation
and then look to generalize it as a follow-up.  There may be some more
optimizations to add later as well.

R.

Re: [PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking

[Jeff Law]

On 07/09/2018 10:38 AM, Richard Earnshaw wrote:
> This patch is the main part of the speculation tracking code.  It adds
> a new target-specific pass that is run just before the final branch
> reorg pass (so that it can clean up any new edge insertions we make).
> The pass is only run with -mtrack-speculation is passed on the command
> line.
> 
> One thing that did come to light as part of this was that the stack pointer
> register was not being permitted in comparision instructions.  We rely on
> that for moving the tracking state between SP and the scratch register at
> function call boundaries.
Note that the sp in comparison instructions issue came up with the
improvements to stack-clash that Tamar, Richard S. and you worked on.


> 
>   * config/aarch64/aarch64-speculation.cc: New file.
>   * config/aarch64/aarch64-passes.def (pass_track_speculation): Add before
>   pass_reorder_blocks.
>   * config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add
>   prototype.
>   * config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix
>   X14 and X15 when tracking speculation.
>   * config/aarch64/aarch64.md (register name constants): Add
>   SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM.
>   (unspec): Add UNSPEC_SPECULATION_TRACKER.
>   (speculation_barrier): New insn attribute.
>   (cmp): Allow SP in comparisons.
>   (speculation_tracker): New insn.
>   (speculation_barrier): Add speculation_barrier attribute.
>   * config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o.
>   * config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs.
>   * doc/invoke.texi (AArch64 Options): Document -mtrack-speculation.
> ---
>  gcc/config.gcc|   2 +-
>  gcc/config/aarch64/aarch64-passes.def |   1 +
>  gcc/config/aarch64/aarch64-protos.h   |   3 +-
>  gcc/config/aarch64/aarch64-speculation.cc | 494 
> ++
>  gcc/config/aarch64/aarch64.c  |  13 +
>  gcc/config/aarch64/aarch64.md |  30 +-
>  gcc/config/aarch64/t-aarch64  |  10 +
>  gcc/doc/invoke.texi   |  10 +-
>  8 files changed, 558 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/config/aarch64/aarch64-speculation.cc
Given the consensus forming about using these kind of masking
instructions being the preferred way to mitigate (as opposed to lfence
barriers and the like) I have to ask your opinions about making the bulk
of this a general pass rather than one specific to the aarch backend.
I'd hate to end up duplicating all this stuff across multiple architectures.

I think it all looks pretty reasonable though.

jeff

[PATCH 6/7] AArch64 - new pass to add conditional-branch speculation tracking

[Richard Earnshaw]

This patch is the main part of the speculation tracking code.  It adds
a new target-specific pass that is run just before the final branch
reorg pass (so that it can clean up any new edge insertions we make).
The pass is only run with -mtrack-speculation is passed on the command
line.

One thing that did come to light as part of this was that the stack pointer
register was not being permitted in comparision instructions.  We rely on
that for moving the tracking state between SP and the scratch register at
function call boundaries.

* config/aarch64/aarch64-speculation.cc: New file.
* config/aarch64/aarch64-passes.def (pass_track_speculation): Add before
pass_reorder_blocks.
* config/aarch64/aarch64-protos.h (make_pass_track_speculation): Add
prototype.
* config/aarch64/aarch64.c (aarch64_conditional_register_usage): Fix
X14 and X15 when tracking speculation.
* config/aarch64/aarch64.md (register name constants): Add
SPECULATION_TRACKER_REGNUM and SPECULATION_SCRATCH_REGNUM.
(unspec): Add UNSPEC_SPECULATION_TRACKER.
(speculation_barrier): New insn attribute.
(cmp): Allow SP in comparisons.
(speculation_tracker): New insn.
(speculation_barrier): Add speculation_barrier attribute.
* config/aarch64/t-aarch64: Add make rule for aarch64-speculation.o.
* config.gcc (aarch64*-*-*): Add aarch64-speculation.o to extra_objs.
* doc/invoke.texi (AArch64 Options): Document -mtrack-speculation.
---
 gcc/config.gcc|   2 +-
 gcc/config/aarch64/aarch64-passes.def |   1 +
 gcc/config/aarch64/aarch64-protos.h   |   3 +-
 gcc/config/aarch64/aarch64-speculation.cc | 494 ++
 gcc/config/aarch64/aarch64.c  |  13 +
 gcc/config/aarch64/aarch64.md |  30 +-
 gcc/config/aarch64/t-aarch64  |  10 +
 gcc/doc/invoke.texi   |  10 +-
 8 files changed, 558 insertions(+), 5 deletions(-)
 create mode 100644 gcc/config/aarch64/aarch64-speculation.cc

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 78e84c2..b17fdba 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
 	extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
 	c_target_objs="aarch64-c.o"
 	cxx_target_objs="aarch64-c.o"
-	extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+	extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o aarch64-speculation.o"
 	target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
 	target_has_targetm_common=yes
 	;;
diff --git a/gcc/config/aarch64/aarch64-passes.def b/gcc/config/aarch64/aarch64-passes.def
index 87747b4..3d6a254 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
.  */
 
 INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);
+INSERT_PASS_BEFORE (pass_reorder_blocks, 1, pass_track_speculation);
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index bc11a78..e80ffcf 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -554,7 +554,8 @@ enum aarch64_parse_opt_result aarch64_parse_extension (const char *,
 std::string aarch64_get_extension_string_for_isa_flags (unsigned long,
 			unsigned long);
 
-rtl_opt_pass *make_pass_fma_steering (gcc::context *ctxt);
+rtl_opt_pass *make_pass_fma_steering (gcc::context *);
+rtl_opt_pass *make_pass_track_speculation (gcc::context *);
 
 poly_uint64 aarch64_regmode_natural_size (machine_mode);
 
diff --git a/gcc/config/aarch64/aarch64-speculation.cc b/gcc/config/aarch64/aarch64-speculation.cc
new file mode 100644
index 000..2dd06ae
--- /dev/null
+++ b/gcc/config/aarch64/aarch64-speculation.cc
@@ -0,0 +1,494 @@
+/* Speculation tracking and mitigation (e.g. CVE 2017-5753) for AArch64.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   Contributed by ARM Ltd.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   GCC is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GCC; see the file COPYING3.  If not see
+   .  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "target.h"
+#include "rtl.h"
+#include "tree-pass.h"
+#include "profile-count.h"
+#include "cfg.h"
+#include "cfgbuild.h"
+#include "print-rtl.h"
+#include "cfgrtl.h"
+#include "function.h"
+#include