[PATCH] Fix -fsanitize=address VLA instrumentation (PR sanitizer/85230)

[Maxim Ostapenko]

On 04/13/2018 01:16 AM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, we need to unpoison the red zones when leaving a
> scope with VLA variable(s); this is done through __asan_allocas_unpoison
> call, unfortunately it is called after the __builtin_stack_restore which
> restores the stack pointer; now, if an interrupt comes in between the stack
> restore and the __asan_allocas_unpoison call, the interrupt handler might
> have some stack bytes marked as red zones in the shadow memory and might
> diagnose sanitizing error even when there is none in the original program.
>
> The following patch ought to fix this by swapping the two calls, so we first
> unpoison and only after it is unpoisoned in shadow memory release the stack.
> The second argument to the __asan_allocas_unpoison call is meant to
> be virtual_dynamic_stack_rtx after the __builtin_stack_restore, i.e. the new
> stack_pointer_rtx value + STACK_DYNAMIC_OFFSET (current_function_decl).
> As the STACK_DYNAMIC_OFFSET value isn't known until the vregs pass, the code
> used a hack where it ignored the second argument and replaced it by
> virtual_dynamic_stack_rtx.  With the asan.c change below this doesn't work
> anymore, because virtual_dynamic_stack_rtx aka stack_pointer_rtx +
> STACK_DYNAMIC_OFFSET (current_function_decl) before the
> __builtin_stack_restore is a different value.  The patch instead uses the
> argument passed to the __asan_allocas_unpoison at GIMPLE time, which is the
> same as passed to __builtin_stack_restore; this is the new stack_pointer_rtx
> value after __builtin_stack_restore.  And, because we don't want that value,
> but that + STACK_DYNAMIC_OFFSET (current_function_decl), we compute
> arg1 + (virtual_dynamic_stack_rtx - stack_pointer_rtx) and let CSE/combiner
> optimize it into arg1 (on targets like x86_64 where STACK_DYNAMIC_OFFSET can
> be even 0 when not accumulating outgoing args or when that size is 0) or
> arg1 + some_constant.
>
> Bootstrapped on
> {x86_64,i686,powerpc64,powerpc64le,aarch64,s390x,armv7hl}-linux, regtested
> on {x86_64,i686,powerpc64,powerpc64le}-linux so far, but on the power* ones
> on virtual address space size that isn't really supported (likely
> https://github.com/google/sanitizers/issues/933#issuecomment-380058705
> issue, so while nothing regresses there, pretty much all asan tests fail
> there before and after the patch); also tested successfully with
> asan.exp=alloca* on gcc110 and gcc112 on compile farm where it doesn't
> suffer from the VA issue.  Ok if testing passes also on aarch64, s390x
> and armv7hl?

OK with me, thanks.

-Maxim

> 2018-04-12  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR sanitizer/85230
> 	* asan.c (handle_builtin_stack_restore): Adjust comment.  Emit
> 	__asan_allocas_unpoison call and last_alloca_addr = new_sp before
> 	__builtin_stack_restore rather than after it.
> 	* builtins.c (expand_asan_emit_allocas_unpoison): Pass
> 	arg1 + (virtual_dynamic_stack_rtx - stack_pointer_rtx) as second
> 	argument instead of virtual_dynamic_stack_rtx.
>
> --- gcc/asan.c.jj	2018-01-09 21:53:38.821577722 +0100
> +++ gcc/asan.c	2018-04-12 13:22:59.166095523 +0200
> @@ -554,14 +554,14 @@ get_last_alloca_addr ()
>     return last_alloca_addr;
>   }
>   
> -/* Insert __asan_allocas_unpoison (top, bottom) call after
> +/* Insert __asan_allocas_unpoison (top, bottom) call before
>      __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 = new_sp;
>        __asan_allocas_unpoison (top, bot);
>        last_alloca_addr = new_sp;
> +     __builtin_stack_restore (new_sp);
>      In general, we can't use new_sp as bot parameter because on some
>      architectures SP has non zero offset from dynamic stack area.  Moreover, on
>      some architectures this offset (STACK_DYNAMIC_OFFSET) becomes known for each
> @@ -570,9 +570,8 @@ get_last_alloca_addr ()
>      http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#DYNAM-STACK.
>      To overcome the issue we use following trick: pass new_sp as a second
>      parameter to __asan_allocas_unpoison and rewrite it during expansion with
> -   virtual_dynamic_stack_rtx later in expand_asan_emit_allocas_unpoison
> -   function.
> -*/
> +   new_sp + (virtual_dynamic_stack_rtx - sp) later in
> +   expand_asan_emit_allocas_unpoison function.  */
>   
>   static void
>   handle_builtin_stack_restore (gcall *call, gimple_stmt_iterator *iter)
> @@ -584,9 +583,9 @@ handle_builtin_stack_restore (gcall *cal
>     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, restored_stack);
> -  gsi_insert_after (iter, g, GSI_NEW_STMT);
> +  gsi_insert_before (iter, g, GSI_SAME_STMT);
>     g = gimple_build_assign (last_alloca, restored_stack);
> -  gsi_insert_after (iter, g, GSI_NEW_STMT);
> +  gsi_insert_before (iter, g, GSI_SAME_STMT);
>   }
>   
>   /* Deploy and poison redzones around __builtin_alloca call.  To do this, we
> --- gcc/builtins.c.jj	2018-04-04 21:33:20.530639395 +0200
> +++ gcc/builtins.c	2018-04-12 13:35:34.328395156 +0200
> @@ -5068,18 +5068,24 @@ expand_builtin_alloca (tree exp)
>     return result;
>   }
>   
> -/* Emit a call to __asan_allocas_unpoison call in EXP.  Replace second argument
> -   of the call with virtual_stack_dynamic_rtx because in asan pass we emit a
> -   dummy value into second parameter relying on this function to perform the
> -   change.  See motivation for this in comment to handle_builtin_stack_restore
> -   function.  */
> +/* Emit a call to __asan_allocas_unpoison call in EXP.  Add to second argument
> +   of the call virtual_stack_dynamic_rtx - stack_pointer_rtx, which is the
> +   STACK_DYNAMIC_OFFSET value.  See motivation for this in comment to
> +   handle_builtin_stack_restore function.  */
>   
>   static rtx
>   expand_asan_emit_allocas_unpoison (tree exp)
>   {
>     tree arg0 = CALL_EXPR_ARG (exp, 0);
> +  tree arg1 = CALL_EXPR_ARG (exp, 1);
>     rtx top = expand_expr (arg0, NULL_RTX, ptr_mode, EXPAND_NORMAL);
> -  rtx bot = convert_memory_address (ptr_mode, virtual_stack_dynamic_rtx);
> +  rtx bot = expand_expr (arg1, NULL_RTX, ptr_mode, EXPAND_NORMAL);
> +  rtx off = expand_simple_binop (Pmode, MINUS, virtual_stack_dynamic_rtx,
> +				 stack_pointer_rtx, NULL_RTX, 0,
> +				 OPTAB_LIB_WIDEN);
> +  off = convert_modes (ptr_mode, Pmode, off, 0);
> +  bot = expand_simple_binop (ptr_mode, PLUS, bot, off, NULL_RTX, 0,
> +			     OPTAB_LIB_WIDEN);
>     rtx ret = init_one_libfunc ("__asan_allocas_unpoison");
>     ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode,
>   				 top, ptr_mode, bot, ptr_mode);
>
> 	Jakub
>
>
>