This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH V6] amd64-mpx: initialize bnd register before performing inferior calls.


This patch initializes the bnd registers before executing the inferior
call.  BND registers can be in arbitrary values at the moment of the
inferior call.  In case the function being called uses as part of the
parameters bnd register, e.g. when passing a pointer as parameter, the
current value of the register will be used, without taking into account
the pointers being passed.  This can cause boundary violations that are
not due to a real bug or even desired by the user.  In this sense the
best to be done is set the bnd registers to allow
access to the whole memory, i.e. initialized state, before pushing the
inferior call.

In case the user would like to set the bound registers experimentally,
investigating running conditions, it is possible to set a break point
at the beginning of the function and perform the setup there, as usual.

Aside of that having in mind that 95% of the usage cases for inferior
calls are related to the output of the function this patch improves
that for the MPX case.

Changes in behavior using the test provided as example:

As is:
(gdb) c
Continuing.
Breakpoint 2, main () at i386-mpx-call.c:116
116	      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
(gdb)  p upper(x,a,b,c,d,0)
Program received signal SIGSEGV, Segmentation fault
Lower bound violation while accessing address 0x603010
Bounds: [lower = 0x603090, upper = 0x6030a3].
0x0000000000400a5d in upper (p=0x603010,...
71	  value = *(p + len);


With the patch:
(gdb) c
Continuing.
Breakpoint 2, main () at i386-mpx-call.c:116
116	      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
(gdb) p upper(x,a,b,c,d,0)
$1 = 1


Comments:
* Patch has been discussed together with an additional patch.
Since it can be reviewed and approved separately I am resubmitting it.
* The present patch is the last version already posted.
* Commit comment was also improved, former commit message was not
explaining the real reason behind the patch.
* Added a test related to this commit only (test was only on the second
ABI patch).
* I realized also that i was owning some aswer to Yao, related to:
https://sourceware.org/ml/gdb-patches/2016-04/msg00574.html

Thanks for the review!

In case i could not answer your comments with the rewriting of the
commit message.

For the breakpoint case pointed by Yao, user has added intentionally
where it should be hit. GDB behavior at this point is completely right.
In the bound violation case, that is also expected but we intend with the
present patch provide a more deterministic behavior for the whole scenario.

In terms of the initialization of the bnd registers the situations is a
bit different.

Yao, sorry also for forwarding instead of answering. Internal issues
with e-mail client, finally i was able to configure my client again.

2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>

gdb/ChangeLog:

	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.
	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.

gdb/testsuite/ChangeLog:

	* i386-mpx-call.c: New file.
	* i386-mpx-call.exp: New file.

---
 gdb/amd64-tdep.c                         |   5 ++
 gdb/i386-tdep.c                          |   5 ++
 gdb/i387-tdep.c                          |  16 +++++
 gdb/i387-tdep.h                          |   6 ++
 gdb/testsuite/gdb.arch/i386-mpx-call.c   | 107 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-mpx-call.exp |  58 +++++++++++++++++
 6 files changed, 197 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.exp

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index a636742..646a16d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -999,6 +999,11 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   gdb_byte buf[8];
 
+  /* When MPX is enabled, all bnd registers have to be initialized
+     before the call.  This avoids an undesired bound violation
+     during the function's execution.  */
+  i387_reset_bnd_regs (gdbarch, regcache);
+
   /* Pass arguments.  */
   sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 8a4d59f..6e0c1b4 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2663,6 +2663,11 @@ i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   int write_pass;
   int args_space = 0;
 
+  /* When MPX is enabled, all bnd registers have to be initialized
+     before the call.  This avoids an undesired bound violation
+     during the function's execution.  */
+  i387_reset_bnd_regs (gdbarch, regcache);
+
   /* Determine the total space required for arguments and struct
      return address in a first pass (allowing for 16-byte-aligned
      arguments), then push arguments in a second pass.  */
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index adbe721..a0d1587 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -1772,3 +1772,19 @@ i387_return_value (struct gdbarch *gdbarch, struct regcache *regcache)
   regcache_raw_write_unsigned (regcache, I387_FTAG_REGNUM (tdep), 0x3fff);
 
 }
+
+void
+i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i;
+
+      memset (bnd_buf, 0, 16);
+      for (i = 0; i < I387_BND0R_REGNUM (tdep); i++)
+	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
+    }
+}
diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
index 81d45b7..6261da0 100644
--- a/gdb/i387-tdep.h
+++ b/gdb/i387-tdep.h
@@ -156,4 +156,10 @@ extern void i387_collect_xsave (const struct regcache *regcache,
 extern void i387_return_value (struct gdbarch *gdbarch,
 			       struct regcache *regcache);
 
+/* Set all bnd registers to the INIT state. INIT state means
+   all memory range can be accessed.  */
+
+extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
+			         struct regcache *regcache);
+
 #endif /* i387-tdep.h */
diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
new file mode 100644
index 0000000..a0de3b1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
@@ -0,0 +1,107 @@
+/* Test for inferior function calls MPX context.
+
+   Copyright (C) 2017 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>
+#include "x86-cpuid.h"
+
+#define OUR_SIZE    5
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+int
+upper (int *p, int *a, int *b, int *c, int *d, int len)
+{
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+
+  value = value - value + 1;
+  return value;
+}
+
+int *
+upper_ptr (int *p, int *a, int *b, int *c, int *d, int len)
+{
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+  free (p);
+  p = calloc (OUR_SIZE * 2, sizeof (int));
+
+  return ++p;
+}
+
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      int sx[OUR_SIZE];
+      int sa[OUR_SIZE];
+      int sb[OUR_SIZE];
+      int sc[OUR_SIZE];
+      int sd[OUR_SIZE];
+      int *x, *a, *b, *c, *d;
+
+      x = calloc (OUR_SIZE, sizeof (int));
+      a = calloc (OUR_SIZE, sizeof (int));
+      b = calloc (OUR_SIZE, sizeof (int));
+      c = calloc (OUR_SIZE, sizeof (int));
+      d = calloc (OUR_SIZE, sizeof (int));
+
+      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
+      x = upper_ptr (sx, sa, sb, sc, sd, 0);
+
+      free (x);
+      free (a);
+      free (b);
+      free (c);
+      free (d);
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.exp b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
new file mode 100644
index 0000000..025f7ad
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
@@ -0,0 +1,58 @@
+# Copyright (C) 2017 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/>.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
+
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+    [list debug nowarnings additional_flags=${comp_flags}]] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test_multiple "print have_mpx ()" "have mpx" {
+    -re ".*= 1\r\n$gdb_prompt " {
+        pass "check whether processor supports MPX"
+    }
+    -re ".*= 0\r\n$gdb_prompt " {
+        verbose "processor does not support MPX; skipping MPX tests"
+        return
+    }
+}
+
+# Needed by the return command.
+gdb_test_no_output "set confirm off"
+
+set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
+
+set break "bkpt 1."
+gdb_breakpoint [gdb_get_line_number "${break}"]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p upper (x, a, b, c, d, 0)" " = 1"\
+    "test the call of int function - int"
+gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
+    " = \\\(int \\\*\\\) $hex" "test the call of function - ptr"
+
-- 
2.9.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]