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: Tsimbalist, Igor V
> Sent: Friday, August 18, 2017 4:43 PM
> To: 'Richard Biener' <richard.guenther@gmail.com>
> Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com>
> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> 
> > -----Original Message-----
> > From: Richard Biener [mailto:richard.guenther@gmail.com]
> > Sent: Friday, August 18, 2017 3:53 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 Fri, Aug 18, 2017 at 3:11 PM, Tsimbalist, Igor V
> > <igor.v.tsimbalist@intel.com> wrote:
> > >> -----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/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
> > >> +            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.
> >
> > Hmm.  But what does this mean?  I guess direct calls are always
> > 'notrack' then and thus we're fine to ignore it.
> 
> Yes, that's correct - direct calls are always 'notrack' and we are ignoring it in
> calls.
> 
> > > 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?
> >
> > Yes, but note that the type on the gimple 'call' isn't yet correctly
> > set (only the caller does that).  The gimple_build_call_from_tree is
> > really an ugly thing and it should be privatized inside the gimplifier...
> 
> I'll look into this more carefully. It looks like it has to be rewritten as you
> suggested.
I've rewrote the code as you suggested. There are couple calls to gimple_build_call_from_tree from
c/gimple-parser.c. As I introduced an additional argument in gimple_build_call_from_tree I pass NULL
argument for these cases and check for NULL inside gimple_build_call_from_tree.

> > >> +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.
Fixed.

> > >> Please change the names to omit 'with_', thus just notrack and
> > >> GF_CALL_NOTRACK.
> > >
> > > Ok, I will rename.
Fixed.

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

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)?
> > >
> > > 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.
> >
> > Ok, so if we generate a special section with say 'cettable' as symbol
> > the verification runtime would be invoked at each indirect call site
> > with the destination address and it would search the table and abort
> > if the destination is not recorded.  We probably have to support a set
> > of CET tables and register them to support shared libraries (and cross
> shared object indirect calls).
> >
> > Do you plan to contribute sth like this?  Maybe within the sanitizer
> > framework?
> 
> I do not have plans for any sw based implementation. My plan is to introduce
> the concept in gcc and make its enabling for Intel CET. Even with my current
> plans more work needs to be done to completely enable CET.
> 
> Thanks,
> Igor
> 
> > Thanks,
> > Richard.
> >
> > > Thanks,
> > > Igor
> > >
> > >> Thanks,
> > >> Richard.