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