[PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

Srinath Parvathaneni
Hi All,

GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
it creates two new instructions secure gateway (sg) and original branch destination (b.w),
place those two instructions in ".gnu.sgstubs" section of executable.
Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
present in ".gnu.sgstubs" section.

Example:
Following is a function call to cmse secure entry function "foo":
        ...
        bl xxxx <foo>   --->(a)
        ...
        <foo>
        xxxx: push    {r7, lr}

GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
place them in ".gnu.sgstubs" section (marked by c).

The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
(as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
       ...
       bl yyyy <foo>  ---> (b)
       ...
       section .gnu.sgstubs: ---> (c)
       yyyy <foo>
       yyyy: sg   // secure gateway
             b.w xxxx <__acle_se_foo>  // original_branch_dest
       ...
       0000xxxx <__acle_se_foo>
       xxxx: push    {r7, lr} ---> (d)

On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
(sg address) which is a trampoline and which does not exist in source code. So GDB jumps
to next line without jumping to "__acle_se_foo" (marked by d).

This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
command at "b", so that the control jumps to "__acle_se_foo" (marked by d).

This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.

Ok for master?

Hi All,

GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
it creates two new instructions secure gateway (sg) and original branch destination (b.w),
place those two instructions in ".gnu.sgstubs" section of executable.
Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
present in ".gnu.sgstubs" section.

Example:
Following is a function call to cmse secure entry function "foo":
        ...
        bl xxxx <foo>   --->(a)
        ...
        <foo>
        xxxx: push    {r7, lr}

GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
place them in ".gnu.sgstubs" section (marked by c).

The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
(as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
       ...
       bl yyyy <foo>  ---> (b)
       ...
       section .gnu.sgstubs: ---> (c)
       yyyy <foo>
       yyyy: sg   // secure gateway
             b.w xxxx <__acle_se_foo>  // original_branch_dest
       ...
       0000xxxx <__acle_se_foo>
       xxxx: push    {r7, lr} ---> (d)

On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
(sg address) which is a trampoline and which does not exist in source code. So GDB jumps
to next line without jumping to "__acle_se_foo" (marked by d).

This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
command at "b", so that the control jumps to "__acle_se_foo" (marked by d).

This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.

Ok for master?

gdb/ChangeLog:

2019-07-17  Srinath Parvathaneni  <[hidden email]>

        * arm-tdep.h (check_section_name): New function declaration.
        * arm-tdep.c (arm_skip_sg_jump_to_bw): New function. When gdb
        encounters a "step" command on cmse secure entry function (eg:func),
        this function return an address of "__acle_se_<func>" to PC instead
        of secure gateaway (sg) address which is present in ".gnu.sgstubs"
        section.
        (check_section_name): New function. To check the current section is
        ".gnu.sgstubs".
        (arm_skip_stub): Modify to call arm_skip_sg_jump_to_bw function.

gdb/testsuite/ChangeLog:

2019-07-17  Srinath Parvathaneni  <[hidden email]>

        * gdb.base/arm-main-cmse.c: New test.
        * gdb.base/arm-main-cmse.exp: New file.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

Alan Hayward


> On 17 Jul 2019, at 16:20, Srinath Parvathaneni <[hidden email]> wrote:
>
> Hi All,

Hi, thanks for the patch.
There are a few nits to clean up (see comments below).
The only big issue is that when I ran your test without your fixes it passed for me (see
final comment).

>
> GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
> Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
> it creates two new instructions secure gateway (sg) and original branch destination (b.w),
> place those two instructions in ".gnu.sgstubs" section of executable.
> Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
> present in ".gnu.sgstubs" section.
>
> Example:
> Following is a function call to cmse secure entry function "foo":
>        ...
>        bl xxxx <foo>   --->(a)
>        ...
>        <foo>
>        xxxx: push    {r7, lr}
>
> GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
> place them in ".gnu.sgstubs" section (marked by c).
>
> The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
> (as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
>       ...
>       bl yyyy <foo>  ---> (b)
>       ...
>       section .gnu.sgstubs: ---> (c)
>       yyyy <foo>
>       yyyy: sg   // secure gateway
>     b.w xxxx <__acle_se_foo>  // original_branch_dest
>       ...
>       0000xxxx <__acle_se_foo>
>       xxxx: push    {r7, lr} ---> (d)
>
> On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
> (sg address) which is a trampoline and which does not exist in source code. So GDB jumps
> to next line without jumping to "__acle_se_foo" (marked by d).
>

Could you please include a reference to the document on developer.arm.com where this is
specified, plus the relevant section number in the document.


> This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
> command at "b", so that the control jumps to "__acle_se_foo" (marked by d).
>
> This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
> Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.
>
> Ok for master?
>
> Hi All,
>
> GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
> Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
> it creates two new instructions secure gateway (sg) and original branch destination (b.w),
> place those two instructions in ".gnu.sgstubs" section of executable.
> Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
> present in ".gnu.sgstubs" section.
>
> Example:
> Following is a function call to cmse secure entry function "foo":
>        ...
>        bl xxxx <foo>   --->(a)
>        ...
>        <foo>
>        xxxx: push    {r7, lr}
>
> GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
> place them in ".gnu.sgstubs" section (marked by c).
>
> The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
> (as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
>       ...
>       bl yyyy <foo>  ---> (b)
>       ...
>       section .gnu.sgstubs: ---> (c)
>       yyyy <foo>
>       yyyy: sg   // secure gateway
>     b.w xxxx <__acle_se_foo>  // original_branch_dest
>       ...
>       0000xxxx <__acle_se_foo>
>       xxxx: push    {r7, lr} ---> (d)
>
> On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
> (sg address) which is a trampoline and which does not exist in source code. So GDB jumps
> to next line without jumping to "__acle_se_foo" (marked by d).
>
> This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
> command at "b", so that the control jumps to "__acle_se_foo" (marked by d).
>
> This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
> Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.
>
> Ok for master?
>
> gdb/ChangeLog:
>
> 2019-07-17  Srinath Parvathaneni  <[hidden email]>
>
> * arm-tdep.h (check_section_name): New function declaration.
> * arm-tdep.c (arm_skip_sg_jump_to_bw): New function. When gdb
> encounters a "step" command on cmse secure entry function (eg:func),
> this function return an address of "__acle_se_<func>" to PC instead
> of secure gateaway (sg) address which is present in ".gnu.sgstubs"
> section.
> (check_section_name): New function. To check the current section is
> ".gnu.sgstubs".
> (arm_skip_stub): Modify to call arm_skip_sg_jump_to_bw function.
>
> gdb/testsuite/ChangeLog:
>
> 2019-07-17  Srinath Parvathaneni  <[hidden email]>
>
> * gdb.base/arm-main-cmse.c: New test.
> * gdb.base/arm-main-cmse.exp: New file.
> <diff>

If possible, it’s better to send the patch inline rather than as an attachment.
However, email clients have a habit of editing the format of patches when you
put them inline. Therefore I’d highly recommend using git send-email.

> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index 23dd40ea8beb1b00289a4cd4e65647399d351580..c56ed5dc6508a9c0994eebe1c90e60ede2df892f 100644
> --- a/gdb/arm-tdep.h
> +++ b/gdb/arm-tdep.h
> @@ -259,6 +259,7 @@ ULONGEST arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
>  
>  CORE_ADDR arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
>       CORE_ADDR val);
> +int check_section_name (struct obj_section *);
>  
>  int arm_get_next_pcs_is_thumb (struct arm_get_next_pcs *self);
>  
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index d244707210628ab045f677c0cbad3d8b0c6d6269..b51663031b45bda1df61ed6caca37deb531a5cd7 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8212,6 +8212,53 @@ arm_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>    return 1;
>  }
>  
> +/* A call to cmse secure entry function "foo" at "a" is modified by
> +     GNU ld as "b".
> +     a) bl xxxx <foo>
> +
> +     <foo>
> +     xxxx:
> +
> +     b) bl yyyy <__acle_se_foo>
> +
> +     section .gnu.sgstubs:
> +     <foo>
> +     yyyy: sg   // secure gateway
> +   b.w xxxx <__acle_se_foo>  // original_branch_dest
> +
> +     <__acle_se_foo>
> +     xxxx:
> +
> +  When the control at "b", the pc contains "yyyy" (sg address) which is a
> +  trampoline and does not exist in source code.  This function returns
> +  the target pc "xxxx".  */

Add the name of the reference document, plus section.
(don’t need a full url link here).


> +
> +static CORE_ADDR
> +arm_skip_sg_jump_to_bw (CORE_ADDR pc, const char *name, struct objfile *objfile)

I think this is better named as arm_skip_cmse_entry or something similar.
It keeps the function name at a “higher” level.


> +{
> +  struct bound_minimal_symbol minsym;
> +  char *target_name;
> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
> +  target_name = (char *) alloca (target_len);

You might as well put “char *” on the start of the line, and remove the
declaration of target_name two lines ago.

> +  snprintf (target_name, target_len, "%s%s", "__acle_se_",name);
> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
> +  if (minsym.minsym != NULL)
> +    return BMSYMBOL_VALUE_ADDRESS (minsym);

Could you use nullptr instead of NULL. nullptr is for where a pointer is
expected.

(You’ll find lots of places where NULL is used, but that’s because GDB has
 moved from being C to C++ and has lots of old code).


> +  return 0;
> +}
> +
> +/* Return 1 when pc holds an address that belongs to ".gnu.sgstubs"
> +   section.  */
> +int

Can you use bool instead of int. (again, you’ll see int elsewhere in the code
due to old C code).

> +check_section_name (struct obj_section *sec)

Needs a better name. Maybe arm_is_cmse_section.

> +{
> + if (sec != NULL && sec->the_bfd_section != NULL
> +     && sec->the_bfd_section->name != NULL
> +     && !strcmp (sec->the_bfd_section->name,".gnu.sgstubs”))

"0 == strcmp” is easier to parse than !strcmp.

Same NULL issues.


> +   return 1;
> + return 0;
> +}
> +
>  /* Recognize GCC and GNU ld's trampolines.  If we are in a trampoline,
>     return the target PC.  Otherwise return 0.  */
>  
> @@ -8221,6 +8268,7 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
>    const char *name;
>    int namelen;
>    CORE_ADDR start_addr;
> +  struct obj_section *section;
>  
>    /* Find the starting address and name of the function containing the PC.  */
>    if (find_pc_partial_function (pc, &name, &start_addr, NULL) == 0)
> @@ -8290,6 +8338,10 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
>   return 0;
>      }
>  
> +  section = find_pc_section (pc);
> +  /* checks whether target pc belows to ".gnu.sgstubs" section.  */
> +  if (check_section_name (section))
> +    return arm_skip_sg_jump_to_bw (pc, name, section->objfile);
>    return 0; /* not a stub */
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/arm-main-cmse.c b/gdb/testsuite/gdb.base/arm-main-cmse.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bde55075fdc21bc781a5dd6ac8221a550faca4c5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/arm-main-cmse.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +extern void func();
> +void __acle_se_func ()
> +{
> +  printf("__acle_se_func\n");
> +}
> +
> +asm ("\t.section .gnu.sgstubs,\"ax\",%progbits\n"
> +     "\t.global func\n"
> +     "\t.type func, %function\n"
> +     "func:\n"
> +     "\tnop @sg\n"
> +     "\tb __acle_se_func @b.w”);

Add a comment to state why you are writing this in asm instead of C.

> +
> +void fun1 ()
> +{
> +  printf("In fun1\n");
> +}
> +
> +int main (void)
> +{
> +  while(1)
> +   {

Remove the while. Test programs should exit by themselves when run.

If GDB dies/exits in a specific way when the test is running, there is a chance the
test application may continue to run by itself An infinite loop would mean it runs
forever stealing cpu.


> +     func();
> +     fun1();
> +     __acle_se_func();
> +   }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/arm-main-cmse.exp b/gdb/testsuite/gdb.base/arm-main-cmse.exp


arm-main-cmse should either be arm-cmse or even better add something to specify
the cmse functionality being tested, for example arm-cmse-sgstubs.

Also, it should be in gdb.arch, not gdb.base.


> new file mode 100644
> index 0000000000000000000000000000000000000000..5c9abf43908f505f13151f3753f81db44b9eb6b3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/arm-main-cmse.exp
> @@ -0,0 +1,56 @@
> +# Copyright 2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +if { ![istarget "arm*-*-*"]} {
> +     return 1
> +}
> +


> +standard_testfile arm-main-cmse.c
> +set exefile [standard_output_file ${testfile}.out]
> +set opts [list debug "additional_flags=-g -dA"]
> +
> +if { [gdb_compile ${srcdir}/${subdir}/${testfile}.c ${exefile} executable $opts] != ""} {
> +    return -1
> +}
> +
> +clean_restart ${exefile}

You can reduce the above section of code to:

standard_testfile
if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
    return -1
}


> +if ![runto_main] {

Add an untested line here if the runto fails.

> +   return -1
> +}
> +
> +set test "branch to func from main"
> +gdb_test "si" "0x.*" "$test"
> +
> +set test "next instruction in func"
> +gdb_test "ni" "0x.*" "$test"
> +
> +set test "branch to __acle_se_func from func"
> +gdb_test "ni" "__acle_se_func .*" "$test"
> +
> +set test "next in __acle_se_func function"
> +gdb_test "next" "5  .*" "$test"
> +
> +set test "next in __acle_se_func function outputs __acle_se_func"
> +gdb_test "next" "__acle_se_func.*" "$test"
> +
> +gdb_test "continue" "Continuing.*" "continue to func() in main"
> +
> +set test "control branches to __acle_se_func from main via func"
> +gdb_test "step" "__acle_se_fun.*" "$test"
> +
> +set test "next in __acle_se_func function via func"
> +gdb_test "next" "__acle_se_func.*" "$test”
>

When I run this test on HEAD without your patch it passes for me. I think there
might be something missing from the test?



Alan.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

Simon Marchi-4
On 2019-07-17 2:35 p.m., Alan Hayward wrote:
>> +{
>> + if (sec != NULL && sec->the_bfd_section != NULL
>> +     && sec->the_bfd_section->name != NULL
>> +     && !strcmp (sec->the_bfd_section->name,".gnu.sgstubs”))
>
> "0 == strcmp” is easier to parse than !strcmp.

Nothing much to add after Alan's review, but I just wanted to note that we have streq for this, e.g.:

  if (streq (sec->the_bfd_section->name, ".gnu.sgstubs”))

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

Tom Tromey-2
In reply to this post by Alan Hayward
>>>>> "Alan" == Alan Hayward <[hidden email]> writes:

>> +  struct bound_minimal_symbol minsym;
>> +  char *target_name;
>> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
>> +  target_name = (char *) alloca (target_len);

Alan> You might as well put “char *” on the start of the line, and remove the
Alan> declaration of target_name two lines ago.

>> +  snprintf (target_name, target_len, "%s%s", "__acle_se_",name);

Occasionally I think we should remove all uses of alloca from gdb, since
alloca is often just a bug waiting to happen.  However, since gdb still
uses it, the above could either be written using the libiberty ACONCAT
macro; or if not that, then xsnprintf would be preferable.

>> +set opts [list debug "additional_flags=-g -dA"]

I am curious whether -dA is needed here, and if so why.
If it is necessary then a comment would be nice.

thanks,
Tom