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

["Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>]

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, August 15, 2017 3:43 PM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@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.tsimbalist@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<const gcall *> (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 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.

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

Yes, your idea looks right. Microsoft has published the option /guard:cf and according to the description it uses similar idea of gathering information about control-transfer targets. This info is kept in the binary. And 'yes' for last two your questions.

Thanks,
Igor

> Thanks,
> Richard.