public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)
@ 2024-03-12 11:57 liuhongt
  2024-03-13  1:27 ` Hongtao Liu
  2024-03-25 12:51 ` Jakub Jelinek
  0 siblings, 2 replies; 9+ messages in thread
From: liuhongt @ 2024-03-12 11:57 UTC (permalink / raw)
  To: gcc-patches; +Cc: crazylht, hjl.tools

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)
  2024-03-12 11:57 [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE) liuhongt
@ 2024-03-13  1:27 ` Hongtao Liu
  2024-03-25 12:51 ` Jakub Jelinek
  1 sibling, 0 replies; 9+ messages in thread
From: Hongtao Liu @ 2024-03-13  1:27 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches, hjl.tools

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
>


-- 
BR,
Hongtao

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)
  2024-03-12 11:57 [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE) liuhongt
  2024-03-13  1:27 ` Hongtao Liu
@ 2024-03-25 12:51 ` Jakub Jelinek
  2024-03-26  3:26   ` Hongtao Liu
  1 sibling, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-03-25 12:51 UTC (permalink / raw)
  To: liuhongt; +Cc: gcc-patches, crazylht, hjl.tools

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)
  2024-03-25 12:51 ` Jakub Jelinek
@ 2024-03-26  3:26   ` Hongtao Liu
  2024-03-26  3:34     ` Hongtao Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Hongtao Liu @ 2024-03-26  3:26 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: liuhongt, gcc-patches, hjl.tools

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
>


-- 
BR,
Hongtao

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE)
  2024-03-26  3:26   ` Hongtao Liu
@ 2024-03-26  3:34     ` Hongtao Liu
  2024-03-26  6:08       ` [PATCH V2] sanitizer: [PR110027] Align asan_vec[0] to MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) liuhongt
  0 siblings, 1 reply; 9+ messages in thread
From: Hongtao Liu @ 2024-03-26  3:34 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: liuhongt, gcc-patches, hjl.tools

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH V2] sanitizer: [PR110027] Align asan_vec[0] to MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE)
  2024-03-26  3:34     ` Hongtao Liu
@ 2024-03-26  6:08       ` liuhongt
  2024-04-11  8:38         ` [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: liuhongt @ 2024-03-26  6:08 UTC (permalink / raw)
  To: gcc-patches; +Cc: jakub

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

Ok?

gcc/ChangeLog:

	PR sanitizer/110027
	* cfgexpand.cc (expand_stack_vars): Align frame offset to
	MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE).

gcc/testsuite/ChangeLog:

	* g++.dg/asan/pr110027.C: New test.
---
 gcc/cfgexpand.cc                     |  3 ++-
 gcc/testsuite/g++.dg/asan/pr110027.C | 20 ++++++++++++++++++++
 2 files changed, 22 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..70cc89b29f4 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -1214,7 +1214,8 @@ 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 (BIGGEST_ALIGNMENT / BITS_PER_UNIT,
+					   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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]
  2024-03-26  6:08       ` [PATCH V2] sanitizer: [PR110027] Align asan_vec[0] to MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) liuhongt
@ 2024-04-11  8:38         ` Jakub Jelinek
  2024-04-11  8:53           ` Richard Biener
  2024-04-11 11:13           ` Liu, Hongtao
  0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2024-04-11  8:38 UTC (permalink / raw)
  To: Richard Biener, Jeff Law, liuhongt; +Cc: gcc-patches

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?

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]
  2024-04-11  8:38         ` [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] Jakub Jelinek
@ 2024-04-11  8:53           ` Richard Biener
  2024-04-11 11:13           ` Liu, Hongtao
  1 sibling, 0 replies; 9+ messages in thread
From: Richard Biener @ 2024-04-11  8:53 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Jeff Law, liuhongt, gcc-patches

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

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027]
  2024-04-11  8:38         ` [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] Jakub Jelinek
  2024-04-11  8:53           ` Richard Biener
@ 2024-04-11 11:13           ` Liu, Hongtao
  1 sibling, 0 replies; 9+ messages in thread
From: Liu, Hongtao @ 2024-04-11 11:13 UTC (permalink / raw)
  To: Jakub Jelinek, Richard Biener, Jeff Law; +Cc: gcc-patches



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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-04-11 11:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-12 11:57 [PATCH] sanitizer: [PR110027] Align asan_vec[0] to MAX (alignb, ASAN_RED_ZONE_SIZE) liuhongt
2024-03-13  1:27 ` Hongtao Liu
2024-03-25 12:51 ` Jakub Jelinek
2024-03-26  3:26   ` Hongtao Liu
2024-03-26  3:34     ` Hongtao Liu
2024-03-26  6:08       ` [PATCH V2] sanitizer: [PR110027] Align asan_vec[0] to MAX (BIGGEST_ALIGNMENT / BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) liuhongt
2024-04-11  8:38         ` [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] Jakub Jelinek
2024-04-11  8:53           ` Richard Biener
2024-04-11 11:13           ` Liu, Hongtao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).