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

[Maxim Ostapenko <m.ostapenko@samsung.com>]

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