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

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

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

Srinath Parvathaneni
Hello Alan, Simon and Tom.

Thank you very much for reviewing my first patch to GDB and providing me your feedback.
https://sourceware.org/ml/gdb-patches/2019-07/msg00392.html

I have incorporating all your review comments and sending a new patch here.

Hello,

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

The above details are published on the Arm website [1], please refer to section 5.4 (Entry functions)
and section 3.4.4 (C level development flow of secure code).

[1] https://developer.arm.com/architectures/cpu-architecture/m-profile/docs/ecm0359818/latest/armv8-m-security-extensions-requirements-on-development-tools-engineering-specification

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? If ok, could someone commit this on my behalf,
I don't have the commit rights.

gdb/ChangeLog:

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

        * gdb/arm-tdep.c (arm_skip_cmse_entry):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.
        (arm_is_sgstubs_section):New function. To check the current section is
        ".gnu.sgstubs".
        (arm_skip_stub):Add call to arm_skip_cmse_entry function.
        * gdb/arm-tdep.h (arm_is_sgstubs_section):New function declaration.

gdb/testsuite/ChangeLog:

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

        * gdb.arch/arm-cmse-sgstubs.c: New test.
        * gdb.arch/arm-cmse-sgstubs.exp: New file.



###############     Attachment also inlined for ease of reply    ###############


diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 23dd40ea8beb1b00289a4cd4e65647399d351580..9482e765a7fc5bb58676096f6b879eae2a7c858e 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);
+bool arm_is_sgstubs_section (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..e3b7ab6f096eb91da067d772b8798ffd0737e3d6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8211,6 +8211,53 @@ arm_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
   *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
   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".  For more details please refer to section 5.4
+  (Entry functions) and section 3.4.4 (C level development flow of secure code)
+  of "armv8-m-security-extensions-requirements-on-development-tools-engineering-specification"
+  document on www.developer.arm.com.  */
+
+static CORE_ADDR
+arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
+{
+  struct bound_minimal_symbol minsym;
+  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
+  char *target_name  = (char *) alloca (target_len);
+  xsnprintf (target_name, target_len, "%s%s", "__acle_se_",name);
+  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
+  if (minsym.minsym != nullptr)
+    return BMSYMBOL_VALUE_ADDRESS (minsym);
+  return 0;
+}
+
+/* Return true when sec points to ".gnu.sgstubs" section.  */
+bool
+arm_is_sgstubs_section (struct obj_section *sec)
+{
+ if (sec != nullptr && sec->the_bfd_section != nullptr
+     && sec->the_bfd_section->name != nullptr
+     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))
+   return true;
+ return false;
+}
 
 /* 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 address pc holds belows to ".gnu.sgstubs" section.  */
+  if (arm_is_sgstubs_section (section))
+    return arm_skip_cmse_entry (pc, name, section->objfile);
   return 0; /* not a stub */
 }
 
diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
new file mode 100644
index 0000000000000000000000000000000000000000..7f3b40f20c67abfdd2410614e7ee29ae77d37966
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
@@ -0,0 +1,29 @@
+#include <stdio.h>
+extern void func();
+void __acle_se_func ()
+{
+  printf("__acle_se_func\n");
+}
+
+/* The following code is written using asm so that the instructions in function
+ * "func" will be placed in .gnu.sgstubs section of the executable.  */
+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");
+
+void fun1 ()
+{
+  printf("In fun1\n");
+}
+
+int main (void)
+{
+  func();
+  fun1();
+  __acle_se_func();
+  func();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
new file mode 100644
index 0000000000000000000000000000000000000000..3416e887d9ebe5ebc52336eff15ba83a6d16df21
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
@@ -0,0 +1,60 @@
+# 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
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile ]} {
+    return -1
+}
+
+if ![runto_main] {
+   untested "could not run to main"
+   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"
+
+set test "next in __acle_se_func function controls returns to main"
+gdb_test "next" "main ().*" "$test"
+
+set test "next in main outputs In fun1"
+gdb_test "next" "In fun1.*" "$test"
+
+set test "next in main outputs __acle_se_func"
+gdb_test "next" "__acle_se_func.*" "$test"
+
+set test "control jumps to __acle_se_func from main via func"
+gdb_test "step" "__acle_se_func ().*" "${test}"
+
+set test "next in __acle_se_func function via func"
+gdb_test "next" "__acle_se_func.*" "$test"


rb11415.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alan Hayward


> On 19 Jul 2019, at 13:23, Srinath Parvathaneni <[hidden email]> wrote:
>
> Hello Alan, Simon and Tom.
>
> Thank you very much for reviewing my first patch to GDB and providing me your feedback.
> https://sourceware.org/ml/gdb-patches/2019-07/msg00392.html
>
> I have incorporating all your review comments and sending a new patch here.
>
> Hello,
>
> 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).
>
> The above details are published on the Arm website [1], please refer to section 5.4 (Entry functions)
> and section 3.4.4 (C level development flow of secure code).
>
> [1] https://developer.arm.com/architectures/cpu-architecture/m-profile/docs/ecm0359818/latest/armv8-m-security-extensions-requirements-on-development-tools-engineering-specification
>
> 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? If ok, could someone commit this on my behalf,
> I don't have the commit rights.

LGTM.
I tried the patch, and it works for me.

I’ll give it a few days for someone else to comment, and if not, then I'll commit
early next week.


Thanks!
Alan.



>
> gdb/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>
> * gdb/arm-tdep.c (arm_skip_cmse_entry):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.
> (arm_is_sgstubs_section):New function. To check the current section is
> ".gnu.sgstubs".
> (arm_skip_stub):Add call to arm_skip_cmse_entry function.
> * gdb/arm-tdep.h (arm_is_sgstubs_section):New function declaration.
>
> gdb/testsuite/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>
> * gdb.arch/arm-cmse-sgstubs.c: New test.
> * gdb.arch/arm-cmse-sgstubs.exp: New file.
>
>
>
> ###############     Attachment also inlined for ease of reply    ###############
>
>
> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index 23dd40ea8beb1b00289a4cd4e65647399d351580..9482e765a7fc5bb58676096f6b879eae2a7c858e 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);
> +bool arm_is_sgstubs_section (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..e3b7ab6f096eb91da067d772b8798ffd0737e3d6 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8211,6 +8211,53 @@ arm_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>   *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
>   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".  For more details please refer to section 5.4
> +  (Entry functions) and section 3.4.4 (C level development flow of secure code)
> +  of "armv8-m-security-extensions-requirements-on-development-tools-engineering-specification"
> +  document on www.developer.arm.com.  */
> +
> +static CORE_ADDR
> +arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
> +{
> +  struct bound_minimal_symbol minsym;
> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
> +  char *target_name  = (char *) alloca (target_len);
> +  xsnprintf (target_name, target_len, "%s%s", "__acle_se_",name);
> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
> +  if (minsym.minsym != nullptr)
> +    return BMSYMBOL_VALUE_ADDRESS (minsym);
> +  return 0;
> +}
> +
> +/* Return true when sec points to ".gnu.sgstubs" section.  */
> +bool
> +arm_is_sgstubs_section (struct obj_section *sec)
> +{
> + if (sec != nullptr && sec->the_bfd_section != nullptr
> +     && sec->the_bfd_section->name != nullptr
> +     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))
> +   return true;
> + return false;
> +}
>
> /* 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 address pc holds belows to ".gnu.sgstubs" section.  */
> +  if (arm_is_sgstubs_section (section))
> +    return arm_skip_cmse_entry (pc, name, section->objfile);
>   return 0; /* not a stub */
> }
>
> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..7f3b40f20c67abfdd2410614e7ee29ae77d37966
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +extern void func();
> +void __acle_se_func ()
> +{
> +  printf("__acle_se_func\n");
> +}
> +
> +/* The following code is written using asm so that the instructions in function
> + * "func" will be placed in .gnu.sgstubs section of the executable.  */
> +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");
> +
> +void fun1 ()
> +{
> +  printf("In fun1\n");
> +}
> +
> +int main (void)
> +{
> +  func();
> +  fun1();
> +  __acle_se_func();
> +  func();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> new file mode 100644
> index 0000000000000000000000000000000000000000..3416e887d9ebe5ebc52336eff15ba83a6d16df21
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> @@ -0,0 +1,60 @@
> +# 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
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile ]} {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +   untested "could not run to main"
> +   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"
> +
> +set test "next in __acle_se_func function controls returns to main"
> +gdb_test "next" "main ().*" "$test"
> +
> +set test "next in main outputs In fun1"
> +gdb_test "next" "In fun1.*" "$test"
> +
> +set test "next in main outputs __acle_se_func"
> +gdb_test "next" "__acle_se_func.*" "$test"
> +
> +set test "control jumps to __acle_se_func from main via func"
> +gdb_test "step" "__acle_se_func ().*" "${test}"
> +
> +set test "next in __acle_se_func function via func"
> +gdb_test "next" "__acle_se_func.*" "$test"
>
> <rb11415.patch>

Reply | Threaded
Open this post in threaded view
|

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

Srinath Parvathaneni


On 7/19/19 3:58 PM, Alan Hayward wrote:

>
>
>> On 19 Jul 2019, at 13:23, Srinath Parvathaneni <[hidden email]> wrote:
>>
>> Hello Alan, Simon and Tom.
>>
>> Thank you very much for reviewing my first patch to GDB and providing me your feedback.
>> https://sourceware.org/ml/gdb-patches/2019-07/msg00392.html
>>
>> I have incorporating all your review comments and sending a new patch here.
>>
>> Hello,
>>
>> 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).
>>
>> The above details are published on the Arm website [1], please refer to section 5.4 (Entry functions)
>> and section 3.4.4 (C level development flow of secure code).
>>
>> [1] https://developer.arm.com/architectures/cpu-architecture/m-profile/docs/ecm0359818/latest/armv8-m-security-extensions-requirements-on-development-tools-engineering-specification
>>
>> 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? If ok, could someone commit this on my behalf,
>> I don't have the commit rights.
>
> LGTM.
> I tried the patch, and it works for me.

Thank you for the review.


> I’ll give it a few days for someone else to comment, and if not, then I'll commit
> early next week.

Understood and thank you once again.


> Thanks!
> Alan.
>
>
>
>>
>> gdb/ChangeLog:
>>
>> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>>
>> * gdb/arm-tdep.c (arm_skip_cmse_entry):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.
>> (arm_is_sgstubs_section):New function. To check the current section is
>> ".gnu.sgstubs".
>> (arm_skip_stub):Add call to arm_skip_cmse_entry function.
>> * gdb/arm-tdep.h (arm_is_sgstubs_section):New function declaration.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>>
>> * gdb.arch/arm-cmse-sgstubs.c: New test.
>> * gdb.arch/arm-cmse-sgstubs.exp: New file.
>>
>>
>>
>> ###############     Attachment also inlined for ease of reply    ###############
>>
>>
>> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
>> index 23dd40ea8beb1b00289a4cd4e65647399d351580..9482e765a7fc5bb58676096f6b879eae2a7c858e 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);
>> +bool arm_is_sgstubs_section (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..e3b7ab6f096eb91da067d772b8798ffd0737e3d6 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -8211,6 +8211,53 @@ arm_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>>    *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
>>    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".  For more details please refer to section 5.4
>> +  (Entry functions) and section 3.4.4 (C level development flow of secure code)
>> +  of "armv8-m-security-extensions-requirements-on-development-tools-engineering-specification"
>> +  document on www.developer.arm.com.  */
>> +
>> +static CORE_ADDR
>> +arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
>> +{
>> +  struct bound_minimal_symbol minsym;
>> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
>> +  char *target_name  = (char *) alloca (target_len);
>> +  xsnprintf (target_name, target_len, "%s%s", "__acle_se_",name);
>> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
>> +  if (minsym.minsym != nullptr)
>> +    return BMSYMBOL_VALUE_ADDRESS (minsym);
>> +  return 0;
>> +}
>> +
>> +/* Return true when sec points to ".gnu.sgstubs" section.  */
>> +bool
>> +arm_is_sgstubs_section (struct obj_section *sec)
>> +{
>> + if (sec != nullptr && sec->the_bfd_section != nullptr
>> +     && sec->the_bfd_section->name != nullptr
>> +     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))
>> +   return true;
>> + return false;
>> +}
>>
>> /* 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 address pc holds belows to ".gnu.sgstubs" section.  */
>> +  if (arm_is_sgstubs_section (section))
>> +    return arm_skip_cmse_entry (pc, name, section->objfile);
>>    return 0;/* not a stub */
>> }
>>
>> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..7f3b40f20c67abfdd2410614e7ee29ae77d37966
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
>> @@ -0,0 +1,29 @@
>> +#include <stdio.h>
>> +extern void func();
>> +void __acle_se_func ()
>> +{
>> +  printf("__acle_se_func\n");
>> +}
>> +
>> +/* The following code is written using asm so that the instructions in function
>> + * "func" will be placed in .gnu.sgstubs section of the executable.  */
>> +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");
>> +
>> +void fun1 ()
>> +{
>> +  printf("In fun1\n");
>> +}
>> +
>> +int main (void)
>> +{
>> +  func();
>> +  fun1();
>> +  __acle_se_func();
>> +  func();
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..3416e887d9ebe5ebc52336eff15ba83a6d16df21
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
>> @@ -0,0 +1,60 @@
>> +# 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
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile ]} {
>> +    return -1
>> +}
>> +
>> +if ![runto_main] {
>> +   untested "could not run to main"
>> +   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"
>> +
>> +set test "next in __acle_se_func function controls returns to main"
>> +gdb_test "next" "main ().*" "$test"
>> +
>> +set test "next in main outputs In fun1"
>> +gdb_test "next" "In fun1.*" "$test"
>> +
>> +set test "next in main outputs __acle_se_func"
>> +gdb_test "next" "__acle_se_func.*" "$test"
>> +
>> +set test "control jumps to __acle_se_func from main via func"
>> +gdb_test "step" "__acle_se_func ().*" "${test}"
>> +
>> +set test "next in __acle_se_func function via func"
>> +gdb_test "next" "__acle_se_func.*" "$test"
>>
>> <rb11415.patch>
>
Reply | Threaded
Open this post in threaded view
|

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

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

Srinath> I have incorporating all your review comments and sending a new
Srinath> patch here.

Thank you for the patch.

I have some trivial nits but otherwise nothing else.  I don't know much
about ARM but if Alan thinks it is ready to go in, then I agree :-)

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

Should be a space after the "," before "name".

Srinath> +     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))

Space after the "," here too.

Srinath> +  /* checks whether address pc holds belows to ".gnu.sgstubs" section.  */

Should be "Checks" -- i.e., upper cased.

Tom
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
In reply to this post by Srinath Parvathaneni
Hi Srinath,

I looked at the patch in more details, I just have some minor comments about formatting.

> gdb/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>
> * gdb/arm-tdep.c (arm_skip_cmse_entry):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.

You can use this ChangeLog entry:

        * arm-tdep.c (arm_skip_cmse_entry): New function.

Note:

- Keep the entry succinct, about what changed ("New function." is typical).  You explained well what the code does in the commit message, which is good.
- Remove gdb/, as you want the filename to be relative to the location of the ChangeLog file (gdb/ChangeLog)
- Space after the colon.

> (arm_is_sgstubs_section):New function. To check the current section is
> ".gnu.sgstubs".

Here too, just "New function.".

> (arm_skip_stub):Add call to arm_skip_cmse_entry function.
> * gdb/arm-tdep.h (arm_is_sgstubs_section):New function declaration.
>
> gdb/testsuite/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <[hidden email]>
>
> * gdb.arch/arm-cmse-sgstubs.c: New test.
> * gdb.arch/arm-cmse-sgstubs.exp: New file.
>
>
>
> ###############     Attachment also inlined for ease of reply    ###############
>
>
> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index
> 23dd40ea8beb1b00289a4cd4e65647399d351580..9482e765a7fc5bb58676096f6b879eae2a7c858e
> 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);
> +bool arm_is_sgstubs_section (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..e3b7ab6f096eb91da067d772b8798ffd0737e3d6
> 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8211,6 +8211,53 @@ arm_get_longjmp_target (struct frame_info
> *frame, CORE_ADDR *pc)
>    *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
>    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".  For more details please refer to section 5.4
> +  (Entry functions) and section 3.4.4 (C level development flow of secure code)
> +  of
> "armv8-m-security-extensions-requirements-on-development-tools-engineering-specification"
> +  document on www.developer.arm.com.  */
> +
> +static CORE_ADDR
> +arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
> +{
> +  struct bound_minimal_symbol minsym;
> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
> +  char *target_name  = (char *) alloca (target_len);

Extra space before the equal sign.

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

Space after comma.

> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
> +  if (minsym.minsym != nullptr)
> +    return BMSYMBOL_VALUE_ADDRESS (minsym);
> +  return 0;
> +}

Not a strict rule, but my personal taste would be to add a few empty lines between logical blocks in the
function above, to space things a bit.  I think it would help readability.  Something like:

static CORE_ADDR
arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
{
  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
  char *target_name = (char *) alloca (target_len);
  xsnprintf (target_name, target_len, "%s%s", "__acle_se_", name);

  bound_minimal_symbol minsym
    = lookup_minimal_symbol (target_name, NULL, objfile);

  if (minsym.minsym != nullptr)
    return BMSYMBOL_VALUE_ADDRESS (minsym);

  return 0;
}

> +
> +/* Return true when sec points to ".gnu.sgstubs" section.  */
> +bool
> +arm_is_sgstubs_section (struct obj_section *sec)
> +{
> + if (sec != nullptr && sec->the_bfd_section != nullptr
> +     && sec->the_bfd_section->name != nullptr
> +     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))
> +   return true;
> + return false;
> +}

For non-static functions, what we do is, in the .c file:

/* See arm-tdep.h.  */

bool
arm_is_sgstubs_section (struct obj_section *sec)
{
  ...
}

And in the .h file, put the doc:

/* Return true when SEC points to the ".gnu.sgstubs" section.  */

bool arm_is_sgstubs_section (struct obj_section *);

I know the current code in arm-tdep.h/arm-tdep.c is not all like that, but we try to be
consistent for new code or code that we modify.

Note: when referring to a parameter name, we put it in caps (SEC).

But actually, I think this function doesn't need to be visible to other files, as it's only
used in arm-tdep.c.  So should it be static?

Watch the indentation, the function body is indented with a single space, it should be two.
Also, it can be simplified to a single return statement:

  return (sec != nullptr
          && sec->the_bfd_section != nullptr
          && sec->the_bfd_section->name != nullptr
          && streq (sec->the_bfd_section->name,".gnu.sgstubs"));

>  /* 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;

You can declare this variable at the point where you use it.

>
>    /* 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 address pc holds belows to ".gnu.sgstubs" section.  */

Capital C to "checks".  Also, I don't really understand the sentence, could it be
clarified?  Alan, maybe you can help with this?

> +  if (arm_is_sgstubs_section (section))
> +    return arm_skip_cmse_entry (pc, name, section->objfile);
>    return 0; /* not a stub */
>  }
>
> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..7f3b40f20c67abfdd2410614e7ee29ae77d37966
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +extern void func();
> +void __acle_se_func ()
> +{
> +  printf("__acle_se_func\n");
> +}
> +
> +/* The following code is written using asm so that the instructions in function
> + * "func" will be placed in .gnu.sgstubs section of the executable.  */
> +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");
> +
> +void fun1 ()
> +{
> +  printf("In fun1\n");
> +}
> +
> +int main (void)
> +{
> +  func();
> +  fun1();
> +  __acle_se_func();
> +  func();
> +  return 0;
> +}

We try to format the test source files according to GNU standards, just like our source files.  That means:

- Return type on its own line
- Space before parenthesis in declarations, definitions and calls
- Copyright header at the top

> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..3416e887d9ebe5ebc52336eff15ba83a6d16df21
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> @@ -0,0 +1,60 @@
> +# 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
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile ]} {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +   untested "could not run to main"
> +   return -1
> +}
> +
> +set test "branch to func from main"
> +gdb_test "si" "0x.*" "$test"

I would suggest just putting the test name on the gdb_test line, as we do everywhere:

  gdb_test "si" "0x.*" "branch to func from main"

If the same test name is used at multiple places, then it's worth putting it in a variable
to avoid duplicating it.

Thanks,

Simon