This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM")
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 23 Oct 2014 08:39:47 -0700
- Subject: [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM")
- Authentication-results: sourceware.org; auth=none
- References: <20141022142231 dot GF4786 at adacore dot com> <87y4s7h553 dot fsf at codesourcery dot com>
> > /* 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