This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM")


> >       /* Step 2: ldr Rd, [Rn, #immed], encoding T1.  */
> >       /* Step 3: str Rd, [Rn, #immed], encoding T1.  */
> >
> > Looking at the code and the function description, it seems to me
> > that the normal situation would be what the comment alluded to,
> > and that if it was the entire story, we wouldn't have needed
> > the code doing steps 2 & 3. But, looking at the email archives
> 
> Sorry, I don't understand why do you think steps 2 & 3 are not needed?
> Do you mean we don't have to go to step 2 & 3 if we can't find symbol
> __stack_chk_guard in step 1?

I see what you mean. I misread the code that it would stop if
the address was pointing to the __stack_chk_guard symbol. But
in reality, it stops if it is pointing to a symbol that is NOT
__stack_chk_guard.

> > as well as the bug report initially referenced, I can't find
> > really any explanation for what prompted you to add that code.
> > I would need that in order to adjust the heuristics without
> > breaking your situation.
> 
> Currently, we do so in order to handle the case symbol __stack_chk_guard
> is removed, as the comments said:
> 
>   /* 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.  */
> 
> However, I don't recall under what circumstance symbol
> '__stack_chk_guard' can be removed.  __stack_chk_guard is in .dynsym
> section, so it can't be removed.  (I presume symbols in .dynsym can't be
> removed, correct me if I am wrong).  If I am correct, we can restrict
> the condition in step 1 that return early if the symbol name doesn't
> start with '__stack_chk_guard'.  Then, step 2 & 3 is not needed, or we
> can keep them as a sanity check?

For heuristics, I would keep the heuristics as strict as possible
to avoid possible incorrect matches, so I would keep it.

What do you think of the attached patch?

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.

Tested on arm-eabi using AdaCore's testsuite.

Thanks,
-- 
Joel
>From 6dbcc17c72575438c4c6a41c1d10938ad1a08090 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.

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

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e2559ec..c7f7d97 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1309,8 +1309,8 @@ arm_skip_stack_protector(CORE_ADDR pc, struct gdbarch *gdbarch)
   /* 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),
+  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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]