Re: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]

[Richard Biener <rguenther@suse.de>]

On Thu, 11 Apr 2024, Jakub Jelinek wrote:

> On Tue, Mar 26, 2024 at 02:08:02PM +0800, liuhongt wrote:
> > > > So, try to add some other variable with larger size and smaller alignment
> > > > to the frame (and make sure it isn't optimized away).
> > > >
> > > > alignb above is the alignment of the first partition's var, if
> > > > align_frame_offset really needs to depend on the var alignment, it probably
> > > > should be the maximum alignment of all the vars with alignment
> > > > alignb * BITS_PER_UNIT <=3D MAX_SUPPORTED_STACK_ALIGNMENT
> > > >
> > 
> > In asan_emit_stack_protection, when it allocated fake stack, it assume
> > bottom of stack is also aligned to alignb. And the place violated this
> > is the first var partition. which is 32 bytes offsets,  it should be
> > BIGGEST_ALIGNMENT / BITS_PER_UNIT.
> > So I think we need to use MAX (BIGGEST_ALIGNMENT /
> > BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) for the first var partition.
> 
> Your first patch aligned offsets[0] to maximum of alignb and
> ASAN_RED_ZONE_SIZE.  But as I wrote in the reply to that mail, alignb there
> is the alignment of just a single variable which is the first one to appear
> in the sorted list and is placed in the highest spot in the stack frame. 
> That is not necessarily the largest alignment, the sorting ensures that it
> is a variable with the largest size in the frame (and only if several of
> them have equal size, largest alignment from the same sized ones).  Your
> second patch used maximum of BIGGEST_ALIGNMENT / BITS_PER_UNIT and
> ASAN_RED_ZONE_SIZE.  That doesn't change anything at all when using
> -mno-avx512f - offsets[0] is still just 32-byte aligned in that case
> relative to top of frame, just changes the -mavx512f case to be 64-byte
> aligned offsets[0] (aka offsets[0] is then either 0 or -64 instead of either
> 0 or -32).  That will not help if any variable in the frame needs 128-byte,
> 256-byte, 512-byte ...  4096-byte alignment.  If you want to fix the bug in
> the spot you've touched, you'd need to walk all the
> stack_vars[stack_vars_sorted[si2]] for si2 [si + 1, n - 1] and for those
> where the loop would do anything (i.e.
> stack_vars[i2].representative == i2
> && TREE_CODE (decl2) == SSA_NAME
>    ? SA.partition_to_pseudo[var_to_partition (SA.map, decl2)] == NULL_RTX
>    : DECL_RTL (decl2) == pc_rtx
> and the pred applies (but that means also walking the earlier ones!
> because with -fstack-protector* the vars can be processed in several calls) and
> alignb2 * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT
> and compute maximum of those alignments.
> That maximum is already computed,
> data->asan_alignb = MAX (data->asan_alignb, alignb);
> computes that, but you get the final result only after you do all the
> expand_stack_vars calls.  You'd need to compute it before.
> 
> Though, that change would be still in the wrong place.
> The thing is, it would be a waste of the precious stack space when it isn't
> needed at all (e.g.  when asan will not at compile time do the use after
> return checking, or if it won't do it at runtime, or even if it will do at
> runtime it will waste the space on the stack).
> 
> The following patch fixes it solely for the __asan_stack_malloc_N
> allocations, doesn't enlarge unnecessarily further the actual stack frame.
> Because asan is only supported on FRAME_GROWS_DOWNWARD architectures
> (mips, rs6000 and xtensa are conditional FRAME_GROWS_DOWNWARD arches, which
> for -fsanitize=address or -fstack-protector* use FRAME_GROWS_DOWNWARD 1,
> otherwise 0, others supporting asan always just use 1), the assumption for
> the dynamic stack realignment is that the top of the stack frame (aka offset
> 0) is aligned to alignb passed to the function (which is the maximum of alignb
> of all the vars in the frame).  As checked by the assertion in the patch,
> offsets[0] is 0 most of the time and so that assumption is correct, the only
> case when it is not 0 is if -fstack-protector* is on together with
> -fsanitize=address and cfgexpand.cc (create_stack_guard) created a stack
> guard.  That is the only variable which is allocated in the stack frame
> right away, for all others with -fsanitize=address defer_stack_allocation
> (or -fstack-protector*) returns true and so they aren't allocated
> immediately but handled during the frame layout phases.  So, the original
> frame_offset of 0 is changed because of the stack guard to
> -pointer_size_in_bytes and later at the
>               if (data->asan_vec.is_empty ())
>                 {
>                   align_frame_offset (ASAN_RED_ZONE_SIZE);
>                   prev_offset = frame_offset.to_constant ();
>                 }
> to -ASAN_RED_ZONE_SIZE.  The asan_emit_stack_protection code wasn't
> taking this into account though, so essentially assumed in the
> __asan_stack_malloc_N allocated memory it needs to align it such that
> pointer corresponding to offsets[0] is alignb aligned.  But that isn't
> correct if alignb > ASAN_RED_ZONE_SIZE, in that case it needs to ensure that
> pointer corresponding to frame offset 0 is alignb aligned.
> 
> The following patch fixes that.  Unlike the previous case where
> we knew that asan_frame_size + base_align_bias falls into the same bucket
> as asan_frame_size, this isn't in some cases true anymore, so the patch
> recomputes which bucket to use and if going to bucket 11 (because there is
> no __asan_stack_malloc_11 function in the library) disables the after return
> sanitization.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM.

Thanks,
Richard.

> 2024-04-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/110027
> 	* asan.cc (asan_emit_stack_protection): Assert offsets[0] is
> 	zero if there is no stack protect guard, otherwise
> 	-ASAN_RED_ZONE_SIZE.  If alignb > ASAN_RED_ZONE_SIZE and there is
> 	stack pointer guard, take the ASAN_RED_ZONE_SIZE bytes allocated at
> 	the top of the stack into account when computing base_align_bias.
> 	Recompute use_after_return_class from asan_frame_size + base_align_bias
> 	and set to -1 if that would overflow to 11.
> 
> 	* gcc.dg/asan/pr110027.c: New test.
> 
> --- gcc/asan.cc.jj	2024-04-10 09:54:39.661231059 +0200
> +++ gcc/asan.cc	2024-04-10 12:12:11.337978004 +0200
> @@ -1911,19 +1911,39 @@ asan_emit_stack_protection (rtx base, rt
>      }
>    str_cst = asan_pp_string (&asan_pp);
>  
> +  gcc_checking_assert (offsets[0] == (crtl->stack_protect_guard
> +				      ? -ASAN_RED_ZONE_SIZE : 0));
>    /* Emit the prologue sequence.  */
>    if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase
>        && param_asan_use_after_return)
>      {
> +      HOST_WIDE_INT adjusted_frame_size = asan_frame_size;
> +      /* The stack protector guard is allocated at the top of the frame
> +	 and cfgexpand.cc then uses align_frame_offset (ASAN_RED_ZONE_SIZE);
> +	 while in that case we can still use asan_frame_size, we need to take
> +	 that into account when computing base_align_bias.  */
> +      if (alignb > ASAN_RED_ZONE_SIZE && crtl->stack_protect_guard)
> +	adjusted_frame_size += ASAN_RED_ZONE_SIZE;
>        use_after_return_class = floor_log2 (asan_frame_size - 1) - 5;
>        /* __asan_stack_malloc_N guarantees alignment
>  	 N < 6 ? (64 << N) : 4096 bytes.  */
>        if (alignb > (use_after_return_class < 6
>  		    ? (64U << use_after_return_class) : 4096U))
>  	use_after_return_class = -1;
> -      else if (alignb > ASAN_RED_ZONE_SIZE && (asan_frame_size & (alignb - 1)))
> -	base_align_bias = ((asan_frame_size + alignb - 1)
> -			   & ~(alignb - HOST_WIDE_INT_1)) - asan_frame_size;
> +      else if (alignb > ASAN_RED_ZONE_SIZE
> +	       && (adjusted_frame_size & (alignb - 1)))
> +	{
> +	  base_align_bias
> +	    = ((adjusted_frame_size + alignb - 1)
> +	       & ~(alignb - HOST_WIDE_INT_1)) - adjusted_frame_size;
> +	  use_after_return_class
> +	    = floor_log2 (asan_frame_size + base_align_bias - 1) - 5;
> +	  if (use_after_return_class > 10)
> +	    {
> +	      base_align_bias = 0;
> +	      use_after_return_class = -1;
> +	    }
> +	}
>      }
>  
>    /* Align base if target is STRICT_ALIGNMENT.  */
> --- gcc/testsuite/gcc.dg/asan/pr110027.c.jj	2024-04-10 12:01:19.939768472 +0200
> +++ gcc/testsuite/gcc.dg/asan/pr110027.c	2024-04-10 12:11:52.728229147 +0200
> @@ -0,0 +1,50 @@
> +/* PR middle-end/110027 */
> +/* { dg-do run } */
> +/* { dg-additional-options "-fstack-protector-strong" { target fstack_protector } } */
> +/* { dg-set-target-env-var ASAN_OPTIONS "detect_stack_use_after_return=1" } */
> +
> +struct __attribute__((aligned (128))) S { char s[128]; };
> +struct __attribute__((aligned (64))) T { char s[192]; };
> +struct __attribute__((aligned (32))) U { char s[256]; };
> +struct __attribute__((aligned (64))) V { char s[320]; };
> +struct __attribute__((aligned (128))) W { char s[512]; };
> +
> +__attribute__((noipa)) void
> +foo (void *p, void *q, void *r, void *s)
> +{
> +  if (((__UINTPTR_TYPE__) p & 31) != 0
> +      || ((__UINTPTR_TYPE__) q & 127) != 0
> +      || ((__UINTPTR_TYPE__) r & 63) != 0)
> +    __builtin_abort ();
> +  (void *) s;
> +}
> +
> +__attribute__((noipa)) int
> +bar (void)
> +{
> +  struct U u;
> +  struct S s;
> +  struct T t;
> +  char p[4];
> +  foo (&u, &s, &t, &p);
> +  return 42;
> +}
> +
> +__attribute__((noipa)) int
> +baz (void)
> +{
> +  struct W w;
> +  struct U u;
> +  struct V v;
> +  char p[4];
> +  foo (&u, &w, &v, &p);
> +  return 42;
> +}
> +
> +int
> +main ()
> +{
> +  bar ();
> +  baz ();
> +  return 0;
> +}
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)