> -----Original Message-----
> From: Jakub Jelinek <ja...@redhat.com>
> Sent: Thursday, April 11, 2024 4:39 PM
> To: Richard Biener <rguent...@suse.de>; Jeff Law <jeffreya...@gmail.com>;
> Liu, Hongtao <hongtao....@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 <ja...@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
