This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/7] Mitigation against unsafe data speculation (CVE-2017-5753)


On Mon, Jul 9, 2018 at 6:39 PM Richard Earnshaw
<Richard.Earnshaw@arm.com> wrote:
>
>
> The patches I posted earlier this year for mitigating against
> CVE-2017-5753 (Spectre variant 1) attracted some useful feedback, from
> which it became obvious that a rethink was needed.  This mail, and the
> following patches attempt to address that feedback and present a new
> approach to mitigating against this form of attack surface.
>
> There were two major issues with the original approach:
>
> - The speculation bounds were too tightly constrained - essentially
>   they had to represent and upper and lower bound on a pointer, or a
>   pointer offset.
> - The speculation constraints could only cover the immediately preceding
>   branch, which often did not fit well with the structure of the existing
>   code.
>
> An additional criticism was that the shape of the intrinsic did not
> fit particularly well with systems that used a single speculation
> barrier that essentially had to wait until all preceding speculation
> had to be resolved.
>
> To address all of the above, these patches adopt a new approach, based
> in part on a posting by Chandler Carruth to the LLVM developers list
> (https://lists.llvm.org/pipermail/llvm-dev/2018-March/122085.html),
> but which we have extended to deal with inter-function speculation.
> The patches divide the problem into two halves.
>
> The first half is some target-specific code to track the speculation
> condition through the generated code to provide an internal variable
> which can tell us whether or not the CPU's control flow speculation
> matches the data flow calculations.  The idea is that the internal
> variable starts with the value TRUE and if the CPU's control flow
> speculation ever causes a jump to the wrong block of code the variable
> becomes false until such time as the incorrect control flow
> speculation gets unwound.
>
> The second half is that a new intrinsic function is introduced that is
> much simpler than we had before.  The basic version of the intrinsic
> is now simply:
>
>       T var = __builtin_speculation_safe_value (T unsafe_var);
>
> Full details of the syntax can be found in the documentation patch, in
> patch 1.  In summary, when not speculating the intrinsic returns
> unsafe_var; when speculating then if it can be shown that the
> speculative flow has diverged from the intended control flow then zero
> is returned.  An optional second argument can be used to return an
> alternative value to zero.  The builtin may cause execution to pause
> until the speculation state is resolved.

So a trivial target implementation would be to emit a barrier and then
it would always return unsafe_var and never zero.  What I don't understand
fully is what users should do here, thus what the value of ever returning
"unsafe" is.  Also I wonder why the API is forcing you to single-out a
special value instead of doing

 bool safe = __builtin_speculation_safe_value_p (T unsafe_value);
 if (!safe)
   /* what now? */

I'm only guessing that the correct way to handle "unsafe" is basically

 while (__builtin_speculation_safe_value (val) == 0)
    ;

use val, it's now safe

that is, the return value is only interesting in sofar as to whether it is equal
to val or the special value?

That said, I wonder why we don't hide that details from the user and
provide a predicate instead.

Richard.

> There are seven patches in this set, as follows.
>
> 1) Introduces the new intrinsic __builtin_sepculation_safe_value.
> 2) Adds a basic hard barrier implementation for AArch32 (arm) state.
> 3) Adds a basic hard barrier implementation for AArch64 state.
> 4) Adds a new command-line option -mtrack-speculation (currently a no-op).
> 5) Disables CB[N]Z and TB[N]Z when -mtrack-speculation.
> 6) Adds the new speculation tracking pass for AArch64
> 7) Uses the new speculation tracking pass to generate CSDB-based barrier
>    sequences
>
> I haven't added a speculation-tracking pass for AArch32 at this time.
> It is possible to do this, but would require quite a lot of rework for
> the arm backend due to the limited number of registers that are
> available.
>
> Although patch 6 is AArch64 specific, I'd appreciate a review from
> someone more familiar with the branch edge code than myself.  There
> appear to be a number of tricky issues with more complex edges so I'd
> like a second opinion on that code in case I've missed an important
> case.
>
> R.
>
>
>
> Richard Earnshaw (7):
>   Add __builtin_speculation_safe_value
>   Arm - add speculation_barrier pattern
>   AArch64 - add speculation barrier
>   AArch64 - Add new option -mtrack-speculation
>   AArch64 - disable CB[N]Z TB[N]Z when tracking speculation
>   AArch64 - new pass to add conditional-branch speculation tracking
>   AArch64 - use CSDB based sequences if speculation tracking is enabled
>
>  gcc/builtin-types.def                     |   6 +
>  gcc/builtins.c                            |  57 ++++
>  gcc/builtins.def                          |  20 ++
>  gcc/c-family/c-common.c                   | 143 +++++++++
>  gcc/c-family/c-cppbuiltin.c               |   5 +-
>  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              |  88 +++++-
>  gcc/config/aarch64/aarch64.md             | 140 ++++++++-
>  gcc/config/aarch64/aarch64.opt            |   4 +
>  gcc/config/aarch64/iterators.md           |   3 +
>  gcc/config/aarch64/t-aarch64              |  10 +
>  gcc/config/arm/arm.md                     |  21 ++
>  gcc/config/arm/unspecs.md                 |   1 +
>  gcc/doc/cpp.texi                          |   4 +
>  gcc/doc/extend.texi                       |  29 ++
>  gcc/doc/invoke.texi                       |  10 +-
>  gcc/doc/md.texi                           |  15 +
>  gcc/doc/tm.texi                           |  20 ++
>  gcc/doc/tm.texi.in                        |   2 +
>  gcc/target.def                            |  23 ++
>  gcc/targhooks.c                           |  27 ++
>  gcc/targhooks.h                           |   2 +
>  gcc/testsuite/gcc.dg/spec-barrier-1.c     |  40 +++
>  gcc/testsuite/gcc.dg/spec-barrier-2.c     |  19 ++
>  gcc/testsuite/gcc.dg/spec-barrier-3.c     |  13 +
>  28 files changed, 1191 insertions(+), 11 deletions(-)
>  create mode 100644 gcc/config/aarch64/aarch64-speculation.cc
>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-3.c


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]