Message ID | Zhehj9nIxyGLoVHF@tucnak |
---|---|
State | New |
Headers | show |
Series | asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] | expand |
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 > >
> -----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
--- 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; +}