[PATCH] Fix -fsanitize=address with > 32 byte aligned vars (PR sanitizer/88619)

[PATCH] Fix -fsanitize=address with > 32 byte aligned vars (PR sanitizer/88619)

[Jakub Jelinek]

Hi!

The prev_offset we push into the vector is supposed to be the end of
the red zone for the variable, it needs to be ASAN_MIN_RED_ZONE_SIZE
aligned, but by aligning it more we can end up with red zone with negative
size.  E.g. on the testcase, we have initial frame_offset -8 (because of
-fstack-protector guard), we align it to -32 first (required that the
start and end of the var block is 32 byte aligned (4 byte aligned in shadow
memory)).  The first variable needs 64 byte alignment, but is just 4 bytes,
so the next after
  offset = alloc_stack_frame_space (size, alignment);
we end up with frame_offset (== offset) -64 and the start of red zone is
after that 4 byte variable, i.e. at -60.  But the align_base tweak from
-32 with 64 byte alignment gives -64.

Now, the stack vars are sorted first by large vs. small alignment (not
relevant to x86), then by size and only afterwards by alignment, so similar
problems could appear if we have some large variable followed by > 32 byte
aligned very small variable.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2019-01-03  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/88619
	* cfgexpand.c (expand_stack_vars): Only align prev_offset to
	ASAN_MIN_RED_ZONE_SIZE, not to maximum of that and alignb.

	* c-c++-common/asan/pr88619.c: New test.

--- gcc/cfgexpand.c.jj	2019-01-01 12:37:17.328972145 +0100
+++ gcc/cfgexpand.c	2019-01-03 14:11:16.209366858 +0100
@@ -1130,7 +1130,7 @@ expand_stack_vars (bool (*pred) (size_t)
 		  prev_offset = frame_offset.to_constant ();
 		}
 	      prev_offset = align_base (prev_offset,
-					MAX (alignb, ASAN_MIN_RED_ZONE_SIZE),
+					ASAN_MIN_RED_ZONE_SIZE,
 					!FRAME_GROWS_DOWNWARD);
 	      tree repr_decl = NULL_TREE;
 	      unsigned HOST_WIDE_INT size
--- gcc/testsuite/c-c++-common/asan/pr88619.c.jj	2019-01-03 14:16:00.779659964 +0100
+++ gcc/testsuite/c-c++-common/asan/pr88619.c	2019-01-03 14:15:40.568994254 +0100
@@ -0,0 +1,14 @@
+/* PR sanitizer/88619 */
+/* { dg-do compile { target fstack_protector } } */
+/* { dg-options "-fstack-protector-strong -fsanitize=address" } */
+
+typedef int A __attribute__((aligned (64)));
+
+int
+main ()
+{
+  A b;
+  int *p = &b;
+  *(p - 1) = 123;
+  __builtin_alloca (b);
+}

	Jakub

Re: [PATCH] Fix -fsanitize=address with > 32 byte aligned vars (PR sanitizer/88619)

[Martin Liška]

On 1/3/19 11:44 PM, Jakub Jelinek wrote:
> Hi!
> 
> The prev_offset we push into the vector is supposed to be the end of
> the red zone for the variable, it needs to be ASAN_MIN_RED_ZONE_SIZE
> aligned, but by aligning it more we can end up with red zone with negative
> size.  E.g. on the testcase, we have initial frame_offset -8 (because of
> -fstack-protector guard), we align it to -32 first (required that the
> start and end of the var block is 32 byte aligned (4 byte aligned in shadow
> memory)).  The first variable needs 64 byte alignment, but is just 4 bytes,
> so the next after
>   offset = alloc_stack_frame_space (size, alignment);
> we end up with frame_offset (== offset) -64 and the start of red zone is
> after that 4 byte variable, i.e. at -60.  But the align_base tweak from
> -32 with 64 byte alignment gives -64.

Thank you Jakub for the patch.
I was also considering adding a sanity check that will ensure
the offsets in data->asan_vec are always in increasing order.
What do you think about it?

> 
> Now, the stack vars are sorted first by large vs. small alignment (not
> relevant to x86), then by size and only afterwards by alignment, so similar
> problems could appear if we have some large variable followed by > 32 byte
> aligned very small variable.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

I can add here that asan.exp tests work both on ppc64le-linux-gnu and ppc64-linux-gnu.
I'm going to trigger asan-bootstrap on one of them.

Martin

> 
> 2019-01-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/88619
> 	* cfgexpand.c (expand_stack_vars): Only align prev_offset to
> 	ASAN_MIN_RED_ZONE_SIZE, not to maximum of that and alignb.
> 
> 	* c-c++-common/asan/pr88619.c: New test.
> 
> --- gcc/cfgexpand.c.jj	2019-01-01 12:37:17.328972145 +0100
> +++ gcc/cfgexpand.c	2019-01-03 14:11:16.209366858 +0100
> @@ -1130,7 +1130,7 @@ expand_stack_vars (bool (*pred) (size_t)
>  		  prev_offset = frame_offset.to_constant ();
>  		}
>  	      prev_offset = align_base (prev_offset,
> -					MAX (alignb, ASAN_MIN_RED_ZONE_SIZE),
> +					ASAN_MIN_RED_ZONE_SIZE,
>  					!FRAME_GROWS_DOWNWARD);
>  	      tree repr_decl = NULL_TREE;
>  	      unsigned HOST_WIDE_INT size
> --- gcc/testsuite/c-c++-common/asan/pr88619.c.jj	2019-01-03 14:16:00.779659964 +0100
> +++ gcc/testsuite/c-c++-common/asan/pr88619.c	2019-01-03 14:15:40.568994254 +0100
> @@ -0,0 +1,14 @@
> +/* PR sanitizer/88619 */
> +/* { dg-do compile { target fstack_protector } } */
> +/* { dg-options "-fstack-protector-strong -fsanitize=address" } */
> +
> +typedef int A __attribute__((aligned (64)));
> +
> +int
> +main ()
> +{
> +  A b;
> +  int *p = &b;
> +  *(p - 1) = 123;
> +  __builtin_alloca (b);
> +}
> 
> 	Jakub
> 

Re: [PATCH] Fix -fsanitize=address with > 32 byte aligned vars (PR sanitizer/88619)

[Martin Liška]

On 1/4/19 11:57 AM, Martin Liška wrote:
> I'm going to trigger asan-bootstrap on one of them.

Works for me on gcc112.fsffrance.org machine (ppc64le).

Martin

Re: [PATCH] Fix -fsanitize=address with > 32 byte aligned vars (PR sanitizer/88619)

[Richard Biener]

On Thu, 3 Jan 2019, Jakub Jelinek wrote:

> Hi!
> 
> The prev_offset we push into the vector is supposed to be the end of
> the red zone for the variable, it needs to be ASAN_MIN_RED_ZONE_SIZE
> aligned, but by aligning it more we can end up with red zone with negative
> size.  E.g. on the testcase, we have initial frame_offset -8 (because of
> -fstack-protector guard), we align it to -32 first (required that the
> start and end of the var block is 32 byte aligned (4 byte aligned in shadow
> memory)).  The first variable needs 64 byte alignment, but is just 4 bytes,
> so the next after
>   offset = alloc_stack_frame_space (size, alignment);
> we end up with frame_offset (== offset) -64 and the start of red zone is
> after that 4 byte variable, i.e. at -60.  But the align_base tweak from
> -32 with 64 byte alignment gives -64.
> 
> Now, the stack vars are sorted first by large vs. small alignment (not
> relevant to x86), then by size and only afterwards by alignment, so similar
> problems could appear if we have some large variable followed by > 32 byte
> aligned very small variable.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
OK.

Richard.

> 2019-01-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/88619
> 	* cfgexpand.c (expand_stack_vars): Only align prev_offset to
> 	ASAN_MIN_RED_ZONE_SIZE, not to maximum of that and alignb.
> 
> 	* c-c++-common/asan/pr88619.c: New test.
> 
> --- gcc/cfgexpand.c.jj	2019-01-01 12:37:17.328972145 +0100
> +++ gcc/cfgexpand.c	2019-01-03 14:11:16.209366858 +0100
> @@ -1130,7 +1130,7 @@ expand_stack_vars (bool (*pred) (size_t)
>  		  prev_offset = frame_offset.to_constant ();
>  		}
>  	      prev_offset = align_base (prev_offset,
> -					MAX (alignb, ASAN_MIN_RED_ZONE_SIZE),
> +					ASAN_MIN_RED_ZONE_SIZE,
>  					!FRAME_GROWS_DOWNWARD);
>  	      tree repr_decl = NULL_TREE;
>  	      unsigned HOST_WIDE_INT size
> --- gcc/testsuite/c-c++-common/asan/pr88619.c.jj	2019-01-03 14:16:00.779659964 +0100
> +++ gcc/testsuite/c-c++-common/asan/pr88619.c	2019-01-03 14:15:40.568994254 +0100
> @@ -0,0 +1,14 @@
> +/* PR sanitizer/88619 */
> +/* { dg-do compile { target fstack_protector } } */
> +/* { dg-options "-fstack-protector-strong -fsanitize=address" } */
> +
> +typedef int A __attribute__((aligned (64)));
> +
> +int
> +main ()
> +{
> +  A b;
> +  int *p = &b;
> +  *(p - 1) = 123;
> +  __builtin_alloca (b);
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)