[RFA] ARM: stricter __stack_chk_guard check during prologue
Commit Message
> 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?
Comments
Joel Brobecker <brobecker@adacore.com> writes:
> Thanks! Attached is a revised patch. I will push it after you
> fix the latent bug you noticed - just let me know when?
I'll post the patch next Monday.
Yao Qi <yao@codesourcery.com> writes:
>> Thanks! Attached is a revised patch. I will push it after you
>> fix the latent bug you noticed - just let me know when?
>
> I'll post the patch next Monday.
For the record, I posted the patch here
https://sourceware.org/ml/gdb-patches/2014-10/msg00690.html
Yao Qi <yao@codesourcery.com> writes:
>>> Thanks! Attached is a revised patch. I will push it after you
>>> fix the latent bug you noticed - just let me know when?
>>
>> I'll post the patch next Monday.
>
> For the record, I posted the patch here
> https://sourceware.org/ml/gdb-patches/2014-10/msg00690.html
Joel,
I've pushed the patch in. You can push your patch in now.
> >>> Thanks! Attached is a revised patch. I will push it after you
> >>> fix the latent bug you noticed - just let me know when?
> >>
> >> I'll post the patch next Monday.
> >
> > For the record, I posted the patch here
> > https://sourceware.org/ml/gdb-patches/2014-10/msg00690.html
>
> Joel,
> I've pushed the patch in. You can push your patch in now.
Thank you, Yao. My patch is now in as well.
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(-)
@@ -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