Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Joel Brobecker <brobecker at adacore dot com>]

> I run regression tests on arm-linux-gnueabi with your patch.  There
> are some fails on armv4t arm and thumb mode.  It is an existing bug
> introduced by my patch :( and your patch just exposed it.  I'll fix it
> separately.

Thanks, Yao.

> >    /* If name of symbol doesn't start with '__stack_chk_guard', this
> >       instruction sequence is not for stack protector.  If symbol is
> >       removed, we conservatively think this sequence is for stack protector.  */
> 
> We need to update the comment to sync with the code below.

Indeed!

> > -  if (stack_chk_guard.minsym
> > -      && strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym),
> > +  if (stack_chk_guard.minsym == NULL
> > +      || strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym),
> >  		  "__stack_chk_guard",
> >  		  strlen ("__stack_chk_guard")) != 0)
> 
> Otherwise, it looks good to me.

Thanks! Attached is a revised patch.  I will push it after you
fix the latent bug you noticed - just let me know when?

-- 
Joel

>From fae3d8ff9fc547d879aa64b7d3f89ed7323a7e1d Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 23 Oct 2014 08:25:20 -0700
Subject: [PATCH] ARM: stricter __stack_chk_guard check during prologue
 analysis

We are trying to insert a breakpoint on line 4 for the following
Ada code.

  3 procedure STR is
  4    XX : String (1 .. Blocks.Sz) := (others => 'X'); -- STOP
  5    K : Integer;
  6 begin
  7    K := 13;

The code generated on ARM (-march=armv7-m) starts like this:

    (gdb) disass str'address
    Dump of assembler code for function _ada_str:
       --# Line str.adb:3
       0x08000014 <+0>:     push    {r4, r7, lr}
       0x08000016 <+2>:     sub     sp, #28
       0x08000018 <+4>:     add     r7, sp, #0
       0x0800001a <+6>:     mov     r3, sp
       0x0800001c <+8>:     mov     r4, r3
       --# Line str.adb:4
       0x0800001e <+10>:    ldr     r3, [pc, #84]   ; (0x8000074 <_ada_str+96>)
       0x08000020 <+12>:    ldr     r3, [r3, #0]
       0x08000022 <+14>:    str     r3, [r7, #20]
       0x08000024 <+16>:    ldr     r3, [r7, #20]
       [...]

When computing the address related to str.adb:4, GDB correctly
resolves it to 0x0800001e first, but then considers the next
3 instructions as being part of the prologue because it thinks
they are part of stack-protector code. As a result, instead
of inserting the breakpoint at line 4, it skips those instruction
and consequently the rest of the instructions until the next
line start, which his line 7.

The stack-protector code is expected to start like this...

        ldr     Rn, .Label
        ....
        .Lable:
        .word   __stack_chk_guard

... but the implementation actually accepts a sequence where
the ldr location points to an address for which there is no symbol.
It only aborts if the address points to a symbol which is not
__stack_chk_guard.

Since the __stack_chk_guard symbol is always expected to exist
when used (it lives in .dynsym), this patch fixes the issue by
requiring that the ldr gets the address of the __stack_chk_guard
symbol. If the address could not be resolved, then it rejects
the sequence as being stack-protector code.

gdb/ChangeLog:

        arm-tdep.c (arm_skip_stack_protector): Return early if
        address loaded by first "ldr" instruction does not have
        a corresponding minimal symbol.  Update comment.

Tested on arm-eabi using AdaCore's testsuite.
---
 gdb/arm-tdep.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e2559ec..172e54f 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1306,11 +1306,10 @@ arm_skip_stack_protector(CORE_ADDR pc, struct gdbarch *gdbarch)
     return pc;
 
   stack_chk_guard = lookup_minimal_symbol_by_pc (addr);
-  /* If name of symbol doesn't start with '__stack_chk_guard', this
-     instruction sequence is not for stack protector.  If symbol is
-     removed, we conservatively think this sequence is for stack protector.  */
-  if (stack_chk_guard.minsym
-      && strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym),
+  /* ADDR must correspond to a symbol whose name is __stack_chk_guard.
+     Otherwise, this sequence cannot be for stack protector.  */
+  if (stack_chk_guard.minsym == NULL
+      || strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym),
 		  "__stack_chk_guard",
 		  strlen ("__stack_chk_guard")) != 0)
    return pc;
-- 
1.9.1