[v3,ARM] : gdb cannot step across CMSE secure entry function code.

Message ID DBBPR08MB47759CB06F588942CAC222FE9BC40@DBBPR08MB4775.eurprd08.prod.outlook.com
State New, archived
Headers

Commit Message

Srinath Parvathaneni July 22, 2019, 11:01 a.m. UTC
  Thank you Simon, Tom and Alan for the detailed review.
I appreciate you for reviewing the patch very patiently and providing your feedback.
I have incorparated all your review comments in this new patch.

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 please commit this on my behalf,
I don't have the commit rights.

gdb/ChangeLog:

2019-07-22  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

	* arm-tdep.c (arm_skip_cmse_entry): New function.
	(arm_is_sgstubs_section): New function.
	(arm_skip_stub): Add call to arm_skip_cmse_entry function.

gdb/testsuite/ChangeLog:

2019-07-22  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>

	* 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.c b/gdb/arm-tdep.c
index d244707210628ab045f677c0cbad3d8b0c6d6269..2d5dd02a369508d7ae1125354ef4f2f713dd9695 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8211,6 +8211,56 @@ 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)
+{
+  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);
+
+  struct 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.  */
+
+static bool
+arm_is_sgstubs_section (struct obj_section *sec)
+{
+  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.  */
@@ -8290,6 +8340,12 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
 	return 0;
     }
 
+  struct obj_section *section = find_pc_section (pc);
+
+  /* Check whether SECTION points to the ".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..3ac8e6e45767e29534db0fb0c9053f00dad19d90
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
@@ -0,0 +1,50 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#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..42a265ca67697f67fa5e530d7fd64a7f6fd69929
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
@@ -0,0 +1,50 @@
+# 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
+}
+
+gdb_test "si" "0x.*" "branch to func from main"
+
+gdb_test "ni" "0x.*" "next instruction in func"
+
+gdb_test "ni" "__acle_se_func ().*" "branch to __acle_se_func from func"
+
+gdb_test "next" "23	  .*" "next in __acle_se_func function"
+
+gdb_test "next" "__acle_se_func.*" "next in __acle_se_func function outputs __acle_se_func"
+
+gdb_test "next" "main ().*" "next in __acle_se_func function controls returns to main"
+
+gdb_test "next" "In fun1.*" "next in main outputs In fun1"
+
+gdb_test "next" "__acle_se_func.*" "next in main outputs __acle_se_func"
+
+gdb_test "step" "__acle_se_func ().*" "control jumps to __acle_se_func from main via func"
+
+gdb_test "next" "__acle_se_func.*" "next in __acle_se_func function via func"
  

Comments

Simon Marchi July 22, 2019, 3:58 p.m. UTC | #1
Hi Srinath,

Just a formatting nit:

> +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);
> +
> +  struct bound_minimal_symbol minsym
> +   = lookup_minimal_symbol (target_name, NULL, objfile);

Missing one space here, before the equal.  No need to send a new version for this
if everything else is fine, just make sure to fix it before pushing.

Thanks,

Simon
  
Alan Hayward July 23, 2019, 11:16 a.m. UTC | #2
> On 22 Jul 2019, at 16:58, Simon Marchi <simark@simark.ca> wrote:

> 

> Hi Srinath,

> 

> Just a formatting nit:

> 

>> +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);

>> +

>> +  struct bound_minimal_symbol minsym

>> +   = lookup_minimal_symbol (target_name, NULL, objfile);

> 

> Missing one space here, before the equal.  No need to send a new version for this

> if everything else is fine, just make sure to fix it before pushing.

> 


I’ve pushed this on behalf of Srinath.


Thanks!
Alan.
  

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d244707210628ab045f677c0cbad3d8b0c6d6269..2d5dd02a369508d7ae1125354ef4f2f713dd9695 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8211,6 +8211,56 @@  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)
+{
+  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);
+
+  struct 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.  */
+
+static bool
+arm_is_sgstubs_section (struct obj_section *sec)
+{
+  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.  */
@@ -8290,6 +8340,12 @@  arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
 	return 0;
     }
 
+  struct obj_section *section = find_pc_section (pc);
+
+  /* Check whether SECTION points to the ".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..3ac8e6e45767e29534db0fb0c9053f00dad19d90
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
@@ -0,0 +1,50 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#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..42a265ca67697f67fa5e530d7fd64a7f6fd69929
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
@@ -0,0 +1,50 @@ 
+# 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
+}
+
+gdb_test "si" "0x.*" "branch to func from main"
+
+gdb_test "ni" "0x.*" "next instruction in func"
+
+gdb_test "ni" "__acle_se_func ().*" "branch to __acle_se_func from func"
+
+gdb_test "next" "23	  .*" "next in __acle_se_func function"
+
+gdb_test "next" "__acle_se_func.*" "next in __acle_se_func function outputs __acle_se_func"
+
+gdb_test "next" "main ().*" "next in __acle_se_func function controls returns to main"
+
+gdb_test "next" "In fun1.*" "next in main outputs In fun1"
+
+gdb_test "next" "__acle_se_func.*" "next in main outputs __acle_se_func"
+
+gdb_test "step" "__acle_se_func ().*" "control jumps to __acle_se_func from main via func"
+
+gdb_test "next" "__acle_se_func.*" "next in __acle_se_func function via func"