Invalid unpoisoning of stack redzones on ARM

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

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

> 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
2013-09-30  Yury Gribov  <y.gribov@samsung.com>

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

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

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