Re: [PATCH, updated] Vtable pointer verification, main gcc changes (patch 2 of 3)

[Caroline Tice <cmtice@google.com>]

On Wed, Mar 13, 2013 at 10:48 AM, Diego Novillo <dnovillo@google.com> wrote:
>
> On 2013-02-25 14:27 , Caroline Tice wrote:
>
>> Index: libgcc/Makefile.in
>> ===================================================================
>> --- libgcc/Makefile.in    (revision 196266)
>> +++ libgcc/Makefile.in    (working copy)
>> @@ -22,6 +22,7 @@
>>  libgcc_topdir = @libgcc_topdir@
>>  host_subdir = @host_subdir@
>>
>> +gcc_srcdir = $(libgcc_topdir)/gcc
>>  gcc_objdir = $(MULTIBUILDTOP)../../$(host_subdir)/gcc
>>
>>  srcdir = @srcdir@
>> @@ -969,6 +970,16 @@ crtendS$(objext): $(srcdir)/crtstuff.c
>>  # This is a version of crtbegin for -static links.
>>  crtbeginT$(objext): $(srcdir)/crtstuff.c
>>      $(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_BEGIN -DCRTSTUFFT_O
>> +
>> +# These are used in vtable verification; see comments in source files for
>> +# more details.
>> +vtv_start$(objext): $(gcc_srcdir)/vtv_start.c
>> +    $(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
>> +      -c $(gcc_srcdir)/vtv_start.c
>> +
>> +vtv_end$(objext): $(gcc_srcdir)/vtv_end.c
>> +    $(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
>> +      -c $(gcc_srcdir)/vtv_end.c
>>  endif
>
>
> Why not have these two files in libgcc?  I don't think we want to depend on source files in gcc/ from libgcc/
>

I will put them there.

>>
>>
>>  /* IPA Passes */
>>  extern struct simple_ipa_opt_pass pass_ipa_lower_emutls;
>> Index: gcc/tree-vtable-verify.c
>> ===================================================================
>> --- gcc/tree-vtable-verify.c    (revision 0)
>> +++ gcc/tree-vtable-verify.c    (revision 0)
>
> I would like to get rid of this naming convention where we prefix file names with 'tree-'.  Just vtable-verify.c is fine.
>
>> @@ -0,0 +1,724 @@
>> +/*   Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
>
> Copyright (C) 2013
>
>>
>> +/* Virtual Table Pointer Security Pass - Detect corruption of vtable pointers
>> +   before using them for virtual method dispatches.  */
>
>
> Do you have a URL to a paper/presentation for it?

No, I don't at this time.

>
>> +
>> +  To find and reference the set of valid vtable pointers for any given
>> +  virtual class, we create a special global varible for each virtual
>
> s/varible/variable/
>
>>
>> +
>> +  Some implementation details for this pass:
>> +
>> +  To find the all of the virtual calls, we iterate through all the
>
> s/the all/all/
>>
>>
>> +  struct vtable_registration key;
>> +  struct vtable_registration **slot;
>> +
>> +  gcc_assert (node && node->registered);
>> +
>> +  key.vtable_decl = vtable_decl;
>> +  slot = (struct vtable_registration **) htab_find_slot (node->registered,
>> + &key, NO_INSERT);
>
> Unless you need to use this in an old branch, I strongly suggest using the new hash table facilities (hash-table.h).
>

Ok, will do.

>> +
>> +
>> +/* Here are the three two structures into which we insert vtable map nodes.
>
> 'three two'?
>>
>>
>> +  /* Verify that there aren't any qualifiers on the type.  */
>> +  type_quals = TYPE_QUALS (TREE_TYPE (class_type_decl));
>> +  gcc_assert (type_quals == TYPE_UNQUALIFIED);
>> +
>> +  /* Get the mangled name for the unqualified type.  */
>> +  class_name = DECL_ASSEMBLER_NAME (class_type_decl);
>
>
> DECL_ASSEMBLER_NAME has side-effects (it generates one if there isn't one already).  Just to avoid this unwanted side effect, protect it with if (HAS_DECL_ASSEMBLER_NAME_P).  In fact, I think you should abort if the class_type_decl does *not* have one.  So, just adding gcc_assert (HAS_DECL_ASSEMBLER_NAME_P(class_type_decl)) should be sufficient.
>

Will do.

>>
>> +
>> +/* This function attempts to recover the declared class of an object
>> +   that is used in making a virtual call.  We try to get the type from
>> +   the type cast in the gimple assignment statement that extracts the
>> +   vtable pointer from the object (DEF_STMT).  The gimple statment
>
> s/statment/statement/
>>
>> +   usually looks something like this:
>> +
>> +   D.2201_4 = MEM[(struct Event *)this_1(D)]._vptr.Event    */
>> +
>> +static tree
>> +/* extract_object_class_type (gimple def_stmt) */
>
> Remove this?

Yes (I didn't realize I had left the commented out code in the patch;
I'm sorry about that).

>>
>> +extract_object_class_type (tree rhs)
>> +{
>> +  /* tree rhs = NULL_TREE; */
>> +
>> +  /* Try to find and extract the type cast from that stmt.  */
>> +
>> +  /* rhs = gimple_assign_rhs1 (def_stmt); */
>> +  /*
>> +  if (TREE_CODE (rhs) == COMPONENT_REF)
>> +    {
>> +      while (TREE_CODE (rhs) == COMPONENT_REF
>> +             && (TREE_CODE (TREE_OPERAND (rhs, 0)) == COMPONENT_REF))
>> +        rhs = TREE_OPERAND (rhs, 0);
>> +
>> +      if (TREE_CODE (rhs) == COMPONENT_REF
>> +          && TREE_CODE (TREE_OPERAND (rhs, 0)) == MEM_REF
>> +          && TREE_CODE (TREE_TYPE (TREE_OPERAND (rhs, 0)))== RECORD_TYPE)
>> +        rhs = TREE_TYPE (TREE_OPERAND (rhs, 0));
>> +      else
>> +        rhs = NULL_TREE;
>> +    }
>> +  else
>> +    rhs = NULL_TREE;
>> +  */
>
>
> What's this?  Is it needed?  There's some other commented code here that seems out of place.
>

I will remove it.  I didn't mean for it to be in the patch.

>> +
>> +  tree result = NULL_TREE;
>> +
>> +
>> +  if (TREE_CODE (rhs) == COMPONENT_REF)
>> +    {
>> +      tree op0 = TREE_OPERAND (rhs, 0);
>> +      tree op1 = TREE_OPERAND (rhs, 1);
>> +
>> +      if (TREE_CODE (op1) == FIELD_DECL
>> +      && DECL_VIRTUAL_P (op1))
>> +    {
>> +      if (TREE_CODE (op0) == COMPONENT_REF
>> +          && TREE_CODE (TREE_OPERAND (op0, 0)) == MEM_REF
>> +          && TREE_CODE (TREE_TYPE (TREE_OPERAND (op0, 0)))== RECORD_TYPE)
>> +        result = TREE_TYPE (TREE_OPERAND (op0, 0));
>> +      else
>> +        result = TREE_TYPE (op0);
>> +    }
>> +      else if (TREE_CODE (op0) == COMPONENT_REF)
>> +    {
>> +      result = extract_object_class_type (op0);
>> +      if (result == NULL_TREE
>> +          && TREE_CODE (op1) == COMPONENT_REF)
>> +        result = extract_object_class_type (op1);
>> +    }
>> +    }
>
> Indentation seems off here.
>>
>> +
>> +  return result;
>> +}
>> +
>> +bool
>> +vtv_defuse_fn (tree var, gimple def_stmt, void *data)
>> +{
>> +  bool retval = false;
>> +
>> +  return retval;
>> +}
>
>
> Huh?
>

Another piece I meant to remove previously (sorry again).

>>
>> +  for (; !gsi_end_p (gsi_virtual_call); gsi_next (&gsi_virtual_call))
>> +    {
>> +      stmt = gsi_stmt (gsi_virtual_call);
>> +      if (is_vtable_assignment_stmt (stmt))
>> +        {
>> +          tree lhs = gimple_assign_lhs (stmt);
>> +          tree vtbl_var_decl = NULL_TREE;
>> +          struct vtbl_map_node *vtable_map_node;
>> +          tree vtbl_decl = NULL_TREE;
>> +          gimple call_stmt;
>> +          struct gimplify_ctx gctx;
>> +          const char *vtable_name = "<unknown>";
>> +          tree tmp0;
>> +          bool found;
>> +
>> +          gsi_vtbl_assign = gsi_for_stmt (stmt);
>
> Why not use 'gsi_virtual_call'?
>

I really do want to use two separate iterators here.  gsi_virtual_call
iterates linearly through all the statements in the basic block.
gsi_vtbl_assign gets moved around a bit, later down in the code, to
find the exact position for inserting the verification call.

>>
>> +                  if (TREE_CODE (vtbl_decl) == VAR_DECL)
>> +                    vtable_name = IDENTIFIER_POINTER (DECL_NAME (vtbl_decl));
>> +
>> +                  push_gimplify_context (&gctx);
>
> Not needed.  You are not calling the gimplifier.
>

Ok.

>>
>> +
>> +          if (!next_stmt)
>> +            {
>> +              pop_gimplify_context (NULL);
>
> Likewise.

Ok.

>
>>
>> +          gsi_insert_after (&gsi_vtbl_assign, call_stmt,
>> +                    GSI_NEW_STMT);
>> +
>> +                  pop_gimplify_context (NULL);
>
> Likewise.
>

Ok.
>
> Diego.