> -----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
