[PATCH] Invalid unpoisoning of stack redzones on ARM

[PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

Hi all,

I've recently submitted a bug report regarding invalid unpoisoning of 
stack frame redzones 
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone take 
a look at proposed patch (a simple one-liner) and check whether it's ok 
for commit?

Thanks!

-Yuri

[-- Attachment #2: fix_reg_alias.diff --]
[-- Type: text/x-patch, Size: 520 bytes --]

diff --git a/gcc/asan.c b/gcc/asan.c
index 32f1837..acb00ea 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -895,7 +895,7 @@ asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
 
   gcc_assert ((len & 3) == 0);
   top_label = gen_label_rtx ();
-  addr = force_reg (Pmode, XEXP (shadow_mem, 0));
+  addr = copy_to_reg (force_reg (Pmode, XEXP (shadow_mem, 0)));
   shadow_mem = adjust_automodify_address (shadow_mem, SImode, addr, 0);
   end = force_reg (Pmode, plus_constant (Pmode, addr, len));
   emit_label (top_label);

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Jakub Jelinek]

On Fri, Sep 27, 2013 at 06:10:41PM +0400, Yury Gribov wrote:
> Hi all,
> 
> I've recently submitted a bug report regarding invalid unpoisoning
> of stack frame redzones
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone
> take a look at proposed patch (a simple one-liner) and check whether
> it's ok for commit?

Can you please be more verbose on why do you think it is the right fix,
what exactly is the problem and why force_reg wasn't sufficient?
What exactly was XEXP (shadow_mem, 0) that force_reg didn't force it into
a pseudo?

Also, you are missing a ChangeLog entry.

> diff --git a/gcc/asan.c b/gcc/asan.c
> index 32f1837..acb00ea 100644
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -895,7 +895,7 @@ asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
>  
>    gcc_assert ((len & 3) == 0);
>    top_label = gen_label_rtx ();
> -  addr = force_reg (Pmode, XEXP (shadow_mem, 0));
> +  addr = copy_to_reg (force_reg (Pmode, XEXP (shadow_mem, 0)));
>    shadow_mem = adjust_automodify_address (shadow_mem, SImode, addr, 0);
>    end = force_reg (Pmode, plus_constant (Pmode, addr, len));
>    emit_label (top_label);


	Jakub

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

 > Can you please be more verbose

Right, I should have been.

So as you can see from the asm log in the bug description, prologue 
writes shadow bytes corresponding to words at frame_shadow_base + { 0, 
4, 8, 12, 16, 24, 28}. Epilogue should clear those but instead it zeros 
out frame_shadow_base + { 0, 4, 8, 12, 16, 40, 44}, thus causing words 
at frame_shadow_base + {24, 28} to remain poisoned and causing false 
Asan errors later.

The reason as I see it is that we change the address of shadow_mem in 
asan_emit_stack_protection twice: once in asan_clear_shadow
   tmp = expand_simple_binop (Pmode, PLUS, addr, gen_int_mode (4, 
Pmode), addr,
                              true, OPTAB_LIB_WIDEN);
   if (tmp != addr)
     emit_move_insn (addr, tmp);
and asan_emit_stack_protection itself:
   if (last_size)
     {
       shadow_mem = adjust_address (shadow_mem, VOIDmode,
                    (last_offset - prev_offset)
                    >> ASAN_SHADOW_SHIFT);
This would translate into
   add  r4, r4, #4
and
  add  r3, r4, #24
in the asm. The shadow_mem will thus have the next block offset added to 
it _twice_ and will point to invalid position.
My simple fix uses a temp register in asan_clear_shadow to avoid 
spoiling the shadow_mem inside the loop.

I'm not yet a gcc guru so I wanted some experienced person to say 
whether I'm doing something completely wrong here.

BTW I forgot to mention that Asan tests pass both on ARM and on x86_64.

 > Also, you are missing a ChangeLog entry.

Attached.

-Y

[-- Attachment #2: Changelog.add --]
[-- Type: text/plain, Size: 135 bytes --]

2013-09-30  Yury Gribov  <y.gribov@samsung.com>

	PR sanitizer/58543
	* asan.c: Use new temporary for iteration in asan_clear_shadow.

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

Hi Jakub,

 >> I've recently submitted a bug report regarding invalid unpoisoning 
of stack frame redzones 
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone take 
a look at proposed patch (a simple one-liner) and check whether it's ok 
for commit?
 >
 > Can you please be more verbose

Do you think the proposed patch is ok or should I provide some 
additional info?

-Y

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

 >>> I've recently submitted a bug report regarding invalid unpoisoning 
of stack frame redzones 
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone take 
a look at proposed patch (a simple one-liner) and check whether it's ok 
for commit?
 >>
 >> Can you please be more verbose
 >
 > Do you think the proposed patch is ok or should I provide some 
additional info?

Jakub, Dodji,

Any updates on this one? Note that this bug is a huge blocker for using 
AddressSanitizer on ARM platforms.

-Y

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Jakub Jelinek]

On Wed, Oct 16, 2013 at 09:35:21AM +0400, Yury Gribov wrote:
> >>> I've recently submitted a bug report regarding invalid
> unpoisoning of stack frame redzones
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone
> take a look at proposed patch (a simple one-liner) and check whether
> it's ok for commit?
> >>
> >> Can you please be more verbose
> >
> > Do you think the proposed patch is ok or should I provide some
> additional info?
> 
> Jakub, Dodji,
> 
> Any updates on this one? Note that this bug is a huge blocker for
> using AddressSanitizer on ARM platforms.

Sorry for the delay, I finally found time to look at it.
While your patch fixes the issue, I wonder if for the case where
shadow_mem before the asan_clear_shadow call is already of the form
(mem (reg)) it isn't better to just adjust next asan_clear_shadow
base from the end of the cleared region rather than from the start of it,
because the end will be already in the right pseudo, while with your
approach it needs to be done in a different register and potentially
increase register pressure.  So here is (completely untested) alternate fix:

2013-10-29  Jakub Jelinek  <jakub@redhat.com>
	    Yury Gribov  <y.gribov@samsung.com>

	PR sanitizer/58543
	* asan.c (asan_clear_shadow): Return bool whether the emitted loop
	(if any) updated shadow_mem's base.
	(asan_emit_stack_protection): If asan_clear_shadow returned true,
	adjust shadow_mem and prev_offset.

--- gcc/asan.c.jj	2013-10-23 14:43:15.000000000 +0200
+++ gcc/asan.c	2013-10-29 10:26:56.085406934 +0100
@@ -878,10 +878,11 @@ asan_shadow_cst (unsigned char shadow_by
 /* Clear shadow memory at SHADOW_MEM, LEN bytes.  Can't call a library call here
    though.  */
 
-static void
+static bool
 asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
 {
   rtx insn, insns, top_label, end, addr, tmp, jump;
+  bool ret;
 
   start_sequence ();
   clear_storage (shadow_mem, GEN_INT (len), BLOCK_OP_NORMAL);
@@ -893,12 +894,13 @@ asan_clear_shadow (rtx shadow_mem, HOST_
   if (insn == NULL_RTX)
     {
       emit_insn (insns);
-      return;
+      return false;
     }
 
   gcc_assert ((len & 3) == 0);
   top_label = gen_label_rtx ();
   addr = force_reg (Pmode, XEXP (shadow_mem, 0));
+  ret = addr == XEXP (shadow_mem, 0);
   shadow_mem = adjust_automodify_address (shadow_mem, SImode, addr, 0);
   end = force_reg (Pmode, plus_constant (Pmode, addr, len));
   emit_label (top_label);
@@ -912,6 +914,7 @@ asan_clear_shadow (rtx shadow_mem, HOST_
   jump = get_last_insn ();
   gcc_assert (JUMP_P (jump));
   add_int_reg_note (jump, REG_BR_PROB, REG_BR_PROB_BASE * 80 / 100);
+  return ret;
 }
 
 /* Insert code to protect stack vars.  The prologue sequence should be emitted
@@ -1048,7 +1051,14 @@ asan_emit_stack_protection (rtx base, HO
 				       (last_offset - prev_offset)
 				       >> ASAN_SHADOW_SHIFT);
 	  prev_offset = last_offset;
-	  asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
+	  if (asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT))
+	    {
+	      shadow_mem
+		= adjust_automodify_address (shadow_mem, VOIDmode,
+					     XEXP (shadow_mem, 0),
+					     last_size >> ASAN_SHADOW_SHIFT);
+	      prev_offset += last_size;
+	    }
 	  last_offset = offset;
 	  last_size = 0;
 	}


	Jakub

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

 > Sorry for the delay, I finally found time to look at it.

Np, thanks for helping!

 > While your patch fixes the issue, I wonder
 > ...
 > potentially increase register pressure.

Makes sense. I didn't take care of this because I believed that we can 
freely allocate vregs and rely on register allocator do it's work.

 > So here is (completely untested) alternate fix:

Nice, this seems to fix my repro cases.

I'll only be able to test my larger ARM app in 1-2 days. We could wait 
until then or commit now.

-Y

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Oct 16, 2013 at 09:35:21AM +0400, Yury Gribov wrote:
>> >>> I've recently submitted a bug report regarding invalid
>> unpoisoning of stack frame redzones
>> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone
>> take a look at proposed patch (a simple one-liner) and check whether
>> it's ok for commit?
>> >>
>> >> Can you please be more verbose
>> >
>> > Do you think the proposed patch is ok or should I provide some
>> additional info?
>> 
>> Jakub, Dodji,
>> 
>> Any updates on this one? Note that this bug is a huge blocker for
>> using AddressSanitizer on ARM platforms.
>
> Sorry for the delay, I finally found time to look at it.
> While your patch fixes the issue, I wonder if for the case where
> shadow_mem before the asan_clear_shadow call is already of the form
> (mem (reg)) it isn't better to just adjust next asan_clear_shadow
> base from the end of the cleared region rather than from the start of it,
> because the end will be already in the right pseudo, while with your
> approach it needs to be done in a different register and potentially
> increase register pressure.  So here is (completely untested) alternate fix:

Hmm, it just seems wrong to be assigning to registers returned by force_reg
and expand_binop though.  Would this variation be OK?

Thanks,
Richard


Index: gcc/asan.c
===================================================================
--- gcc/asan.c	(revision 204124)
+++ gcc/asan.c	(working copy)
@@ -876,9 +876,10 @@
 }
 
 /* Clear shadow memory at SHADOW_MEM, LEN bytes.  Can't call a library call here
-   though.  */
+   though.  If the address of the next byte is in a known register, return
+   that register, otherwise return null.  */
 
-static void
+static rtx
 asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
 {
   rtx insn, insns, top_label, end, addr, tmp, jump;
@@ -893,12 +894,12 @@
   if (insn == NULL_RTX)
     {
       emit_insn (insns);
-      return;
+      return 0;
     }
 
   gcc_assert ((len & 3) == 0);
   top_label = gen_label_rtx ();
-  addr = force_reg (Pmode, XEXP (shadow_mem, 0));
+  addr = copy_to_mode_reg (Pmode, XEXP (shadow_mem, 0));
   shadow_mem = adjust_automodify_address (shadow_mem, SImode, addr, 0);
   end = force_reg (Pmode, plus_constant (Pmode, addr, len));
   emit_label (top_label);
@@ -912,6 +913,7 @@
   jump = get_last_insn ();
   gcc_assert (JUMP_P (jump));
   add_int_reg_note (jump, REG_BR_PROB, REG_BR_PROB_BASE * 80 / 100);
+  return addr;
 }
 
 /* Insert code to protect stack vars.  The prologue sequence should be emitted
@@ -1048,7 +1050,15 @@
 				       (last_offset - prev_offset)
 				       >> ASAN_SHADOW_SHIFT);
 	  prev_offset = last_offset;
-	  asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
+	  rtx end_addr = asan_clear_shadow (shadow_mem,
+					    last_size >> ASAN_SHADOW_SHIFT);
+	  if (end_addr)
+	    {
+	      shadow_mem
+		= adjust_automodify_address (shadow_mem, VOIDmode, end_addr,
+					     last_size >> ASAN_SHADOW_SHIFT);
+	      prev_offset += last_size;
+	    }
 	  last_offset = offset;
 	  last_size = 0;
 	}

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Jakub Jelinek]

On Tue, Oct 29, 2013 at 12:25:33PM +0000, Richard Sandiford wrote:
> >> Any updates on this one? Note that this bug is a huge blocker for
> >> using AddressSanitizer on ARM platforms.
> >
> > Sorry for the delay, I finally found time to look at it.
> > While your patch fixes the issue, I wonder if for the case where
> > shadow_mem before the asan_clear_shadow call is already of the form
> > (mem (reg)) it isn't better to just adjust next asan_clear_shadow
> > base from the end of the cleared region rather than from the start of it,
> > because the end will be already in the right pseudo, while with your
> > approach it needs to be done in a different register and potentially
> > increase register pressure.  So here is (completely untested) alternate fix:
> 
> Hmm, it just seems wrong to be assigning to registers returned by force_reg
> and expand_binop though.  Would this variation be OK?

Why?  If it is a pseudo, it is certainly a pseudo that isn't used for
anything else, as it is the result of (base >> 3) + constant, if it isn't a
pseudo, then supposedly it is better not to just keep adding the offsets to
a base and thus not to use the end address from asan_clear_shadow.
Your patch would force using the end address even if shadow_base
is initially say some_reg + 32, I think it is better to use shadow_mem
some_reg + 72 next time instead of some_other_reg + 40.

	Jakub

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Oct 29, 2013 at 12:25:33PM +0000, Richard Sandiford wrote:
>> >> Any updates on this one? Note that this bug is a huge blocker for
>> >> using AddressSanitizer on ARM platforms.
>> >
>> > Sorry for the delay, I finally found time to look at it.
>> > While your patch fixes the issue, I wonder if for the case where
>> > shadow_mem before the asan_clear_shadow call is already of the form
>> > (mem (reg)) it isn't better to just adjust next asan_clear_shadow
>> > base from the end of the cleared region rather than from the start of it,
>> > because the end will be already in the right pseudo, while with your
>> > approach it needs to be done in a different register and potentially
>> > increase register pressure.  So here is (completely untested) alternate fix:
>> 
>> Hmm, it just seems wrong to be assigning to registers returned by force_reg
>> and expand_binop though.  Would this variation be OK?
>
> Why?

Well, that was the traditional rule:

   The caller must not alter the value in the register we return,
   since we mark it as a "constant" register.  */

rtx
force_reg (enum machine_mode mode, rtx x)
{

but maybe it doesn't matter now, since the constant register flag has
gone and since we seem to use REG_EQUAL rather than REG_EQUIV...

> If it is a pseudo, it is certainly a pseudo that isn't used for
> anything else, as it is the result of (base >> 3) + constant, if it isn't a
> pseudo, then supposedly it is better not to just keep adding the offsets to
> a base and thus not to use the end address from asan_clear_shadow.
> Your patch would force using the end address even if shadow_base
> is initially say some_reg + 32, I think it is better to use shadow_mem
> some_reg + 72 next time instead of some_other_reg + 40.

But I thought the register pressure thing was to avoid having the
original base live across the loop.  Doesn't that apply for reg + offset
as well as plain reg?  If we have to force some_reg + 32 into a
temporary and then loop on it, using the new base should avoid keeping
both it and some_reg live at the same time.  Plus smaller offsets are
often better on some targets.

Thanks,
Richard

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Jakub Jelinek]

On Tue, Oct 29, 2013 at 01:06:21PM +0000, Richard Sandiford wrote:
> > If it is a pseudo, it is certainly a pseudo that isn't used for
> > anything else, as it is the result of (base >> 3) + constant, if it isn't a
> > pseudo, then supposedly it is better not to just keep adding the offsets to
> > a base and thus not to use the end address from asan_clear_shadow.
> > Your patch would force using the end address even if shadow_base
> > is initially say some_reg + 32, I think it is better to use shadow_mem
> > some_reg + 72 next time instead of some_other_reg + 40.
> 
> But I thought the register pressure thing was to avoid having the
> original base live across the loop.  Doesn't that apply for reg + offset
> as well as plain reg?  If we have to force some_reg + 32 into a
> temporary and then loop on it, using the new base should avoid keeping
> both it and some_reg live at the same time.  Plus smaller offsets are
> often better on some targets.

On the other side, if we force the address to be just register based when it
previously wasn't, it makes life harder for alias analysis etc., at least in
the past say on ia64 I remember the fact that it didn't allow anything but
registers as valid addresses often resulted in tons of missed
optimizations), so in the end I guess I have nothing against the original
patch (with whatever form of the copy to reg call is desirable).

	Jakub

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

 > so in the end I guess I have nothing against the original patch
 > (with whatever form of the copy to reg call is desirable).

So ok to commit?

-Y

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Jakub Jelinek]

On Wed, Oct 30, 2013 at 11:32:14AM +0400, Yury Gribov wrote:
> > so in the end I guess I have nothing against the original patch
> > (with whatever form of the copy to reg call is desirable).
> 
> So ok to commit?

Ok with the change suggested by Richard, I think it was:
  addr = copy_to_mode_reg (Pmode, XEXP (shadow_mem, 0));

	Jakub

Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Yury Gribov]

 >> So ok to commit?
 > Ok with the change suggested by Richard, I think it was:
   addr = copy_to_mode_reg (Pmode, XEXP (shadow_mem, 0));

Done, r204251. Tested against x64 and ARM.

-Y