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

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

On Tue, Mar 12, 2024 at 8:00 PM liuhongt <hongtao.liu@intel.com> 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?
CC jakub, I see the code was added by
https://gcc.gnu.org/pipermail/gcc-patches/2018-December/512313.html
The issue in the PR is similar, but __m512 requires bigger
alignment(64 > ASAN_RED_ZONE_SIZE(32)), in that case we need to insert
MAX (alignb, ASAN_RED_ZONE_SIZE) instead of ASAN_RED_ZONE_SIZE.
Assume when alignb > ASAN_RED_ZONE_SIZE, it must be multiple of
ASAN_RED_ZONE_SIZE.
>
> 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,
> 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
>

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

> 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

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.

>
> > 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
>

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