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

["Liu, Hongtao" <hongtao.liu@intel.com>]

> -----Original Message-----
> From: Jakub Jelinek <jakub@redhat.com>
> Sent: Thursday, April 11, 2024 4:39 PM
> To: Richard Biener <rguenther@suse.de>; Jeff Law <jeffreyalaw@gmail.com>;
> Liu, Hongtao <hongtao.liu@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -
> fsanitize=address -fstack-protector* [PR110027]
> 
> 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.
Thanks for the detailed explanation, I understand now.
> 
> 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?
> 
> 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