RE: 0005-Part-5.-Add-x86-CET-documentation
Updated version #3.
> -Original Message-
> From: Sandra Loosemore [mailto:san...@codesourcery.com]
> Sent: Wednesday, September 27, 2017 5:41 AM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; Uros Bizjak
> <ubiz...@gmail.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0005-Part-5.-Add-x86-CET-documentation
>
> On 09/26/2017 07:47 AM, Tsimbalist, Igor V wrote:
> > Here is a new version of the patch.
> >
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > a374890..a900ed1 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -5655,6 +5655,13 @@ compiled with the
> > @option{-fcf-protection=branch} option. The compiler assumes that
> > the function's address is a valid target for a control-flow transfer.
> >
> > +@emph{x86 implementation:} when @option{-fcf-protection} option is
> > +specified the compiler inserts an ENDBR instruction at function's
> > +prologue if the function's type does not have the @code{nocf_check}
> > +attribute and addresses to which indirect control-flow transfer can
> > +happen. The instruction triggers the HW check if a control-flow
> > +transfer to the address of ENDBR instruction is valid.
>
> Implementation details like this should be comments in the code, not
> included in the user-facing documentation.
>
> > @@ -5662,7 +5669,8 @@ not be instrumented when compiled with the
> that
> > the function's address from the pointer is a valid target for a
> > control-flow transfer. A direct function call through a function
> > name is assumed to be a safe call thus direct calls are not
> > -instrumented by the compiler.
> > +instrumented by the compiler. For @emph{x86 implementation} the
> > +compiler inserts a NOTRACK prefix before an indirect call instruction.
>
> Likewise here.
For this comment and above could you please let me know what is the right place
To move the description? Also I enclosed ENDBR and NOTRACK in @code{} and
wrote it in lower case.
> > @@ -21217,6 +21225,25 @@ void __builtin_ia32_wrpkru (unsigned int)
> > unsigned int __builtin_ia32_rdpkru () @end smallexample
> >
> > +The following built-in functions are available when @option{-mcet} is
> used.
> > +They are used to support Intel Control-flow Enforcment Technology (CET).
> > +Each built-in function generate a machine instruction that is part of
> > +the
>
> s/generate a/generates the/
Fixed.
> > @@ -11378,6 +11379,20 @@ You can also use the @code{nocf_check}
> > attribute to identify which functions and calls should be skipped
> > from instrumentation (@pxref{Function Attributes}).
> >
> > +Currently x86 GNU/Linux target provides an implementation based on
>
> s/x86/the x86/
Fixed.
> > +Intel Control-flow Enforcement Technology (CET), thus @option{-mcet}
>
> s/@option/the @option/
Fixed.
> > +option is required to enable this feature.
>
> I think you should put a cross-reference to the x86 options node here, and
> move all the following x86-specific discussion to that section.
Put cross-reference.
> > In order to get an
> > +application to be CET compatible the x86 implementation requires all
> > +object files have to be compiled with @option{-fcf-protection} option
> > +and all linked in libraries have to be CET compatible.
>
> I'm having difficulty parsing this. What does "CET compatible" mean?
> Is this an ABI compatibility issue, so that all objects linked into the
> executable
> have to be compiled with the (same?) @option{-fcf-protection} option if any
> of them do? Or do you just lose checking on code in uninstrumented
> objects?
I re-wrote the paragraph and removed "compatibility topic".
> > +Instrumentation for x86 is controlled by target specific options
>
> hyphenate target-specific here
Fixed.
> > +@option{-mcet}, @option{-mibt} and @option{-mshstk}. The compiler
> > +also provides a number of built-in functions for fine-grained control
> > +of CET-based implementation. See @xref{x86 Built-in Functions}, for
> > +more information.
> > +
> > @item -fstack-protector
> > @opindex fstack-protector
> > Emit extra code to check for buffer overflows, such as stack smashing
> > @@ -25755,15 +25770,19 @@ preferred alignment to @option{-
> mpreferred-stack-boundary=2}.
> > @need 200
> > @itemx -mclzero
> > @opindex mclzero
> > +@need 200
> > @itemx -mpku
> > @opindex mpku
> > +@need 200
> > +@itemx -mcet
> > +@opindex mcet
> > These switches enable the use of instructions in the MMX, SSE, SSE2,
> >
RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
Updated version #4.
> -Original Message-
> From: Sandra Loosemore [mailto:san...@codesourcery.com]
> Sent: Wednesday, September 27, 2017 5:11 AM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Cc: Jeff Law <l...@redhat.com>
> Subject: Re: 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> attribute
>
> On 09/26/2017 07:45 AM, Tsimbalist, Igor V wrote:
> > Here is the updated version (version#3). All comments below are fixed.
>
> This still needs more work. Specific comments below:
>
> > +The @code{nocf_check} attribute is applied to an object's type.
> > +In case of assignment of a function address or a function pointer to
> > +another pointer, the attribute is not carried over from the
> > +right-hand object's type, the type of left-hand object stays
> > +unchanged. The
>
> s/object's type,/object's type;/
Fixed.
> > @@ -11348,6 +11349,35 @@ is used to link a program, the GCC driver
> > automatically links against @file{libmpxwrappers}. See also @option{-
> static-libmpxwrappers}.
> > Enabled by default.
> >
> > +@item -fcf-
> protection==@r{[}full@r{|}branch@r{|}return@r{|}none@r{]}
> > +@opindex fcf-protection
> > +Enable code instrumentation of control-flow transfers to increase
> > +program security by checking that target addresses of control-flow
> > +transfer instructions (such as indirect function call, function
> > +return, indirect jump) are valid. This prevents diverting the
> > +control flow instructions from its original target address to a new
> > +undesigned
>
> s/control flow instructions/control-flow instructions/
>
> I'd rewrite the next sentence as
>
> This prevents diverting the flow of control to an unexpected target.
I used your suggestion.
> > +target. This is intended to protect against such threats as
> > +Return-oriented Programming (ROP), and similarly call/jmp-oriented
> > +programming (COP/JOP).
> > +
> > +Each compiler target, which is going to support the control-flow
> > +instrumentation, is supposed to have its own target specific
> > +implementation. For all targets where an implementation is absent the
> > +usage of @option{-fcf-protection} option causes an error message.
>
> I would really prefer that you list the targets this works on here instead.
Another patch you are reviewing now (its name starts with 0005-Part-5)
has the statement you would like to put here. The important point here is
an error issuing. When I commit the first patch none of target platforms
supports
the option and an error is printed when the option is specified. I removed the
first sentence but keep the second one:
For all targets, which do not support the @option{-fcf-protection}
option, the option usage results in an error message.
> > +The value @code{branch} tells the compiler to implement checking of
> > +validity of control-flow transfer at the point of indirect branch
> > +instructions, i.e. call/jmp instructions. The value @code{return}
> > +implements checking of validity at the point of returning from a
> > +function. The value @code{full} is an alias for specifying both
> > +@code{branch} and @code{return}. The value @code{none} turns off
> > +instrumentation. This value may be used for future architectures
> > +where @option{-fcf-protection} option is switched on by default.
>
> I don't think we need to document GCC's future behavior for future
> architectures (I'm always going around removing useless discussion from
> 20 years ago of possible extensions that never got implemented). I assume
> that this is just provided for completeness and to override a previous -fcf-
> protection option on the command line.
Ok, removed the last sentence.
> > +You can also use the @code{nocf_check} attribute to identify which
> > +functions and calls should be skipped from instrumentation
> > +(@pxref{Function Attributes}).
> > +
> > @item -fstack-protector
> > @opindex fstack-protector
> > Emit extra code to check for buffer overflows, such as stack smashing
> > diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi index
> > 12355c2..b4fc5f3 100644
> > --- a/gcc/doc/rtl.texi
> > +++ b/gcc/doc/rtl.texi
> > @@ -4040,6 +4040,22 @@ is used in place of the actual insn pattern.
> > This is done in cases where the pattern is either complex or misleading.
> > @end table
> >
> > +The note @code{REG_CALL_NOCF_CHECK} is used in conjunction with
> the
> > +@option{-fcf-protection=branch} option. The note is set if a
> > +@code{nocf_check} attribute is specified for a funct
RE: 0005-Part-5.-Add-x86-CET-documentation
> -Original Message-
> From: Florian Weimer [mailto:fwei...@redhat.com]
> Sent: Wednesday, September 27, 2017 10:52 AM
> To: Sandra Loosemore <san...@codesourcery.com>; Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com>; Uros Bizjak <ubiz...@gmail.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0005-Part-5.-Add-x86-CET-documentation
>
> On 09/27/2017 05:40 AM, Sandra Loosemore wrote:
> >>
> >> +@emph{x86 implementation:} when @option{-fcf-protection} option is
> >> +specified the compiler inserts an ENDBR instruction at function's
> >> +prologue if the function's type does not have the @code{nocf_check}
> >> +attribute and addresses to which indirect control-flow transfer can
> >> +happen. The instruction triggers the HW check if a control-flow
> >> +transfer to the address of ENDBR instruction is valid.
> >
> > Implementation details like this should be comments in the code, not
> > included in the user-facing documentation.
>
> This is part of the ABI GCC implements, so it has to be documented
> somewhere, and not just as part of the GCC source code.
A question for both Sandra and Florian - What is your suggestion where the text
should go?
> CET is not properly described in the ABI supplement and I don't think this
> will
> change, so detailed documentation in the GCC manual is very much
> desirable.
>
> That being said, the implementation notes above need some clarification.
> It's not clear to me what the conditions are under which the ENDBR
> instruction is emitted (and we probably should use @code{endbr} in the
> manual), what it is trying to achieve, and how the x86 calling convention
> changes. I assume it is somehow related to what we call internally “the
> suffix
We are diving into implementation details but it's simple enough.
- endbr is generated for every function, which does not have nocf_check
attribute.
Optimization can be done later to exclude functions, whose address was not
taken.
- there is no change in calling convention
Thanks,
Igor
> problem”: without control flow integrity, an attacker might skip over
> precondition/hardening checks, directly to the critical changes we want to
> protect, executing only the suffix of a function (hence the name).
>
> Thanks,
> Florian
RE: 0005-Part-5.-Add-x86-CET-documentation
Here is a new version of the patch. Igor > -Original Message- > From: Sandra Loosemore [mailto:san...@codesourcery.com] > Sent: Monday, September 25, 2017 5:43 AM > To: Uros Bizjak <ubiz...@gmail.com>; Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0005-Part-5.-Add-x86-CET-documentation > > On 09/20/2017 08:13 AM, Uros Bizjak wrote: > > On Wed, Sep 20, 2017 at 11:20 AM, Tsimbalist, Igor V > > <igor.v.tsimbal...@intel.com> wrote: > >> Uros, could you please review this patch as it's a part of x86 specific > changes you have reviewed already. > > > > Please proofread and spell-check the text. There are grammatical errors, > e.g.: > > [snip] > > If/when there is a newer version of this patch, cc it to me and I will also > make > a pass through it. > > -Sandra 0005-Part-5.-Add-x86-CET-documentation.patch Description: 0005-Part-5.-Add-x86-CET-documentation.patch
RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
Here is the updated version (version#3). All comments below are fixed.
Igor
> -Original Message-
> From: Tsimbalist, Igor V
> Sent: Monday, September 25, 2017 11:57 PM
> To: Sandra Loosemore <san...@codesourcery.com>; 'gcc-
> patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Cc: Jeff Law <l...@redhat.com>; Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com>
> Subject: RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> attribute
>
> > -Original Message-
> > From: Sandra Loosemore [mailto:san...@codesourcery.com]
> > Sent: Monday, September 25, 2017 5:07 AM
> > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> > Cc: Jeff Law <l...@redhat.com>
> > Subject: Re:
> > 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> > attribute
> >
> > On 09/19/2017 07:45 AM, Tsimbalist, Igor V wrote:
> > > Here is an updated patch (version #2). Mainly attribute and option
> > > names
> > were changed.
> > >
> > > gcc/doc/
> > > * extend.texi: Add 'nocf_check' documentation.
> > > * gimple.texi: Add second parameter to
> > gimple_build_call_from_tree.
> > > * invoke.texi: Add -fcf-protection documentation.
> > > * rtl.texi: Add REG_CALL_NOTRACK documenation.
> > >
> > > Is it ok for trunk?
> > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > > cd5733e..6bdb183 100644
> > > --- a/gcc/doc/extend.texi
> > > +++ b/gcc/doc/extend.texi
> > > @@ -5646,6 +5646,56 @@ Specify which floating-point unit to use.
> > > You must specify the @code{target("fpmath=sse,387")} option as
> > > @code{target("fpmath=sse+387")} because the comma would separate
> > > different options.
> > > +
> > > +@item nocf_check
> > > +@cindex @code{nocf_check} function attribute The
> @code{nocf_check}
> > > +attribute on a function is used to inform the compiler that the
> > > +function's prolog should not be instrumented when
> >
> > s/prolog/prologue/
>
> Fixed.
>
> > > +compiled with the @option{-fcf-protection=branch} option. The
> > > +compiler assumes that the function's address is a valid target for
> > > +a control-flow transfer.
> > > +
> > > +The @code{nocf_check} attribute on a type of pointer to function is
> > > +used to inform the compiler that a call through the pointer should
> > > +not be instrumented when compiled with the
> > > +@option{-fcf-protection=branch} option. The compiler assumes that
> > > +the function's address from the pointer is a valid target for a
> > > +control-flow transfer. A direct function call through a function
> > > +name is assumed as a safe call thus direct calls will not be
> >
> > ...is assumed to be a safe call, thus direct calls are not...
>
> Fixed.
>
> > > +instrumented by the compiler.
> > > +
> > > +The @code{nocf_check} attribute is applied to an object's type. A
> > > +The @code{nocf_check} attribute is transfered to a call instruction
> > > +at the GIMPLE and RTL translation phases. The attribute is not
> > > +propagated through assignment, store and load.
> >
> > extend.texi is user-facing documentation, but the second sentence here
> > is implementor-speak and not meaningful to users of GCC. I don't
> > understand what the third sentence is trying to say.
>
> The second sentence is removed. The third sentence is re-written as
>
> In case of assignment of a function address or a function pointer to another
> pointer, the attribute is not carried over from the right-hand object's type,
> the type of left-hand object stays unchanged. The compiler checks for
> @code{nocf_check} attribute mismatch and reports a warning in case of
> mismatch.
>
> > > +
> > > +@smallexample
> > > +@{
> > > +int foo (void) __attribute__(nocf_check); void (*foo1)(void)
> > > +__attribute__(nocf_check); void (*foo2)(void);
> > > +
> > > +int
> > > +foo (void) /* The function's address is assumed as valid. */
> >
> > s/as valid/to be valid/
>
> Fixed.
>
> > > +
> > > + /* This call site is not checked for control-flow validness. */
> >
> > s/validness/validity/g
>
> Fixed.
>
> > > + (*foo1)();
> > > +
> > > + foo1 = foo2;
> > > + /* This call site is still not checked for control-flow validness.
RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
> -Original Message-
> From: Sandra Loosemore [mailto:san...@codesourcery.com]
> Sent: Monday, September 25, 2017 5:07 AM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Cc: Jeff Law <l...@redhat.com>
> Subject: Re: 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> attribute
>
> On 09/19/2017 07:45 AM, Tsimbalist, Igor V wrote:
> > Here is an updated patch (version #2). Mainly attribute and option names
> were changed.
> >
> > gcc/doc/
> > * extend.texi: Add 'nocf_check' documentation.
> > * gimple.texi: Add second parameter to
> gimple_build_call_from_tree.
> > * invoke.texi: Add -fcf-protection documentation.
> > * rtl.texi: Add REG_CALL_NOTRACK documenation.
> >
> > Is it ok for trunk?
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > cd5733e..6bdb183 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -5646,6 +5646,56 @@ Specify which floating-point unit to use. You
> > must specify the @code{target("fpmath=sse,387")} option as
> > @code{target("fpmath=sse+387")} because the comma would separate
> > different options.
> > +
> > +@item nocf_check
> > +@cindex @code{nocf_check} function attribute The @code{nocf_check}
> > +attribute on a function is used to inform the compiler that the
> > +function's prolog should not be instrumented when
>
> s/prolog/prologue/
Fixed.
> > +compiled with the @option{-fcf-protection=branch} option. The
> > +compiler assumes that the function's address is a valid target for a
> > +control-flow transfer.
> > +
> > +The @code{nocf_check} attribute on a type of pointer to function is
> > +used to inform the compiler that a call through the pointer should
> > +not be instrumented when compiled with the
> > +@option{-fcf-protection=branch} option. The compiler assumes that
> > +the function's address from the pointer is a valid target for a
> > +control-flow transfer. A direct function call through a function
> > +name is assumed as a safe call thus direct calls will not be
>
> ...is assumed to be a safe call, thus direct calls are not...
Fixed.
> > +instrumented by the compiler.
> > +
> > +The @code{nocf_check} attribute is applied to an object's type. A
> > +The @code{nocf_check} attribute is transfered to a call instruction
> > +at the GIMPLE and RTL translation phases. The attribute is not
> > +propagated through assignment, store and load.
>
> extend.texi is user-facing documentation, but the second sentence here is
> implementor-speak and not meaningful to users of GCC. I don't understand
> what the third sentence is trying to say.
The second sentence is removed. The third sentence is re-written as
In case of assignment of a function address or a function pointer to
another pointer, the attribute is not carried over from the right-hand
object's type, the type of left-hand object stays unchanged. The
compiler checks for @code{nocf_check} attribute mismatch and reports
a warning in case of mismatch.
> > +
> > +@smallexample
> > +@{
> > +int foo (void) __attribute__(nocf_check); void (*foo1)(void)
> > +__attribute__(nocf_check); void (*foo2)(void);
> > +
> > +int
> > +foo (void) /* The function's address is assumed as valid. */
>
> s/as valid/to be valid/
Fixed.
> > +
> > + /* This call site is not checked for control-flow validness. */
>
> s/validness/validity/g
Fixed.
> > + (*foo1)();
> > +
> > + foo1 = foo2;
> > + /* This call site is still not checked for control-flow validness.
> > + */ (*foo1)();
> > +
> > + /* This call site is checked for control-flow validness. */
> > + (*foo2)();
> > +
> > + foo2 = foo1;
> > + /* This call site is still checked for control-flow validness. */
> > + (*foo2)();
> > +
> > + return 0;
> > +@}
> > +@end smallexample
> > +
> > @end table
> >
> > On the x86, the inliner does not inline a diff --git
> > a/gcc/doc/gimple.texi b/gcc/doc/gimple.texi index 635abd3..b6d9149
> > 100644
> > --- a/gcc/doc/gimple.texi
> > +++ b/gcc/doc/gimple.texi
> > @@ -1310,9 +1310,11 @@ operand is validated with
> @code{is_gimple_operand}).
> > @end deftypefn
> >
> >
> > -@deftypefn {GIMPLE function} gcall *gimple_build_call_from_tree (tree
> > call_expr) -Build a @code{GIMPLE_CALL} from a @code{CALL_EXPR} node.
> > The arguments and the -function are taken from the expression
> > dire
RE: 0005-Part-5.-Add-x86-CET-documentation
Uros, could you please review this patch as it's a part of x86 specific changes you have reviewed already. Thanks, Igor > -Original Message- > From: Tsimbalist, Igor V > Sent: Tuesday, August 1, 2017 10:57 AM > To: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > Subject: 0005-Part-5.-Add-x86-CET-documentation > > Part#5. Add x86 CET documentation.
RE: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Uros Bizjak
> Sent: Tuesday, September 19, 2017 6:13 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
>
> On Tue, Sep 19, 2017 at 5:18 PM, Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com> wrote:
> >> -Original Message-
> >> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> >> ow...@gcc.gnu.org] On Behalf Of Uros Bizjak
> >> Sent: Monday, September 18, 2017 12:17 PM
> >> To: gcc-patches@gcc.gnu.org
> >> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; Tsimbalist,
> >> Igor V <igor.v.tsimbal...@intel.com>
> >> Subject: Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> >>
> >> Hello!
> >>
> >> > gcc/testsuite/
> >> >
> >> > * g++.dg/cet-notrack-1.C: New test.
> >> > * gcc.target/i386/cet-intrin-1.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-10.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-2.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-3.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-4.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-5.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-6.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-7.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-8.c: Likewise.
> >> > * gcc.target/i386/cet-intrin-9.c: Likewise.
> >> > * gcc.target/i386/cet-label.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-1a.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-1b.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-2a.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-2b.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-3.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-4a.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-4b.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-5a.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-5b.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-6a.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-6b.c: Likewise.
> >> > * gcc.target/i386/cet-notrack-7.c: Likewise.
> >> > * gcc.target/i386/cet-property-1.c: Likewise.
> >> > * gcc.target/i386/cet-property-2.c: Likewise.
> >> > * gcc.target/i386/cet-rdssp-1.c: Likewise.
> >> > * gcc.target/i386/cet-sjlj-1.c: Likewise.
> >> > * gcc.target/i386/cet-sjlj-2.c: Likewise.
> >> > * gcc.target/i386/cet-sjlj-3.c: Likewise.
> >> > * gcc.target/i386/cet-switch-1.c: Likewise.
> >> > * gcc.target/i386/cet-switch-2.c: Likewise.
> >> > * lib/target-supports.exp (check_effective_target_cet): New proc.
> >>
> >> A couple of questions:
> >>
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -mcet" } */
> >> +/* { dg-final { scan-assembler-times "setssbsy" 2 } } */
> >> +
> >> +#include
> >> +
> >> +void f1 (void)
> >> +{
> >> + __builtin_ia32_setssbsy ();
> >> +}
> >> +
> >> +void f2 (void)
> >> +{
> >> + _setssbsy ();
> >> +}
> >>
> >> Is there a reason that both, __builtin and intrinsic versions are
> >> tested in a couple of places? The intrinsic version is just a wrapper
> >> for __builtin, so IMO testing intrinsic version should be enough.
> > No strong reason. Just to check that intrinsic names are recognized and
> processed correctly.
> > The implementation could change and the test will catch inconsistency.
> > I would also assume a user will use intrinsics that's why I add intrinsic
> > check.
> Should I remove it?
>
> Actually, these __builtins are considered as implementation detail, and their
> use should be discouraged. They are deliberately not documented, and users
> should use intrinsic headers instead. That said, builtins won't change without
> a reason, since Ada needs them.
>
> It can happen that the test fails due to change of intrinsics, so I'd
> recommend
> to remove them.
Ok, I will remove intrinsic.
> >> diff --git a/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> >> b/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> >> new file mode 100644
> >> index 000..f9223a5
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/i386/cet-rd
RE: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Uros Bizjak
> Sent: Monday, September 18, 2017 12:17 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com>
> Subject: Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
>
> Hello!
>
> > gcc/testsuite/
> >
> > * g++.dg/cet-notrack-1.C: New test.
> > * gcc.target/i386/cet-intrin-1.c: Likewise.
> > * gcc.target/i386/cet-intrin-10.c: Likewise.
> > * gcc.target/i386/cet-intrin-2.c: Likewise.
> > * gcc.target/i386/cet-intrin-3.c: Likewise.
> > * gcc.target/i386/cet-intrin-4.c: Likewise.
> > * gcc.target/i386/cet-intrin-5.c: Likewise.
> > * gcc.target/i386/cet-intrin-6.c: Likewise.
> > * gcc.target/i386/cet-intrin-7.c: Likewise.
> > * gcc.target/i386/cet-intrin-8.c: Likewise.
> > * gcc.target/i386/cet-intrin-9.c: Likewise.
> > * gcc.target/i386/cet-label.c: Likewise.
> > * gcc.target/i386/cet-notrack-1a.c: Likewise.
> > * gcc.target/i386/cet-notrack-1b.c: Likewise.
> > * gcc.target/i386/cet-notrack-2a.c: Likewise.
> > * gcc.target/i386/cet-notrack-2b.c: Likewise.
> > * gcc.target/i386/cet-notrack-3.c: Likewise.
> > * gcc.target/i386/cet-notrack-4a.c: Likewise.
> > * gcc.target/i386/cet-notrack-4b.c: Likewise.
> > * gcc.target/i386/cet-notrack-5a.c: Likewise.
> > * gcc.target/i386/cet-notrack-5b.c: Likewise.
> > * gcc.target/i386/cet-notrack-6a.c: Likewise.
> > * gcc.target/i386/cet-notrack-6b.c: Likewise.
> > * gcc.target/i386/cet-notrack-7.c: Likewise.
> > * gcc.target/i386/cet-property-1.c: Likewise.
> > * gcc.target/i386/cet-property-2.c: Likewise.
> > * gcc.target/i386/cet-rdssp-1.c: Likewise.
> > * gcc.target/i386/cet-sjlj-1.c: Likewise.
> > * gcc.target/i386/cet-sjlj-2.c: Likewise.
> > * gcc.target/i386/cet-sjlj-3.c: Likewise.
> > * gcc.target/i386/cet-switch-1.c: Likewise.
> > * gcc.target/i386/cet-switch-2.c: Likewise.
> > * lib/target-supports.exp (check_effective_target_cet): New proc.
>
> A couple of questions:
>
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mcet" } */
> +/* { dg-final { scan-assembler-times "setssbsy" 2 } } */
> +
> +#include
> +
> +void f1 (void)
> +{
> + __builtin_ia32_setssbsy ();
> +}
> +
> +void f2 (void)
> +{
> + _setssbsy ();
> +}
>
> Is there a reason that both, __builtin and intrinsic versions are tested in a
> couple of places? The intrinsic version is just a wrapper for __builtin, so
> IMO
> testing intrinsic version should be enough.
No strong reason. Just to check that intrinsic names are recognized and
processed correctly.
The implementation could change and the test will catch inconsistency. I would
also assume
a user will use intrinsics that's why I add intrinsic check. Should I remove it?
>
> diff --git a/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> b/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> new file mode 100644
> index 000..f9223a5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> @@ -0,0 +1,39 @@
> +/* { dg-do run { target cet } } */
> +/* { dg-options "-O2 -finstrument-control-flow -mcet" } */
>
> The "target cet" directive just checks that CET instructions can be compiled.
> The test will (probably?) fail on targets with binutils that can compile CET
> instructions, but the target itself doesn't support CET. If this is the case,
> then
> check header has to be introduced, so the test can be bypassed on targets
> without runtime support.
The test will not fail even if a target doesn't support CET as 'rdssp'
instruction is a
NOP on such target and further usage of CET instruction is bypassed. In this
case
the code
+ ssp = rdssp (ssp);
Will keep ssp as 0.
Thanks,
Igor
> Uros.
RE: 0004-Part-4.-Update-x86-backend-to-enable-Intel-CET
Uros, thank you for the approval. Based on the approval of the first 3 patches (I've submitted them today), I need to adjust option and attribute names. I will resubmit the patch when I fix option and attribute names. Thanks, Igor > -Original Message- > From: Uros Bizjak [mailto:ubiz...@gmail.com] > Sent: Monday, September 18, 2017 11:58 AM > To: gcc-patches@gcc.gnu.org > Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> > Subject: Re: 0004-Part-4.-Update-x86-backend-to-enable-Intel-CET > > Hello! > > > gcc/ > > > > * common/config/i386/i386-common.c (OPTION_MASK_ISA_IBT_SET): > New. > > (OPTION_MASK_ISA_SHSTK_SET): Likewise. > > (OPTION_MASK_ISA_IBT_UNSET): Likewise. > > (OPTION_MASK_ISA_SHSTK_UNSET): Likewise. > > (ix86_handle_option): Add -mibt, -mshstk, -mcet handling. > > * config.gcc (extra_headers): Add cetintrin.h for x86 targets. > > (extra_objs): Add cet.o for Linux/x86 targets. > > (tmake_file): Add i386/t-cet for Linux/x86 targets. > > * config/i386/cet.c: New file. > > * config/i386/cetintrin.h: Likewise. > > * config/i386/t-cet: Likewise. > > * config/i386/cpuid.h (bit_SHSTK): New. > > (bit_IBT): Likewise. > > * config/i386/driver-i386.c (host_detect_local_cpu): Detect and pass > > IBT and SHSTK bits. > > * config/i386/i386-builtin-types.def > > (VOID_FTYPE_UNSIGNED_PVOID): New. > > (VOID_FTYPE_UINT64_PVOID): Likewise. > > * config/i386/i386-builtin.def: Add CET intrinsics. > > * config/i386/i386-c.c (ix86_target_macros_internal): Add > > OPTION_MASK_ISA_IBT, OPTION_MASK_ISA_SHSTK handling. > > * config/i386/i386-passes.def: Add pass_insert_endbranch pass. > > * config/i386/i386-protos.h (make_pass_insert_endbranch): New > > prototype. > > * config/i386/i386.c (rest_of_insert_endbranch): New. > > (pass_data_insert_endbranch): Likewise. > > (pass_insert_endbranch): Likewise. > > (make_pass_insert_endbranch): Likewise. > > (ix86_notrack_prefixed_insn_p): Likewise. > > (ix86_target_string): Add -mibt, -mshstk flags. > > (ix86_option_override_internal): Add flag_instrument_control_flow > > processing. > > (ix86_valid_target_attribute_inner_p): Set OPT_mibt, OPT_mshstk. > > (ix86_print_operand): Add 'notrack' prefix output. > > (ix86_init_mmx_sse_builtins): Add CET intrinsics. > > (ix86_expand_builtin): Expand CET intrinsics. > > (x86_output_mi_thunk): Add 'endbranch' instruction. > > * config/i386/i386.h (TARGET_IBT): New. > > (TARGET_IBT_P): Likewise. > > (TARGET_SHSTK): Likewise. > > (TARGET_SHSTK_P): Likewise. > > * config/i386/i386.md (unspecv): Add UNSPECV_NOP_RDSSP, > > UNSPECV_INCSSP, UNSPECV_SAVEPREVSSP, UNSPECV_RSTORSSP, > UNSPECV_WRSS, > > UNSPECV_WRUSS, UNSPECV_SETSSBSY, UNSPECV_CLRSSBSY. > > (builtin_setjmp_setup): New pattern. > > (builtin_longjmp): Likewise. > > (rdssp): Likewise. > > (incssp): Likewise. > > (saveprevssp): Likewise. > > (rstorssp): Likewise. > > (wrss): Likewise. > > (wruss): Likewise. > > (setssbsy): Likewise. > > (clrssbsy): Likewise. > > (nop_endbr): Likewise. > > * config/i386/i386.opt: Add -mcet, -mibt, -mshstk and -mcet-switch > > options. > > * config/i386/immintrin.h: Include . > > * config/i386/linux-common.h > > (file_end_indicate_exec_stack_and_cet): New prototype. > > (TARGET_ASM_FILE_END): New. > > LGTM. > > OK for mainline. > > Thanks, > Uros.
RE: 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-notrack attribute
Here is an updated patch (version #2). Mainly attribute and option names were changed. The test for ICF will be introduced in x86 specific tests (patch 0006-Part-6) as the implementation checks if the CF instrumentation is on to adjust a hash based on 'nocf'_check' attribute presence. In generic part CF instrumentation is off as no implementation exist. The patch for x86 specific tests (patch 0006-Part-6) is being reviewed by Uros. gcc/testsuite/ * c-c++-common/fcf-protection-1.c: New test. * c-c++-common/fcf-protection-2.c: Likewise. * c-c++-common/fcf-protection-3.c: Likewise. * c-c++-common/fcf-protection-4.c: Likewise. * c-c++-common/fcf-protection-5.c: Likewise. * c-c++-common/attr-nocf-check-1.c: Likewise. * c-c++-common/attr-nocf-check-2.c: Likewise. * c-c++-common/attr-nocf-check-3.c: Likewise. Is it ok for trunk? Thanks, Igor > -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Friday, August 25, 2017 11:01 PM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc- > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: Re: 0003-Part-3.-Add-tests-for-finstrument-control-flow-and- > notrack attribute > > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote: > > Part#3. Add tests for -finstrument-control-flow and notrack attribute. > > > > > > 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-n.patch > > > > > > From 7869de8a0c0ec55c4e9240c2483fefee97bf34c9 Mon Sep 17 00:00:00 > 2001 > > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com> > > Date: Mon, 3 Jul 2017 17:29:08 +0300 > > Subject: [PATCH 3/9] Part#3. Add tests for -finstrument-control-flow > > and notrack attribute. > > > > gcc/testsuite/ > > > > * c-c++-common/finstrument-control-flow.c: New test. > > * c-c++-common/notrack-1.c: Likewise. > > * c-c++-common/notrack-2.c: Likewise. > No concerns with the existing tests. > > We should consider an ICF test as I outlined in an earlier message. > > We should also consider tests where we drop/add the notrack attribute as > ISTM we ought to be getting warnings in those cases. > > Finally, you should consider tests in gcc.target/i386 that verify we generate > the proper instrumentation for a few tests. > > jeff 0003-Add-tests-for-fcf-protection-and-nocf_check-attribut.patch Description: 0003-Add-tests-for-fcf-protection-and-nocf_check-attribut.patch
RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
Here is an updated patch (version #2). Mainly attribute and option names were
changed.
gcc/doc/
* extend.texi: Add 'nocf_check' documentation.
* gimple.texi: Add second parameter to gimple_build_call_from_tree.
* invoke.texi: Add -fcf-protection documentation.
* rtl.texi: Add REG_CALL_NOTRACK documenation.
Is it ok for trunk?
Thanks,
Igor
> -Original Message-
> From: Tsimbalist, Igor V
> Sent: Friday, September 15, 2017 5:14 PM
> To: 'Jeff Law' <l...@redhat.com>; 'gcc-patches@gcc.gnu.org' patc...@gcc.gnu.org>
> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> Subject: RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> attribute
>
> > -Original Message-
> > From: Jeff Law [mailto:l...@redhat.com]
> > Sent: Friday, August 25, 2017 10:59 PM
> > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> > Subject: Re:
> > 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> > attribute
> >
> > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> > > Part#2. Document -finstrument-control-flow and notrack attribute.
> > >
> > >
> > > 0002-Part-2.-Document-finstrument-control-flow-and-notrac.patch
> > >
> > >
> > > From c3e45c80731672e74d638f787e80ba975279b9b9 Mon Sep 17 00:00:00
> > 2001
> > > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com>
> > > Date: Mon, 3 Jul 2017 17:12:49 +0300
> > > Subject: [PATCH 2/9] Part#2. Document -finstrument-control-flow and
> > > notrack attribute.
> > >
> > > gcc/
> > > * doc/extend.texi: Add 'notrack' documentation.
> > > * doc/invoke.texi: Add -finstrument-control-flow documentation.
> > > * doc/rtl.texi: Add REG_CALL_NOTRACK documenation.
> > > ---
> > > gcc/doc/extend.texi | 52
> > >
> > > gcc/doc/invoke.texi | 22 ++
> > > gcc/doc/rtl.texi| 15 +++
> > > 3 files changed, 89 insertions(+)
> > >
> > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > > 6934b4c..80de8a7 100644
> > > --- a/gcc/doc/extend.texi
> > > +++ b/gcc/doc/extend.texi
> > > @@ -5632,6 +5632,58 @@ Specify which floating-point unit to use.
> > > You must specify the @code{target("fpmath=sse,387")} option as
> > > @code{target("fpmath=sse+387")} because the comma would separate
> > > different options.
> > > +
> > > +@item notrack
> > > +@cindex @code{notrack} function attribute The @code{notrack}
> > > +attribute on a function is used to inform the compiler that the
> > > +function's prolog should not be instrumented when compiled with the
> > > +@option{-finstrument-control-flow} option. The compiler assumes
> > > +that the function's address is a valid target for a control-flow
> > > transfer.
> > Is the default to instrument everything when -finstrument-control-flow
> > is enabled? Or can we avoid instrumentation on a function that never
> > has its address taken (ie, it is only called via a call instruction?)
> The instrumentation is on by default but for all platform except of x86 it
> does
> nothing as the implementation is not supported. For x86 the implementation
> is lightweight and just increase a bit code size due to 'endbranch'
> instruction.
>
> Given a function decl is there an information already available if an address
> was taken from the function? I plan to do what you suggested later as an
> optimization especially for global function where ipa is required.
>
> > > +
> > > +The @code{notrack} attribute on a type of pointer to function is
> > > +used to inform the compiler that a call through the pointer should
> > > +not be instrumented when compiled with the
> > > +@option{-finstrument-control-flow} option. The compiler assumes
> > > +that the function's address from the pointer is a valid target for
> > > +a control-flow transfer. A direct function call through a function
> > > +name is assumed as a save call thus direct calls will not be
> > > +instrumented by the compiler.
> > s/save/safe/
> >
> > FWIW, I think putting the attribute into in the type system is a good
> > thing :-)
> >
> > > +
> > > +The @code{notrack} attribute is applied to an object's type. A The
> > > +@code{notrack} attribute is transfered to a call instruction at the
> > >
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
Here is an updated patch (version #2). The main differences are: - Change attribute and option names; - Add additional parameter to gimple_build_call_from_tree by adding a type parameter and use it 'nocf_check' attribute propagation; - Reimplement fixes in expand_call_stmt to propagate 'nocf_check' attribute; - Consider 'nocf_check' attribute in Identical Code Folding (ICF) optimization; - Add warning for type inconsistency regarding 'nocf_check' attribute; - Many small fixes; gcc/c-family/ * c-attribs.c (handle_nocf_check_attribute): New function. (c_common_attribute_table): Add 'nocf_check' handling. * c-common.c (check_missing_format_attribute): New function. * c-common.h: Likewise. gcc/c/ * c-typeck.c (convert_for_assignment): Add check for nocf_check attribute. * gimple-parser.c: Add second argument NULL to gimple_build_call_from_tree. gcc/cp/ * typeck.c (convert_for_assignment): Add check for nocf_check attribute. gcc/ * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for call insn. * combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK handling. * common.opt: Add fcf-protection flag. * emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling. * flag-types.h: Add enum cf_protection_level. * gimple.c (gimple_build_call_from_tree): Add second parameter. Add 'nocf_check' attribute propagation to gimple call. * gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK. (gimple_call_nocf_check_p): New function. (gimple_call_set_nocf_check): Likewise. * gimplify.c: Add second argument to gimple_build_call_from_tree. * ipa-icf.c: Add nocf_check attribute in statement hash. * recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling. * reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK). * toplev.c (process_options): Add flag_cf_protection handling. Is it ok for trunk? Thanks, Igor > -Original Message- > From: Richard Biener [mailto:richard.guent...@gmail.com] > Sent: Friday, September 15, 2017 2:14 PM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On Fri, Sep 15, 2017 at 1:12 PM, Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> wrote: > >> -----Original Message- > >> From: Tsimbalist, Igor V > >> Sent: Tuesday, September 12, 2017 5:35 PM > >> To: 'Richard Biener' <richard.guent...@gmail.com> > >> Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>; Tsimbalist, > >> Igor V <igor.v.tsimbal...@intel.com> > >> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > >> > -Original Message- > >> > From: Tsimbalist, Igor V > >> > Sent: Friday, August 18, 2017 4:43 PM > >> > To: 'Richard Biener' <richard.guent...@gmail.com> > >> > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > >> > <igor.v.tsimbal...@intel.com> > >> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > >> > > -Original Message- > >> > > From: Richard Biener [mailto:richard.guent...@gmail.com] > >> > > Sent: Friday, August 18, 2017 3:53 PM > >> > > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > >> > > Cc: gcc-patches@gcc.gnu.org > >> > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > > >> > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > >> > > <igor.v.tsimbal...@intel.com> wrote: > >> > > >> -Original Message- > >> > > >> From: Richard Biener [mailto:richard.guent...@gmail.com] > >> > > >> Sent: Tuesday, August 15, 2017 3:43 PM > >> > > >> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > >> > > >> Cc: gcc-patches@gcc.gnu.org > >> > > >> Subject: Re: > >> > > >> 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > >> > > >> > >> > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > >> > > >> <igor.v.tsimbal...@intel.com> wrote: > >> > > >> > Part#1. Add generic part for Intel CET enabling. > >> > > >> > > >> > > >> > The spec is available at > >> > > >> > > >> > > >> > https://software.intel.com/sites/default/files/managed/4d/2a > >> > > >> > /co nt ro l-f
RE: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Jeff Law > Sent: Friday, August 25, 2017 11:03 PM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc- > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation > > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote: > > Part#6. Add x86 tests for Intel CET implementation. > > > > > > 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation.patch > > > > > > From e4a8227e83e8e9f3ddbaa97707f3d335009e0e77 Mon Sep 17 00:00:00 > 2001 > > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com> > > Date: Fri, 21 Jul 2017 19:40:40 +0300 > > Subject: [PATCH 6/9] Part#6. Add x86 tests for Intel CET implementation. > > > > gcc/testsuite/ > > > > * g++.dg/cet-notrack-1.C: New test. > > * gcc.target/i386/cet-intrin-1.c: Likewise. > > * gcc.target/i386/cet-intrin-10.c: Likewise. > > * gcc.target/i386/cet-intrin-2.c: Likewise. > > * gcc.target/i386/cet-intrin-3.c: Likewise. > > * gcc.target/i386/cet-intrin-4.c: Likewise. > > * gcc.target/i386/cet-intrin-5.c: Likewise. > > * gcc.target/i386/cet-intrin-6.c: Likewise. > > * gcc.target/i386/cet-intrin-7.c: Likewise. > > * gcc.target/i386/cet-intrin-8.c: Likewise. > > * gcc.target/i386/cet-intrin-9.c: Likewise. > > * gcc.target/i386/cet-label.c: Likewise. > > * gcc.target/i386/cet-notrack-1a.c: Likewise. > > * gcc.target/i386/cet-notrack-1b.c: Likewise. > > * gcc.target/i386/cet-notrack-2a.c: Likewise. > > * gcc.target/i386/cet-notrack-2b.c: Likewise. > > * gcc.target/i386/cet-notrack-3.c: Likewise. > > * gcc.target/i386/cet-notrack-4a.c: Likewise. > > * gcc.target/i386/cet-notrack-4b.c: Likewise. > > * gcc.target/i386/cet-notrack-5a.c: Likewise. > > * gcc.target/i386/cet-notrack-5b.c: Likewise. > > * gcc.target/i386/cet-notrack-6a.c: Likewise. > > * gcc.target/i386/cet-notrack-6b.c: Likewise. > > * gcc.target/i386/cet-notrack-7.c: Likewise. > > * gcc.target/i386/cet-property-1.c: Likewise. > > * gcc.target/i386/cet-property-2.c: Likewise. > > * gcc.target/i386/cet-rdssp-1.c: Likewise. > > * gcc.target/i386/cet-sjlj-1.c: Likewise. > > * gcc.target/i386/cet-sjlj-2.c: Likewise. > > * gcc.target/i386/cet-sjlj-3.c: Likewise. > > * gcc.target/i386/cet-switch-1.c: Likewise. > > * gcc.target/i386/cet-switch-2.c: Likewise. > > * lib/target-supports.exp (check_effective_target_cet): New > > proc. > Whoops. NEvermind my previous comment about x86 specific tests. I > should have scanned the whole kit before starting to comment on the earlier > patches. > > Uros will have the say on the x86 specific bits. Given it's been 3 weeks, you > might want to ping him directly to start getting his feedback. Thanks, Jeff. Whom should I ping for other patches review, which are related compiler libraries like libgcc and other target libraries? Igor > jeff
RE: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
> -Original Message-
> From: Jeff Law [mailto:l...@redhat.com]
> Sent: Friday, August 25, 2017 10:59 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Subject: Re: 0002-Part-2.-Document-finstrument-control-flow-and-notrack
> attribute
>
> On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> > Part#2. Document -finstrument-control-flow and notrack attribute.
> >
> >
> > 0002-Part-2.-Document-finstrument-control-flow-and-notrac.patch
> >
> >
> > From c3e45c80731672e74d638f787e80ba975279b9b9 Mon Sep 17 00:00:00
> 2001
> > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com>
> > Date: Mon, 3 Jul 2017 17:12:49 +0300
> > Subject: [PATCH 2/9] Part#2. Document -finstrument-control-flow and
> > notrack attribute.
> >
> > gcc/
> > * doc/extend.texi: Add 'notrack' documentation.
> > * doc/invoke.texi: Add -finstrument-control-flow documentation.
> > * doc/rtl.texi: Add REG_CALL_NOTRACK documenation.
> > ---
> > gcc/doc/extend.texi | 52
> >
> > gcc/doc/invoke.texi | 22 ++
> > gcc/doc/rtl.texi| 15 +++
> > 3 files changed, 89 insertions(+)
> >
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > 6934b4c..80de8a7 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -5632,6 +5632,58 @@ Specify which floating-point unit to use. You
> > must specify the @code{target("fpmath=sse,387")} option as
> > @code{target("fpmath=sse+387")} because the comma would separate
> > different options.
> > +
> > +@item notrack
> > +@cindex @code{notrack} function attribute The @code{notrack}
> > +attribute on a function is used to inform the compiler that the
> > +function's prolog should not be instrumented when compiled with the
> > +@option{-finstrument-control-flow} option. The compiler assumes that
> > +the function's address is a valid target for a control-flow transfer.
> Is the default to instrument everything when -finstrument-control-flow is
> enabled? Or can we avoid instrumentation on a function that never has its
> address taken (ie, it is only called via a call instruction?)
The instrumentation is on by default but for all platform except of x86 it does
nothing as
the implementation is not supported. For x86 the implementation is lightweight
and just
increase a bit code size due to 'endbranch' instruction.
Given a function decl is there an information already available if an address
was taken from
the function? I plan to do what you suggested later as an optimization
especially for global
function where ipa is required.
> > +
> > +The @code{notrack} attribute on a type of pointer to function is used
> > +to inform the compiler that a call through the pointer should not be
> > +instrumented when compiled with the
> > +@option{-finstrument-control-flow} option. The compiler assumes that
> > +the function's address from the pointer is a valid target for a
> > +control-flow transfer. A direct function call through a function
> > +name is assumed as a save call thus direct calls will not be
> > +instrumented by the compiler.
> s/save/safe/
>
> FWIW, I think putting the attribute into in the type system is a good thing
> :-)
>
> > +
> > +The @code{notrack} attribute is applied to an object's type. A The
> > +@code{notrack} attribute is transfered to a call instruction at the
> > +GIMPLE and RTL translation phases. The attribute is not propagated
> > +through assignment, store and load.
> > +
> > +@smallexample
> > +@{
> > +void (*foo)(void) __attribute__(notrack); void (*foo1)(void)
> > +__attribute__(notrack); void (*foo2)(void);
> > +
> > +int
> > +foo (void) /* The function's address is not tracked. */
> > +
> > + /* This call site is not tracked for
> > + control-flow instrumentation. */ (*foo1)();
> > + foo1 = foo2;
> > + /* This call site is still not tracked for
> > + control-flow instrumentation. */ (*foo1)();
> > +
> > + /* This call site is tracked for
> > + control-flow instrumentation. */ (*foo2)();
> > + foo2 = foo1;
> > + /* This call site is still tracked for
> > + control-flow instrumentation. */ (*foo2)();
> > +
> > + return 0;
> > +@}
> > +@end smallexample
> Given the notrack attribute is part of the type system, could we issue a
> warning on the foo1 = foo2 assignment since we're discarding tracking
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message- > From: Tsimbalist, Igor V > Sent: Tuesday, September 12, 2017 5:35 PM > To: 'Richard Biener' <richard.guent...@gmail.com> > Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>; Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > -Original Message- > > From: Tsimbalist, Igor V > > Sent: Friday, August 18, 2017 4:43 PM > > To: 'Richard Biener' <richard.guent...@gmail.com> > > Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > > <igor.v.tsimbal...@intel.com> > > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > -Original Message- > > > From: Richard Biener [mailto:richard.guent...@gmail.com] > > > Sent: Friday, August 18, 2017 3:53 PM > > > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V > > > <igor.v.tsimbal...@intel.com> wrote: > > > >> -Original Message- > > > >> From: Richard Biener [mailto:richard.guent...@gmail.com] > > > >> Sent: Tuesday, August 15, 2017 3:43 PM > > > >> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > > > >> Cc: gcc-patches@gcc.gnu.org > > > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > >> > > > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V > > > >> <igor.v.tsimbal...@intel.com> wrote: > > > >> > Part#1. Add generic part for Intel CET enabling. > > > >> > > > > >> > The spec is available at > > > >> > > > > >> > https://software.intel.com/sites/default/files/managed/4d/2a/co > > > >> > nt ro l-f low-enforcement-technology-preview.pdf <..skipped..> > > > >> I think 'notrack' is somewhat unspecific of a name, what > > > >> prevented you to use 'nocet'? > > > > > > > > Actually it's specific. The HW will have a prefix with exactly > > > > this name and > > > the same meaning. And I think, what is more important, 'track/notrack' > > > gives better semantic for a user. CET is a name bound with Intel > > > specific technology. > > > > > > But 'tracking' something is quite unspecific. Tracking for what? > > > 'no_verify_cf' (aka do not verify control flow) maybe? > > > > The name just has to suggest the right semantic. 'no_verify_cf' is > > good, let's use it unless different name appears. > I have renamed all newly introduced function and macro names to use > 'noverify_cf'. But I still keep the attribute name as 'notrack'. Historically > the > attribute name follows the public CET specification, which uses 'no-track > prefix' wording. Is it ok to keep such attribute name? Here is an updated proposal about option name and attribute name. The new option has values to let a user to choose what control-flow protection to activate. -fcf-protection=[full|branch|return|none] branch - do control-flow protection for indirect jumps and calls return - do control-flow protection for function returns full - alias to specify both branch + return none - turn off protection. This value is needed when/if cf-protection is turned on by default by driver in future Attribute name is the most tough one. Here are several names to evaluate: 'nocf_verify' or 'nocf_check', or to be more specific and to mimic option name 'nocf_branch_verify' or 'nocf_branch_check'. I would prefer 'nocf_check' as it applies to functions and function pointers so it's definitely related to a branch and it's a smaller one. If you ok with the new proposal I'll implement it in a general parts (code, documentation and tests) and resend these patches for review. Thanks, Igor
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message-
> From: Tsimbalist, Igor V
> Sent: Tuesday, September 12, 2017 5:59 PM
> To: 'Jeff Law' <l...@redhat.com>; 'gcc-patches@gcc.gnu.org' patc...@gcc.gnu.org>
> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
>
>
> > -Original Message-
> > From: Jeff Law [mailto:l...@redhat.com]
> > Sent: Friday, August 25, 2017 10:50 PM
> > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> >
> > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> > > Part#1. Add generic part for Intel CET enabling.
> > >
>
> > Q. Do we need to do anything with ICF (identical code folding) and CFE?
> > Given two functions which have the same implementation in gimple,
> > except that one has a notrack indirect call and the other has a
> > tracked indirect call, what is proper behavior? I think we'd keep
> > them separate which implies we need to make sure the notrack attribute
> > is part of the ICF hashing implementation. It'd probably even be
> > worth building a test for this :-)
> Are you talking about a case when such two functions are inlined? Or there is
> a possibility to merge function bodies if they are identical?
>
> I agree with you that the functions should be kept separate. I haven't looked
> into such optimization in gcc so I need to learn it.
I thought over this case and my conclusion is that nothing has to be done
regarding ICF.
First of all let's sync on a case we are talking about. A code template could
look like
fn1 definition
{
}
fn2 definition with notrack attribute
{
}
func definition
{
...
}
Is it the case you are talking about? Let's consider different scenarios:
1) calls to fn1 and fn2 are direct calls. In that case 'notrack' has no effect
on direct calls as they are
assumed to be save (it applies to indirect calls only). ICF can be done here;
2) one of calls is an indirect call or both calls are indirect calls. If
compiler can prove what exact functions
are called then indirect call(s) can be replaced by direct call(s) and that
gives us the case 1);
3) if compiler cannot prove what function is called it will keep the indirect
call and so there is nothing
to do for ICF here.
Thanks,
Igor
>
> > > }
> > >
> > >
> > > +/* Return true if call GS is marked as no-track. */
> > > +
> > > +static inline bool
> > > +gimple_call_with_notrack_p (const gcall *gs) {
> > > + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; }
> > > +
> > > +static inline bool
> > > +gimple_call_with_notrack_p (const gimple *gs) {
> > > + const gcall *gc = GIMPLE_CHECK2 (gs);
> > > + return gimple_call_with_notrack_p (gc); }
> > Agree with Richi WRT avoiding gimple * overloads.
> Fixed.
>
> Thanks,
> Igor
>
> >
> >
> > Jeff
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Friday, August 25, 2017 10:50 PM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc- > patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote: > > Part#1. Add generic part for Intel CET enabling. > > > > The spec is available at > > > > https://software.intel.com/sites/default/files/managed/4d/2a/control-f > > low-enforcement-technology-preview.pdf > > > > High-level design. > > -- > > > > A proposal is to introduce a target independent flag > > -finstrument-control-flow with a semantic to instrument a code to > > control validness or integrity of control-flow transfers using jump > > and call instructions. The main goal is to detect and block a possible > > malware execution through transfer the execution to unknown target > > address. Implementation could be either software or target based. Any > > target platforms can provide their implementation for instrumentation > > under this option. > > > > When the -finstrument-control-flow flag is set each implementation has > > to check if a support exists for a target platform and report an error > > if no support is found. > > > > The compiler should instrument any control-flow transfer points in a > > program (ex. call/jmp/ret) as well as any landing pads, which are > > targets of for control-flow transfers. > > > > A new 'notrack' attribute is introduced to provide hand tuning support. > > The attribute directs the compiler to skip a call to a function and a > > function's landing pad from instrumentation (tracking). The attribute > > can be used for function and pointer to function types, otherwise it > > will be ignored. The attribute is saved in a type and propagated to a > > GIMPLE call statement and later to a call instruction. > > > > Currently all platforms except i386 will report the error and do no > > instrumentation. i386 will provide the implementation based on a > > specification published by Intel for a new technology called > > Control-flow Enforcement Technology (CET). > > > > > > 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch > > > > > > From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 > 2001 > > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com> > > Date: Mon, 3 Jul 2017 17:11:58 +0300 > > Subject: [PATCH 1/9] Part#1. Add generic part for Intel CET enabling. > > > > The spec is available at > > > > https://software.intel.com/sites/default/files/managed/4d/2a/control-f > > low-enforcement-technology-preview.pdf > > > > High-level design. > > -- > > > > A proposal is to introduce a target independent flag > > -finstrument-control-flow with a semantic to instrument a code to > > control validness or integrity of control-flow transfers using jump > > and call instructions. The main goal is to detect and block a possible > > malware execution through transfer the execution to unknown target > > address. Implementation could be either software or target based. Any > > target platforms can provide their implementation for instrumentation > > under this option. > > > > When the -finstrument-control-flow flag is set each implementation has > > to check if a support exists for a target platform and report an error > > if no support is found. > > > > The compiler should instrument any control-flow transfer points in a > > program (ex. call/jmp/ret) as well as any landing pads, which are > > targets of for control-flow transfers. > > > > A new 'notrack' attribute is introduced to provide hand tuning support. > > The attribute directs the compiler to skip a call to a function and a > > function's landing pad from instrumentation (tracking). The attribute > > can be used for function and pointer to function types, otherwise it > > will be ignored. The attribute is saved in a type and propagated to a > > GIMPLE call statement and later to a call instruction. > > > > Currently all platforms except i386 will report the error and do no > > instrumentation. i386 will provide the implementation based on a > > specification published by Intel for a new technology called > > Control-flow Enforcement Technology (CET). > > > > gcc/c-family/ > > > > * c-attribs.c (handle_notrack_attribute): New function. > > (c_common_attribut
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Friday, August 25, 2017 10:32 PM > To: Richard Biener <richard.guent...@gmail.com>; Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > On 08/15/2017 07:42 AM, Richard Biener wrote: > > > > Please change the names to omit 'with_', thus just notrack and > > GF_CALL_NOTRACK. > > > > I think 'notrack' is somewhat unspecific of a name, what prevented you > > to use 'nocet'? > I think we should look for something better than notrack. I think "control > flow enforcement/CFE" is commonly used for this stuff. CET is an Intel > marketing name IIRC. > > The tracking is for indirect branch/call targets. So some combination of cfe, > branch/call and track should be sufficient. Still remaining question from me - is it ok to use 'notrack' as the attribute name. I've asked Richard about this in this thread. Thanks, Igor > > > Any idea how to implement a software-based solution efficiently? > > Creating a table of valid destination addresses in a special section > > should be possible without too much work, am I right in that only > > indirect control transfer is checked? Thus CET assumes the code > > itself cannot be changed (and thus the stack isn't executable)? > Well, there's two broad areas that have to be addressed. > > First you need to separate the call stack from the rest of the call frame, or > at > least the parts of the call frame that are potentially vulnerable to overruns. > LLVM has some code to do this. Essentially any object in the stack that is > not > proven to be safely accessed gets put into a separate stack. That roughly > duplicates the shadow stack capability. I think their implementation is just > x86 and IIRC doesn't work in some circumstances -- I'd consider it a proof of > concept, not something ready for production use. > > > Bernd and I also spec'd a couple more approaches to protect the return > address. Essentially, the return address turns into a cookie that a > particular > caller can use to lookup/map to a real return address. We didn't take any of > this to completion because it was pretty clear the ROP mitigation landscape > was going to change and make software only solutions less appealing. > > Second you need the indirect branch/call tracking. I spec'd something out in > this space with Intel's engineers years ago. Essentially building tables of > valid > targets for indirect branches and checking > instrumentation. You can have a global table, per-DSO tables or you > can have a per-branch table. It gets a little hairy in mixed mode > environments. But it basically works how you'd expect. Indirect > branches/calls turn into something considerably more complex as do the > branch/call targets if you have access to something like a last-taken-branch. > > Jeff
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message-
> From: Tsimbalist, Igor V
> Sent: Friday, August 18, 2017 4:43 PM
> To: 'Richard Biener' <richard.guent...@gmail.com>
> Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com>
> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
>
> > -Original Message-
> > From: Richard Biener [mailto:richard.guent...@gmail.com]
> > Sent: Friday, August 18, 2017 3:53 PM
> > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> >
> > On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V
> > <igor.v.tsimbal...@intel.com> wrote:
> > >> -Original Message-
> > >> From: Richard Biener [mailto:richard.guent...@gmail.com]
> > >> Sent: Tuesday, August 15, 2017 3:43 PM
> > >> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> > >> Cc: gcc-patches@gcc.gnu.org
> > >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> > >>
> > >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V
> > >> <igor.v.tsimbal...@intel.com> wrote:
> > >> > Part#1. Add generic part for Intel CET enabling.
> > >> >
> > >> > The spec is available at
> > >> >
> > >> > https://software.intel.com/sites/default/files/managed/4d/2a/cont
> > >> > ro l-f low-enforcement-technology-preview.pdf
> > >> >
> > >> > High-level design.
> > >> > --
> > >> >
> > >> > A proposal is to introduce a target independent flag
> > >> > -finstrument-control-flow with a semantic to instrument a code to
> > >> > control validness or integrity of control-flow transfers using
> > >> > jump and call instructions. The main goal is to detect and block
> > >> > a possible malware execution through transfer the execution to
> > >> > unknown target address. Implementation could be either software
> > >> > or target based. Any target platforms can provide their
> > >> > implementation for instrumentation under this option.
> > >> >
> > >> > When the -finstrument-control-flow flag is set each
> > >> > implementation has to check if a support exists for a target
> > >> > platform and report an error if no support is found.
> > >> >
> > >> > The compiler should instrument any control-flow transfer points
> > >> > in a program (ex. call/jmp/ret) as well as any landing pads,
> > >> > which are targets of for control-flow transfers.
> > >> >
> > >> > A new 'notrack' attribute is introduced to provide hand tuning
> support.
> > >> > The attribute directs the compiler to skip a call to a function
> > >> > and a function's landing pad from instrumentation (tracking). The
> > >> > attribute can be used for function and pointer to function types,
> > >> > otherwise it will be ignored. The attribute is saved in a type
> > >> > and propagated to a GIMPLE call statement and later to a call
> instruction.
> > >> >
> > >> > Currently all platforms except i386 will report the error and do
> > >> > no instrumentation. i386 will provide the implementation based on
> > >> > a specification published by Intel for a new technology called
> > >> > Control-flow Enforcement Technology (CET).
> > >>
> > >> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d
> > >> 100644
> > >> --- a/gcc/gimple.c
> > >> +++ b/gcc/gimple.c
> > >> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t)
> > >>gimple_set_no_warning (call, TREE_NO_WARNING (t));
> > >>gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t));
> > >>
> > >> + if (fndecl == NULL_TREE)
> > >> +{
> > >> + /* Find the type of an indirect call. */
> > >> + tree addr = CALL_EXPR_FN (t);
> > >> + if (TREE_CODE (addr) != FUNCTION_DECL)
> > >> + {
> > >> + tree fntype = TREE_TYPE (addr);
> > >> + gcc_assert (POINTER_TYPE_P (fntype));
> > >> + fntype = TREE_TYPE (fntype);
> > >> +
> > >> + /* Check if its type has the no-track attribute and propagate
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message-
> From: Richard Biener [mailto:richard.guent...@gmail.com]
> Sent: Friday, August 18, 2017 3:53 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
>
> On Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com> wrote:
> >> -Original Message-
> >> From: Richard Biener [mailto:richard.guent...@gmail.com]
> >> Sent: Tuesday, August 15, 2017 3:43 PM
> >> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> >>
> >> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V
> >> <igor.v.tsimbal...@intel.com> wrote:
> >> > Part#1. Add generic part for Intel CET enabling.
> >> >
> >> > The spec is available at
> >> >
> >> > https://software.intel.com/sites/default/files/managed/4d/2a/contro
> >> > l-f low-enforcement-technology-preview.pdf
> >> >
> >> > High-level design.
> >> > --
> >> >
> >> > A proposal is to introduce a target independent flag
> >> > -finstrument-control-flow with a semantic to instrument a code to
> >> > control validness or integrity of control-flow transfers using jump
> >> > and call instructions. The main goal is to detect and block a
> >> > possible malware execution through transfer the execution to
> >> > unknown target address. Implementation could be either software or
> >> > target based. Any target platforms can provide their implementation
> >> > for instrumentation under this option.
> >> >
> >> > When the -finstrument-control-flow flag is set each implementation
> >> > has to check if a support exists for a target platform and report
> >> > an error if no support is found.
> >> >
> >> > The compiler should instrument any control-flow transfer points in
> >> > a program (ex. call/jmp/ret) as well as any landing pads, which are
> >> > targets of for control-flow transfers.
> >> >
> >> > A new 'notrack' attribute is introduced to provide hand tuning support.
> >> > The attribute directs the compiler to skip a call to a function and
> >> > a function's landing pad from instrumentation (tracking). The
> >> > attribute can be used for function and pointer to function types,
> >> > otherwise it will be ignored. The attribute is saved in a type and
> >> > propagated to a GIMPLE call statement and later to a call instruction.
> >> >
> >> > Currently all platforms except i386 will report the error and do no
> >> > instrumentation. i386 will provide the implementation based on a
> >> > specification published by Intel for a new technology called
> >> > Control-flow Enforcement Technology (CET).
> >>
> >> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d
> >> 100644
> >> --- a/gcc/gimple.c
> >> +++ b/gcc/gimple.c
> >> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t)
> >>gimple_set_no_warning (call, TREE_NO_WARNING (t));
> >>gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t));
> >>
> >> + if (fndecl == NULL_TREE)
> >> +{
> >> + /* Find the type of an indirect call. */
> >> + tree addr = CALL_EXPR_FN (t);
> >> + if (TREE_CODE (addr) != FUNCTION_DECL)
> >> + {
> >> + tree fntype = TREE_TYPE (addr);
> >> + gcc_assert (POINTER_TYPE_P (fntype));
> >> + fntype = TREE_TYPE (fntype);
> >> +
> >> + /* Check if its type has the no-track attribute and propagate
> >> +it to the CALL insn. */
> >> + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype)))
> >> + gimple_call_set_with_notrack (call, TRUE);
> >> + }
> >> +}
> >>
> >> this means notrack is not recognized if fndecl is not NULL. Note
> >> that only the two callers know the real function type in effect (they
> >> call gimple_call_set_fntype with it). I suggest to pass down that
> >> type to gimple_build_call_from_tree and move the
> >> gimple_call_set_fntype call there as well. And simply use the type for the
> above.
> >
> > The best way to s
RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> -Original Message-
> From: Richard Biener [mailto:richard.guent...@gmail.com]
> Sent: Tuesday, August 15, 2017 3:43 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
>
> On Tue, Aug 1, 2017 at 10:56 AM, Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com> wrote:
> > Part#1. Add generic part for Intel CET enabling.
> >
> > The spec is available at
> >
> > https://software.intel.com/sites/default/files/managed/4d/2a/control-f
> > low-enforcement-technology-preview.pdf
> >
> > High-level design.
> > --
> >
> > A proposal is to introduce a target independent flag
> > -finstrument-control-flow with a semantic to instrument a code to
> > control validness or integrity of control-flow transfers using jump
> > and call instructions. The main goal is to detect and block a possible
> > malware execution through transfer the execution to unknown target
> > address. Implementation could be either software or target based. Any
> > target platforms can provide their implementation for instrumentation
> > under this option.
> >
> > When the -finstrument-control-flow flag is set each implementation has
> > to check if a support exists for a target platform and report an error
> > if no support is found.
> >
> > The compiler should instrument any control-flow transfer points in a
> > program (ex. call/jmp/ret) as well as any landing pads, which are
> > targets of for control-flow transfers.
> >
> > A new 'notrack' attribute is introduced to provide hand tuning support.
> > The attribute directs the compiler to skip a call to a function and a
> > function's landing pad from instrumentation (tracking). The attribute
> > can be used for function and pointer to function types, otherwise it
> > will be ignored. The attribute is saved in a type and propagated to a
> > GIMPLE call statement and later to a call instruction.
> >
> > Currently all platforms except i386 will report the error and do no
> > instrumentation. i386 will provide the implementation based on a
> > specification published by Intel for a new technology called
> > Control-flow Enforcement Technology (CET).
>
> diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t)
>gimple_set_no_warning (call, TREE_NO_WARNING (t));
>gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t));
>
> + if (fndecl == NULL_TREE)
> +{
> + /* Find the type of an indirect call. */
> + tree addr = CALL_EXPR_FN (t);
> + if (TREE_CODE (addr) != FUNCTION_DECL)
> + {
> + tree fntype = TREE_TYPE (addr);
> + gcc_assert (POINTER_TYPE_P (fntype));
> + fntype = TREE_TYPE (fntype);
> +
> + /* Check if its type has the no-track attribute and propagate
> +it to the CALL insn. */
> + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype)))
> + gimple_call_set_with_notrack (call, TRUE);
> + }
> +}
>
> this means notrack is not recognized if fndecl is not NULL. Note that only
> the
> two callers know the real function type in effect (they call
> gimple_call_set_fntype with it). I suggest to pass down that type to
> gimple_build_call_from_tree and move the gimple_call_set_fntype call
> there as well. And simply use the type for the above.
The best way to say is notrack is not propagated if fndecl is not NULL. Fndecl,
if not NULL, is a direct call and notrack is not applicable for such calls. I
will add a comment before the if.
I would like to propose modifying the existing code without changing
interfaces. The idea is that at the time the notrack is propagated (the code
snippet above) the gimple call was created and the correct type was assigned to
the 'call' exactly by gimple_call_set_fntype. My proposal is to get the type
out of the gimple 'call' (like gimple_call_fntype) instead of the tree 't'. Is
it right?
> +static inline bool
> +gimple_call_with_notrack_p (const gimple *gs) {
> + const gcall *gc = GIMPLE_CHECK2 (gs);
> + return gimple_call_with_notrack_p (gc); }
>
> please do not add gimple * overloads for new APIs, instead make sure to
> pass down gcalls at callers.
Ok, I will remove.
> Please change the names to omit 'with_', thus just notrack and
> GF_CALL_NOTRACK.
Ok, I will rename.
> I think 'notrack' is somewhat unspecific of a name, what prevented you to
> use 'nocet'?
Actually it's specific. The HW will have a prefi
0008-Part-8.-Add-Intel-CET-support-for-EH-in-libgcc
Part#8. Add Intel CET support for EH in libgcc. Control-flow Enforcement Technology (CET), published by Intel, introduces the Shadow Stack feature, which ensures a return from a function is done to exactly the same location from where the function was called. When EH is present the control-flow transfer may skip some stack frames and the shadow stack has to be adjusted not to signal a violation of a control-flow transfer. It's done by counting a number of skipping frames and adjusting shadow stack pointer by this number. Having new semantic of the 'ret' instruction if CET is supported in HW the 'ret' instruction cannot be generated in ix86_expand_epilogue when we are returning after EH is processed. Added a code in ix86_expand_epilogue to adjust Shadow stack pointer and the indirect jump is generated instead of 'ret'. As sp register is used during this adjustment thus the argument in pro_epilogue_adjust_stack is changed to update cfa_reg based on whether control-flow instrumentation is set. Without updating the cfa_reg field there is an assert later in dwarf2 pass related to mismatch the stack register and cfa_reg value. 0008-Part-8.-Add-Intel-CET-support-for-EH-in-libgcc.patch Description: 0008-Part-8.-Add-Intel-CET-support-for-EH-in-libgcc.patch
0004-Part-4.-Update-x86-backend-to-enable-Intel-CET
Part#4. Update x86 backend to enable Intel CET. All platforms except i386 will report the error and do no instrumentation with -finstrument-control-flow option. i386 will provide the implementation based on a specification published by Intel for a new technology called Control-flow Enforcement Technology (CET). The spec is available at https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf The implementation in this patch: 1) enables Control-flow Enforcement Technology (CET), published by Intel. This part introduces i386 specific options -mcet, -mibt and -mshstk, new instructions and intrinsics; 2) provides support for -finstrument-control-flow option by doing needed code instrumentation, which is based on CET features. 0004-Part-4.-Update-x86-backend-to-enable-Intel-CET.PATCH Description: 0004-Part-4.-Update-x86-backend-to-enable-Intel-CET.PATCH
0009-Part-9.-Enable-bootstrap-GCC-with-CET-flags
Part#9. Enable bootstrap GCC with CET flags. 0009-Part-9.-Enable-bootstrap-GCC-with-CET-flags.patch Description: 0009-Part-9.-Enable-bootstrap-GCC-with-CET-flags.patch
0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
Part#6. Add x86 tests for Intel CET implementation. 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation.patch Description: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation.patch
0007-Part-7.-Enable-building-libgcc-with-CET-options
Part#7. Enable building libgcc with CET options. Enable building libgcc with CET options by default on Linux/x86 if binutils supports CET v2.0. It can be disabled with --disable-cet. It is an error to configure GCC with --enable-cet if bintuiils doesn't support CET v2.0. 0007-Part-7.-Enable-building-libgcc-with-CET-options.patch Description: 0007-Part-7.-Enable-building-libgcc-with-CET-options.patch
0005-Part-5.-Add-x86-CET-documentation
Part#5. Add x86 CET documentation. 0005-Part-5.-Add-x86-CET-documentation.patch Description: 0005-Part-5.-Add-x86-CET-documentation.patch
0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
Part#1. Add generic part for Intel CET enabling. The spec is available at https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf High-level design. -- A proposal is to introduce a target independent flag -finstrument-control-flow with a semantic to instrument a code to control validness or integrity of control-flow transfers using jump and call instructions. The main goal is to detect and block a possible malware execution through transfer the execution to unknown target address. Implementation could be either software or target based. Any target platforms can provide their implementation for instrumentation under this option. When the -finstrument-control-flow flag is set each implementation has to check if a support exists for a target platform and report an error if no support is found. The compiler should instrument any control-flow transfer points in a program (ex. call/jmp/ret) as well as any landing pads, which are targets of for control-flow transfers. A new 'notrack' attribute is introduced to provide hand tuning support. The attribute directs the compiler to skip a call to a function and a function's landing pad from instrumentation (tracking). The attribute can be used for function and pointer to function types, otherwise it will be ignored. The attribute is saved in a type and propagated to a GIMPLE call statement and later to a call instruction. Currently all platforms except i386 will report the error and do no instrumentation. i386 will provide the implementation based on a specification published by Intel for a new technology called Control-flow Enforcement Technology (CET). 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch Description: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch
0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute
Part#2. Document -finstrument-control-flow and notrack attribute. 0002-Part-2.-Document-finstrument-control-flow-and-notrac.patch Description: 0002-Part-2.-Document-finstrument-control-flow-and-notrac.patch
0003-Part-3.-Add-tests-for-finstrument-control-flow-and-notrack attribute
Part#3. Add tests for -finstrument-control-flow and notrack attribute. 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-n.patch Description: 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-n.patch
RE: [PATCH] [i386] Enable Control-flow Enforcement Technology (CET).
Thanks for the feedback. I'll redo the patch according to your comments. Igor -Original Message- From: Joseph Myers [mailto:jos...@codesourcery.com] Sent: Tuesday, June 20, 2017 11:30 PM To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> Cc: gcc-patches@gcc.gnu.org; Chupin, Pavel V <pavel.v.chu...@intel.com> Subject: Re: [PATCH] [i386] Enable Control-flow Enforcement Technology (CET). Importance: High This patch is completely missing documentation (in *.texi files) of the new options, attribute, built-in functions etc. You appear to be adding quite target-specific things to the architecture-independent compiler. If the attribute, for example, is to be architecture-independent, the documentation needs to define semantics for it that make sense on any architecture (or any architecture providing such features), not just on x86; the patch submission needs to justify the design choices of what is target-specific and what is target-independent. (Cf. MPX where there are various architecture-independent features for which a software implementation would be logically possible, although actually the only implementation of those features in GCC is for MPX hardware.) I don't think this patch would even build for non-x86 targets, because you're putting completely x86-specific references such as TARGET_CET and gen_nop_endbr in target-independent files. -- Joseph S. Myers jos...@codesourcery.com
[PATCH] [i386] Enable Control-flow Enforcement Technology (CET).
Control-flow Enforcement Technology (CET) provides the following capabilities to defend against ROP/JOP style control-flow subversion attacks: - Shadow Stack - return address protection to defend against Return Oriented Programming, - Indirect branch tracking - free branch protection to defend against Jump/Call Oriented Programming. Details are described in the doc https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf This patch enables CET in the compiler only (gcc directory). The executables built with the CET enabled compiler can run successfully on non-CET i386 HW as executed new instructions are NOPs there. Functional testing can be done through Intel® Software Development Emulator or Intel® SDE. There will be more patches to enable remaining intrinsics, to support CET in the compiler libraries (exception handling) and in glibc. The patch adds 1) new options to control the technology, 2) three new instructions (endbr, rdssp, inssp) and intrinsics, 3) a pass to generate endbr instruction, 4) new 'notrack' attribute for functions and pointers to function and code generation for it, 5) shadow stack processing in setjmp/longjmp builtins. Basic functional tests are added. Bootstrap is done successfully w/o and w/ CET option (-mcet). gcc/ * builtins.c (expand_builtin_setjmp_setup): Add saving shadow stack pointer in jmpbuf using rdssp insatruction. (expand_builtin_longjmp): Add adjusting shadow stack pointer using incssp instruction. * c-family/c-attribs.c (handle_notrack_attribute): New function. (c_common_attribute_table): Add a 'notrack' attribute. * calls.c (emit_call_1): Set REG_CALL_NOTRACK on call insn. (flags_from_decl_or_type): Retrieve notrack attribute from a decl. (expand_call): Retrieve notrack attribute from a decl. * combine.c: Handle REG_CALL_NOTRACK. * common/config/i386/i386-common.c (OPTION_MASK_ISA_CET_SET, OPTION_MASK_ISA_CET_UNSET): New. (ix86_handle_option): Handle OPT_mcet. * config.gcc: Add cetintrin.h. * config/i386/cetintrin.h: New file. * config/i386/cpuid.h: (bit_CET) new bit. * config/i386/driver-i386.c (host_detect_local_cpu): Detect cet. * config/i386/i386-builtin.def (__builtin_ia32_rdsspd, __builtin_ia32_rdsspq, __builtin_ia32_incsspd, __builtin_ia32_incsspd): New intrinsics. * config/i386/i386-c.c (ix86_target_macros_internal): Define __CET__. * config/i386/i386-protos.h (ix86_notrack_prefixed_insn_p): New. * config/i386/i386.c (ix86_target_string): Add -mcet. (ix86_valid_target_attribute_inner_p): Add cet. (ix86_print_operand): Output notrack. BDESC_VERIFYS for CET intrinsics. (ix86_init_mmx_sse_builtins): Define CET intrinsics. (x86_output_mi_thunk): Add endbr instruction. (ix86_notrack_prefixed_insn_p): New function. * config/i386/i386.h (TARGET_CET, TARGET_CET_P): New. * config/i386/i386.md (define_insn "rdssp"): New instruction. (define_insn "incssp"): Likewise. (define_insn "nop_endbr"): Likewise. * config/i386/i386.opt (mcet, mcet-switch, mcet-indbranch-tracking, mcet-shadow-stack): New options. * config/i386/immintrin.h Add include . * final.c (rest_of_handle_cet): New. (pass_data_handle_cet): New. (pass_handle_cet): New. (make_pass_handle_cet): New. * passes.def: (pass_handle_cet) Add pass. * reg-notes.def: (CALL_NOTRACK) New note for notrack. * timevar.def: (TV_CET) New. * tree-core.h: (ECF_NOTRACK) New. * tree-pass.h: (make_pass_handle_cet) New. gcc/testsuite/ * gcc.target/i386/cet-intrin.c: New test. * gcc.target/i386/cet-label.c: Likewise. * gcc.target/i386/cet-notrack.c: Likewise. * gcc.target/i386/cet-sjlj.c: Likewise. * gcc.target/i386/cet-switch-1.c: Likewise. * gcc.target/i386/cet-switch-2.c: Likewise. --- 0001-Enable-Control-flow-Enforcement-Technology-CET.patch Description: 0001-Enable-Control-flow-Enforcement-Technology-CET.patch
