* over-permissive stack_chk_guard on ARM @ 2014-10-22 14:22 Joel Brobecker 2014-10-23 2:53 ` Yao Qi 0 siblings, 1 reply; 9+ messages in thread From: Joel Brobecker @ 2014-10-22 14:22 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 3183 bytes --] Hi Yao, I don't know if you remember, but we discussed an ARM patch back in Dec 2010, which was adding support for skipping stack-check-guard code as part of the prologue: https://www.sourceware.org/ml/gdb-patches/2010-12/msg00110.html I discovered that the heuristic used is mistakenly thinking that some code that fetches a global is some stack_chk_guard code, which, in turn, causes the debugger to skip it when inserting breakpoints. The full code is attached, but I suspect you will not have the Ada compiler to build it. But I can send you the binary if you need it. At this point, I am just trying to collect for more info. In our case, we're trying to insert a breakpoint on str.adb:4, where the code looks like this: 3 procedure STR is 4 XX : String (1 .. Blocks.Sz) := (others => 'X'); -- STOP 5 K : Integer; 6 begin 7 K := 13; Line 4 declares a new variable "XX" whihch is an array whose size is determined by the value of a global variable "Sz" from package "Blocks", and then assigns it an initial value (all 'X'-s). The generated code 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. Looking at the implementation of the prologue analyzing, it seems that a normal sequence would be what you put in the comments: ldr Rn, .Label .... .Lable: .word __stack_chk_guard But the implementation seems to be going further than that. If the location of the first ldr points to data that's not the address of __stack_chk_guard, then it looks at the next two instructions, to see if they might following another pattern: /* 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 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. Do you remember, by any chance? -- Joel [-- Attachment #2: blocks.ads --] [-- Type: text/plain, Size: 109 bytes --] with System; package Blocks is SZ : Integer := 15; procedure Do_Nothing (A : System.Address); end; [-- Attachment #3: blocks.adb --] [-- Type: text/plain, Size: 118 bytes --] package body Blocks is procedure Do_Nothing (A : System.Address) is begin null; end Do_Nothing; end; [-- Attachment #4: str.adb --] [-- Type: text/plain, Size: 200 bytes --] with Blocks; procedure STR is XX : String (1 .. Blocks.Sz) := (others => 'X'); -- STOP K : Integer; begin K := 13; Blocks.Do_Nothing (XX'Address); Blocks.Do_Nothing (K'Address); end; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: over-permissive stack_chk_guard on ARM 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 ` [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM") Joel Brobecker 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2014-10-23 2:53 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches Joel Brobecker <brobecker@adacore.com> writes: > But the implementation seems to be going further than that. > If the location of the first ldr points to data that's not > the address of __stack_chk_guard, then it looks at the next > two instructions, to see if they might following another > pattern: > > /* 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? > 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? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM") 2014-10-23 2:53 ` Yao Qi @ 2014-10-23 15:39 ` Joel Brobecker 2014-10-24 8:29 ` [RFA] ARM: stricter __stack_chk_guard check during prologue Yao Qi 0 siblings, 1 reply; 9+ messages in thread From: Joel Brobecker @ 2014-10-23 15:39 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 2014-10-23 15:39 ` [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM") Joel Brobecker @ 2014-10-24 8:29 ` Yao Qi 2014-10-24 12:23 ` Joel Brobecker 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2014-10-24 8:29 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches Joel Brobecker <brobecker@adacore.com> writes: Joel, 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. > /* 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. > - 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. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 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 0 siblings, 1 reply; 9+ messages in thread From: Joel Brobecker @ 2014-10-24 12:23 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1005 bytes --] > 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 [-- Attachment #2: 0001-ARM-stricter-__stack_chk_guard-check-during-prologue.patch --] [-- Type: text/x-diff, Size: 3432 bytes --] 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 2014-10-24 12:23 ` Joel Brobecker @ 2014-10-24 12:48 ` Yao Qi 2014-10-27 6:26 ` Yao Qi 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2014-10-24 12:48 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 2014-10-24 12:48 ` Yao Qi @ 2014-10-27 6:26 ` Yao Qi 2014-10-29 5:48 ` Yao Qi 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2014-10-27 6:26 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 2014-10-27 6:26 ` Yao Qi @ 2014-10-29 5:48 ` Yao Qi 2014-10-29 13:12 ` Joel Brobecker 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2014-10-29 5:48 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches 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. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] ARM: stricter __stack_chk_guard check during prologue 2014-10-29 5:48 ` Yao Qi @ 2014-10-29 13:12 ` Joel Brobecker 0 siblings, 0 replies; 9+ messages in thread From: Joel Brobecker @ 2014-10-29 13:12 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > >>> 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. -- Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-10-29 13:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM") Joel Brobecker 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
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).