Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling

[Jeff Law <law at redhat dot com>]

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-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
> 
> 
> From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist <igor.v.tsimbalist@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-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).
> 
> gcc/c-family/
> 
> 	* c-attribs.c (handle_notrack_attribute): New function.
> 	(c_common_attribute_table): Add 'notrack' handling.
> 
> gcc/
> 
> 	* cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK.
> 	* combine.c (distribute_notes): Add REG_CALL_NOTRACK handling.
> 	* common.opt: Add finstrument-control-flow flag.
> 	* emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling.
> 	* gimple.c (gimple_build_call_from_tree): Add 'notrack'
> 	propogation.
> 	* gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK.
> 	(gimple_call_with_notrack_p): function.
> 	(gimple_call_set_with_notrack): Likewise.
> 	* recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling.
> 	* reg-notes.def: Add REG_NOTE (CALL_NOTRACK).
> 	* toplev.c (process_options): Add flag_instrument_control_flow
> 	handling.
> ---
>  gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++
>  gcc/cfgexpand.c          | 11 +++++++++++
>  gcc/combine.c            |  1 +
>  gcc/common.opt           |  5 +++++
>  gcc/emit-rtl.c           |  1 +
>  gcc/gimple.c             | 17 +++++++++++++++++
>  gcc/gimple.h             | 34 ++++++++++++++++++++++++++++++++++
>  gcc/recog.c              |  1 +
>  gcc/reg-notes.def        |  7 +++++++
>  gcc/toplev.c             | 11 +++++++++++
>  10 files changed, 111 insertions(+)
> ---
>  gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++
>  gcc/cfgexpand.c          | 11 +++++++++++
>  gcc/combine.c            |  1 +
>  gcc/common.opt           |  5 +++++
>  gcc/emit-rtl.c           |  1 +
>  gcc/gimple.c             | 17 +++++++++++++++++
>  gcc/gimple.h             | 34 ++++++++++++++++++++++++++++++++++
>  gcc/recog.c              |  1 +
>  gcc/reg-notes.def        |  7 +++++++
>  gcc/toplev.c             | 11 +++++++++++
>  10 files changed, 111 insertions(+)
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 0d9ab2d..4d62f7c 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index c9d8118..ed344c5 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -2657,12 +2657,23 @@ expand_call_stmt (gcall *stmt)
>  	  }
>      }
>  
> +  rtx_insn *before_call = get_last_insn ();
>    lhs = gimple_call_lhs (stmt);
>    if (lhs)
>      expand_assignment (lhs, exp, false);
>    else
>      expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
>  
> +  /* Find a generated CALL insn to propagate a 'notrack' attribute.  */
> +  rtx_insn *last = get_last_insn ();
> +  while (!CALL_P (last)
> +	 && (last != before_call))
> +    last = PREV_INSN (last);
> +
> +  if (last != before_call
> +      && gimple_call_with_notrack_p (stmt))
> +    add_reg_note (last, REG_CALL_NOTRACK, const0_rtx);
> +
>    mark_transaction_restart_calls (stmt);
>  }
Just to be sure -- this is only searching through the currently open
sequence to find the RTL call emitted into the sequence to implement the
gimple call we're expanding, right?

ISTM that you don't need to do the search if
!gimple_call_with_notrack_p, right?  Which would imply this code should
look something like

if (gimple_call_with_notrack_p (stmt))
  {
    rtx insn *last = get_last_insn ();
    while (!CALL_P (last)
           && last != before_call)
      last = PREV_INSN (last);
    add_reg_note (last, REG_CALL_NOTRACK, const0_rtx)
  }

Or something close to that?


I'd be slightly concerned if expansion of an argument resulted in a
call.  I'm not sure if that can happen anymore, but if it does, then you
could end up marking the wrong CALL_INSN.

THe only cases where I think that might be possible would be if we
needed to emit a libcall or memcpy.  So perhaps you want to verify that
last is a call *and* that it's an indirect call.



> 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);
> +	}
> +    }
> +
>    return call;
As Richi noted, this deserves a better comment.

It may even be advisable to build a predicate to test if a given gimple
statement is an indirect call and use that rather than fndecl ==
NULL_TREE.  That would help make this code clearer as well.

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



>  }
>  
>  
> +/* 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<const gcall *> (gs);
> +  return gimple_call_with_notrack_p (gc);
> +}
Agree with Richi WRT avoiding gimple * overloads.


Jeff