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

[Jeff Law]

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