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

Message ID 20180412221606.GU8577@tucnak
State New
Headers show
Series
  • Fix -fsanitize=address VLA instrumentation (PR sanitizer/85230)
Related show

Commit Message

Jakub Jelinek April 12, 2018, 10:16 p.m.
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?

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.


	Jakub

Comments

Maxim Ostapenko April 13, 2018, 9:06 a.m. | #1
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
>
>
>
Jakub Jelinek April 13, 2018, 4:42 p.m. | #2
On Fri, Apr 13, 2018 at 12:16:06AM +0200, Jakub Jelinek wrote:
> 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?

Passed regtest also on aarch64, s390x and armv7hl, additionally with the
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85394#c2 patch on top of this
patch bootstrapped/regtested on powerpc64{,le}-linux, this time without
any asan related issues.

Ok for trunk?

	Jakub
Jeff Law April 17, 2018, 5:37 p.m. | #3
On 04/12/2018 04:16 PM, 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?
> 
> 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.
OK.

jeff

Patch

--- 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);