Re: [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)

[Hongtao Liu <crazylht@gmail.com>]

On Tue, Mar 26, 2024 at 11:26 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Mon, Mar 25, 2024 at 8:51 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Tue, Mar 12, 2024 at 07:57:59PM +0800, liuhongt wrote:
> > > if alignb > ASAN_RED_ZONE_SIZE and offset[0] is not multiple of
> > > alignb. (base_align_bias - base_offset) may not aligned to alignb, and
> > > caused segement fault.
> > >
> > > Bootstrapped and regtested on x86_64-linux-gnu{-m32,}.
> > > Ok for trunk and backport to GCC13?
> > >
> > > gcc/ChangeLog:
> > >
> > >       PR sanitizer/110027
> > >       * cfgexpand.cc (expand_stack_vars): Align frame offset to
> > >       MAX (alignb, ASAN_RED_ZONE_SIZE).
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >       * g++.dg/asan/pr110027.C: New test.
> > > ---
> > >  gcc/cfgexpand.cc                     |  2 +-
> > >  gcc/testsuite/g++.dg/asan/pr110027.C | 20 ++++++++++++++++++++
> > >  2 files changed, 21 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/g++.dg/asan/pr110027.C
> > >
> > > diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> > > index 0de299c62e3..92062378d8e 100644
> > > --- a/gcc/cfgexpand.cc
> > > +++ b/gcc/cfgexpand.cc
> > > @@ -1214,7 +1214,7 @@ expand_stack_vars (bool (*pred) (size_t), class stack_vars_data *data)
> > >           {
> > >             if (data->asan_vec.is_empty ())
> > >               {
> > > -               align_frame_offset (ASAN_RED_ZONE_SIZE);
> > > +               align_frame_offset (MAX (alignb, ASAN_RED_ZONE_SIZE));
> > >                 prev_offset = frame_offset.to_constant ();
> > >               }
> > >             prev_offset = align_base (prev_offset,
> >
> > This doesn't look correct to me.
> > The above is done just once for the first var partition.  And
> > var partitions are sorted by stack_var_cmp, which puts > MAX_SUPPORTED_STACK_ALIGNMENT
> > alignment vars first (that should be none on x86, the above is quite huge
> > alignment), then on size decreasing and only after that on alignment
> > decreasing.
> >
> > 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 <= 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
> MAX_SUPPORTED_STACK_ALIGNMENT / BITS_PER_UNIT.
> So I think we need to use MAX (MAX_SUPPORTED_STACK_ALIGNMENT /
> BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) for the first var partition.
It should be MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE).
MAX_SUPPORTED_STACK_ALIGNMENT is huge.
>
> >
> > > diff --git a/gcc/testsuite/g++.dg/asan/pr110027.C b/gcc/testsuite/g++.dg/asan/pr110027.C
> > > new file mode 100644
> > > index 00000000000..0067781bc89
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/asan/pr110027.C
> > > @@ -0,0 +1,20 @@
> > > +/* PR sanitizer/110027 */
> > > +/* { dg-do run } */
> > > +/* { dg-require-effective-target avx512f_runtime } */
> > > +/* { dg-options "-std=gnu++23 -mavx512f -fsanitize=address -O0 -g -fstack-protector-strong" } */
> > > +
> > > +#include <cstddef>
> > > +#include <cstdint>
> > > +
> > > +template <ptrdiff_t W, typename T>
> > > +using Vec [[gnu::vector_size(W * sizeof(T))]] = T;
> > > +
> > > +auto foo() {
> > > +  Vec<8, int64_t> ret{};
> > > +  return ret;
> > > +}
> > > +
> > > +int main() {
> > > +  foo();
> > > +  return 0;
> > > +}
> > > --
> > > 2.31.1
> >
> >         Jakub
> >
>
>
> --
> BR,
> Hongtao



-- 
BR,
Hongtao