This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2][ASAN] Implement dynamic allocas/VLAs sanitization.​


Hi,

On 29/06/17 15:35, Jakub Jelinek wrote:
Hi!

Sorry for the review delay.

On Mon, Jun 26, 2017 at 03:49:23PM +0300, Maxim Ostapenko wrote:
	(handle_builtin_stackrestore): Likewise.
The function is called with _ between stack and restore.

	* match.pd: Add new pattern.
Unless the patch relies on this, I think it should be posted separately
and reviewed by Richard.

OK, good point, will remove from this patch.


@@ -245,6 +246,7 @@ along with GCC; see the file COPYING3.  If not see
  static unsigned HOST_WIDE_INT asan_shadow_offset_value;
  static bool asan_shadow_offset_computed;
  static vec<char *> sanitized_sections;
+static tree last_alloca_addr = NULL_TREE;
You are shadowing this variable in multiple places.  Either rename it to
something different, or rename the results of get_last_alloca_addr.
And the " = NULL_TREE" part is not needed.

Err, thanks, will fix it.


/* Set of variable declarations that are going to be guarded by
     use-after-scope sanitizer.  */
@@ -529,11 +531,183 @@ get_mem_ref_of_assignment (const gassign *assignment,
    return true;
  }
+/* Return address of last allocated dynamic alloca. */
+
+static tree
+get_last_alloca_addr ()
+{
+  if (last_alloca_addr)
+    return last_alloca_addr;
+
+  gimple_seq seq = NULL;
+  gassign *g;
+
+  last_alloca_addr = create_tmp_reg (ptr_type_node, "last_alloca_addr");
+  g = gimple_build_assign (last_alloca_addr, NOP_EXPR,
+			   build_int_cst (ptr_type_node, 0));
Instead of build_int_cst (ptr_type_node, 0) you should use
null_pointer_node.  And the NOP_EXPR there is just wrong, either it
should be gimple_build_assign (last_alloca_addr, null_pointer_node);
or gimple_build_assign (last_alloca_addr, INTEGER_CST, null_pointer_node);

+  gimple_seq_add_stmt_without_update (&seq, g);
Why the seq stuff at all?  You have a single stmt you want to insert on
edge.

Right, will fix it.

+
+  edge e = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun));
+  gsi_insert_seq_on_edge_immediate (e, seq);
So just use here
   gsi_insert_on_edge_immediate (e, g);
instead.

+  return last_alloca_addr;
+}
+
+/* Insert __asan_allocas_unpoison (top, bottom) call after
+   __builtin_stack_restore (new_sp) call.
+   The pseudocode of this routine should look like this:
+     __builtin_stack_restore (new_sp);
+     top = last_alloca_addr;
+     bot = virtual_dynamic_stack_rtx;
+     __asan_allocas_unpoison (top, bottom);
+     last_alloca_addr = new_sp;
The comment doesn't seem to agree with what you actually implement.
There is no virtual_dynamic_stack_rtx during the asan pass, it is there
only during expansion until the virtual regs are instantiated in the next
pass.  Furthermore, you have bot variable, but then use bottom.

Right, 'bottom' should be replaced by 'bot'.
Regarding to virtual_dynamic_stactk_rtx - as you correctly noted below, second parameter of __asan_allocas_unpoison will be completely rewritten in expand_builtin_alloca function by virtual_dynamic_stactk_rtx. Here I've just passed new_sp as a dummy argument. This looks hacky, but the problem is that for several architectures (e.g. PPC) we cannot use new_sp as a 'bot' parameter because new_sp != last_alloca_addr on these targets. Originally, I tried to do something like this:

  top = last_alloca_addr;
  bot = new_sp + STACK_DYNAMIC_OFFSET;
  __asan_allocas_unpoison(top, bot);
  last_alloca_addr = bot;

but I was confused by the fact that STACK_DYNAMIC_OFFSET becomes available only after expansion to RTL. Rewriting 'bot' with virtual_dynamic_stactk_rtx in RTL looks like the most feasible way how to overcome this issue for me.


+  tree last_alloca_addr = get_last_alloca_addr ();
Here is the shadowing I talked about.

+  tree restored_stack = gimple_call_arg (call, 0);
+  tree fn = builtin_decl_implicit (BUILT_IN_ASAN_ALLOCAS_UNPOISON);
+  gimple *g = gimple_build_call (fn, 2, last_alloca_addr, restored_stack);
Here you clearly use the first argument of __builtin_stack_restore, which
is that new_sp.

+  gimple_seq_add_stmt_without_update (&seq, g);
Why the messing up with sequences?  Just insert the stmt immediately in,
and the others as well.

+  g = gimple_build_assign (last_alloca_addr, NOP_EXPR, restored_stack);
This is again wrong, here you really don't know what restored_stack is,
it could be SSA_NAME, but also something different, so you should use
gimple_build_assign (last_alloca_addr, restored_stack);
and let it figure out the rhs code.

Thanks, will fix.


+  /* Extract lower bits from old_size.  */
+  wide_int size_nonzero_bits = get_nonzero_bits (old_size);
+  wide_int rz_mask
+    = wi::uhwi (redzone_mask, wi::get_precision (size_nonzero_bits));
+  wide_int old_size_lower_bits = wi::bit_and (size_nonzero_bits, rz_mask);
+
+  /* If alloca size is aligned to ASAN_RED_ZONE_SIZE, we don't need partial
+     redzone.  Otherwise, compute its size here.  */
+  if (wi::ne_p (old_size_lower_bits, 0))
+    {
+      /* misalign = size & (ASAN_RED_ZONE_SIZE - 1)
+         partial_size = ASAN_RED_ZONE_SIZE - misalign.  */
+      g = gimple_build_assign (make_ssa_name (size_type_node, NULL),
+			       BIT_AND_EXPR, old_size, alloca_rz_mask);
+      gimple_seq_add_stmt_without_update (&seq, g);
+      tree misalign = gimple_assign_lhs (g);
+      g = gimple_build_assign (make_ssa_name (size_type_node, NULL), MINUS_EXPR,
+			       redzone_size, misalign);
+      gimple_seq_add_stmt_without_update (&seq, g);
Again, why add the stmts into a seq first instead of just adding it
immediately into the IL?
@@ -4962,6 +4962,20 @@ expand_builtin_alloca (tree exp)
    return result;
  }
Missing function comment here.

+static rtx
+expand_asan_emit_allocas_unpoison (tree exp)
+{
+  tree arg0 = CALL_EXPR_ARG (exp, 0);
+  rtx top = expand_expr (arg0, NULL_RTX, GET_MODE (virtual_stack_dynamic_rtx),
+			 EXPAND_NORMAL);
+  rtx ret = init_one_libfunc ("__asan_allocas_unpoison");
+  ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, 2, top,
+				 TYPE_MODE (pointer_sized_int_node),
+				 virtual_stack_dynamic_rtx,
+				 TYPE_MODE (pointer_sized_int_node));
I see you are here pretty much ignoring the old second argument and instead
using a different one.  But then the above mentioned comment should explain
that, how you transform it during the asan pass and how you later change it
during expansion.

Yeah, I'll add a comment about motivation.


+    case BUILT_IN_ASAN_ALLOCAS_UNPOISON:
+      target = expand_asan_emit_allocas_unpoison (exp);
+      if (target)
+	return target;
Do you need this test, when it always returns non-NULL?
Just return expand_asan_emit_allocas_unpoison (exp);

	Jakub




-Maxim


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]