RE: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]
> -Original Message-
> From: Jakub Jelinek
> Sent: Thursday, April 11, 2024 4:39 PM
> To: Richard Biener ; Jeff Law ;
> Liu, Hongtao
> 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
Re: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]
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 >
