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. > > > + */ (*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 > > > directly. This routine > > > +@deftypefn {GIMPLE function} gcall *gimple_build_call_from_tree > > > +(tree call_expr, @ tree fnptrtype) Build a @code{GIMPLE_CALL} from > > > +a @code{CALL_EXPR} node. The arguments and the function are taken > > from > > > +the expression directly. The type is set from the second parameter > > > +passed by a caller. This routine > > > assumes that @code{call_expr} is already in GIMPLE form. That is, > > > its operands are GIMPLE values and the function call needs no > > > further simplification. All the call flags in @code{call_expr} are > > > copied over diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > > index > > > e4cacf2..578bc25 100644 > > > --- a/gcc/doc/invoke.texi > > > +++ b/gcc/doc/invoke.texi > > > @@ -460,6 +460,7 @@ Objective-C and Objective-C++ Dialects}. > > > -fchkp-check-read -fchkp-check-write -fchkp-store-bounds @gol > > > -fchkp-instrument-calls -fchkp-instrument-marked-only @gol > > > -fchkp-use-wrappers -fchkp-flexible-struct-trailing-arrays@gol > > > +-fcf- > > protection=@r{[}@var{full}|@var{branch}|@var{return}|@var{none}@ > > > +r{]} @gol > > > > Are full/branch/return/none supposed to be literal strings? @var is > > the wrong markup for that. > > Yes, these are string literals. @var is used a lot in text around my fixes > that's > why I used it. What's the right markup for an option's values? Is it @code? Or > fixed it like this > > -fcf-protection=@r{[}full@r{|}branch@r{|}return@r{|}none@r{]} @gol > > Is it ok? > > > > -fstack-protector -fstack-protector-all -fstack-protector-strong > > > @gol -fstack-protector-explicit -fstack-check @gol > > > -fstack-limit-register=@var{reg} -fstack-limit-symbol=@var{sym} > > > @gol @@ -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{[}@var{full}|@var{branch}|@var{return}|@var{none}@ > > > +r{]} > > > > Again, markup? > > Fixed as above. > > > > +@opindex fcf-protection > > > +Enable code instrumentation of control-flow transfers to increase a > > > +program security by checking a target address of control-flow > > > > s/a program/program/ > > s/checking a target address/checking that target addresses/ > > Fixed. > > > > +transfer instructions (such as indirect function call, function > > > +return, indirect jump) are valid targets. This prevents diverting > > > +the control > > > > s/are valid targets/are valid/ > > Fixed. > > > > +flow instructions from its original target address to a new > > > +undesigned > > > > s/a new undesigned/an unintended/ ?? > > (not sure what you're trying to say here). > > The point here is that an attacker can change a target address in a control > flow transfer instruction so the transfer will go not to the right address > but to > some undesigned/unexpected or wrong address. > > > > +target. This is intended to protect against such theats as > > > > s/theats/threats > > Fixed. > > > > +Return-oriented Programming (ROP), and similarly call/jmp-oriented > > > +programming (COP/JOP). > > > + > > > +Each compiler, which will support the control-flow instrumentation, > > > +is supposed to have its own target specific implementation of the > > > +control-flow instrumentation and in case of absence of such > > > +implementation the usage of @option{-fcf-protection} will cause an > > > +error message. > > > > That whole paragraph is very long-winded. Are you trying to say here > > > > Currently GCC only supports this option on [...] targets. > > > > ?? > > How about this wording: > > 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. > > ? > > > > + > > > +The value @var{branch} tells the compiler to implement checking of > > > > Again, wrong markup? If it's a literal, use @code{branch}. > > Fixed. > > > > +validness of control-flow trasfer for at the point of indirect > > > +branch > > > > s/validness/validity/ > > s/trasfer for/transfer/ > > Fixed. > > > > +instructions, i.e. call/jmp instructions. The value @var{return} > > > > Again, wrong use of @var markup? > > Fixed. > > > > +implements checking of validness at the point of returning from a > > > +function. The value @var{full} is an alias for specifying both > > > +'branch' and 'return'. The value @var{none} turns off instrumentation. > > > > Here too. And do not use literal quotes for markup. > > Fixed. > > > > +This value may be used for those architectures where > > > +@option{-fcf-protection} is switched on by default. > > > > Which architectures are those? > > These are future architectures. I've changed 'those' to 'future'. > > > > +A user also has a control through the @code{nocf_check} attribute > > > +to identify > > > > Users are the readers of this document and should be addressed directly: > > > > You can also use the @code{nocf_check} attribute.... > > Fixed. > > > > +which functions and calls should be skipped from instrumentation. > > > + > > > > Please add a cross-reference here. > > Fixed. > > > > @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..f023067 100644 > > > --- a/gcc/doc/rtl.texi > > > +++ b/gcc/doc/rtl.texi > > > @@ -4040,6 +4040,21 @@ 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} describe the information > > > +connected to the code instrumentation which is done when > > > +@code{-fcf-protection=branch} option is specified. > > > > Hmmm, how about rewriting this as > > > > The note @code{REG_CALL_NOCF_CHECK} is used in conjunction with the > > @option{-fcf-protection=branch} option. > > Good. Agree and fixed. > > > > The note is set if a @code{nocf_check} attribute is > > > +specified. The note is stored in the @code{REG_NOTES} field of an insn. > > > > > +@table @code > > > +@findex REG_CALL_NOCF_CHECK > > > +@item REG_CALL_NOCF_CHECK > > > +A user has a control through the @code{nocf_check} attribute to > > > +identify which call to a function should be skipped from > > > +control-flow instrumentation when the option > > > +@code{-fcf-protection=branch} is specified. The compiler > > > > @option markup instead of @code on options, please. > > Fixed. > > > > +puts a @samp{REG_CALL_NO_VERIFY} note on @samp{CALL_INSN} > > > +instruction, which has a function type marked with a > > > +@code{nocf_check} > > attribute. > > > +@end table > > > + > > > For convenience, the machine mode in an @code{insn_list} or > > > @code{expr_list} is printed using these symbolic codes in debugging > > dumps. > > > > > > -- > > > 1.8.3.1 > > > > > > > -Sandra
0002-Add-documentation-for-fcf-protection-option-and-nocf.patch
Description: 0002-Add-documentation-for-fcf-protection-option-and-nocf.patch
