over-permissive stack_chk_guard on ARM

over-permissive stack_chk_guard on ARM

[Joel Brobecker]

[-- 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;

Re: over-permissive stack_chk_guard on ARM

[Yao Qi]

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 (齐尧)

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

[Joel Brobecker]

[-- 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

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Yao Qi]

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 (齐尧)

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Joel Brobecker]

[-- 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

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Yao Qi]

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 (齐尧)

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Yao Qi]

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 (齐尧)

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Yao Qi]

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 (齐尧)

Re: [RFA] ARM: stricter __stack_chk_guard check during prologue

[Joel Brobecker]

> >>> 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