From: Joel Brobecker <brobecker@adacore.com>
To: Yao Qi <yao@codesourcery.com>
Cc: gdb-patches@sourceware.org
Subject: [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM")
Date: Thu, 23 Oct 2014 15:39:00 -0000 [thread overview]
Message-ID: <20141023153947.GA11707@adacore.com> (raw)
In-Reply-To: <87y4s7h553.fsf@codesourcery.com>
[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]
> > /* 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
[-- Attachment #2: 0001-ARM-stricter-__stack_chk_guard-check-during-prologue.patch --]
[-- Type: text/x-diff, Size: 3195 bytes --]
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
next prev parent reply other threads:[~2014-10-23 15:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-22 14:22 over-permissive stack_chk_guard on ARM Joel Brobecker
2014-10-23 2:53 ` Yao Qi
2014-10-23 15:39 ` Joel Brobecker [this message]
2014-10-24 8:29 ` [RFA] ARM: stricter __stack_chk_guard check during prologue Yao Qi
2014-10-24 12:23 ` Joel Brobecker
2014-10-24 12:48 ` Yao Qi
2014-10-27 6:26 ` Yao Qi
2014-10-29 5:48 ` Yao Qi
2014-10-29 13:12 ` Joel Brobecker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141023153947.GA11707@adacore.com \
--to=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
--cc=yao@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).