Message ID | 20180412221606.GU8577@tucnak |
---|---|
State | New |
Headers | show |
Series | Fix -fsanitize=address VLA instrumentation (PR sanitizer/85230) | expand |
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 > > >
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
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
--- 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);