Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

Joel Brobecker
2016-02-07T07:06:39Z

On Wed, Jan 13, 2016 at 01:39:59PM +0100, Walfred Tedeschi wrote:
> When using the return command, execution of a function is aborted
> and present values are returned from that point. That can cause
> bound violations in the MPX context. To avoid such side-effects

comma at the end of this line: "To avoid such side-effects,"...

> a new set variable was added "mpx-bnd-init-on-return" which controls
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

a new setting ("mpx-bnd-init-on-return") was added to control

> the initialization of bound register when using the return command.
>
> As bound initialization it is understood the set of the BND register
> to zero allowing the associated pointer to access the whole memory.

This sentence is not correct English, I don't think. I think you can
remove it entirely if you modify the next sentences as follow (note,
this is a boolean setting, so use "true" or "false" to describe its
value rather than numeric values)...

> As default the value of "mpx-bnd-init-on-return" is set to 1. So
> bound register are initilized when using the "return" command.

If set to True (the default), all bound registers are set to zero
when using the "return" command.

> 2016-01-13 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> * i386-tdep.c (mpx_bnd_init_on_return): new external variable.
> (show_mpx_init_on_return): New function.
> (_initialize_i386_tdepamd64_init_abi): Add new commands set/show
> mpx-bnd-init-on-return.
> * amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return.
> * NEWS: Add entry for set/show mpx-bnd-init-on-return command.
>
> gdb/doc/ChangeLog:
>
> * gdb.texinfo (Intel Memory Protection Extensions): Add
> documentation on using set/show mpx-bnd-init-on-return.
>
> gdb/testsuite/ChangeLog:
>
> * amd64-mpx-call.c: New file.
> * amd64-mpx-call.exp: New file.
>
> ---
> gdb/NEWS | 4 +
> gdb/amd64-tdep.c | 32 ++++++++
> gdb/doc/gdb.texinfo | 12 +++
> gdb/i386-tdep.c | 27 +++++++
> gdb/testsuite/gdb.arch/amd64-mpx-call.c | 126 ++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.arch/amd64-mpx-call.exp | 90 +++++++++++++++++++++
> 6 files changed, 291 insertions(+)
> create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
> create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a222dfb..80022ec 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>
> *** Changes since GDB 7.10
>
> +* show mpx-bnd-init-on-return
> + set mpx-bnd-init-on-return on i386 and amd64
> + Support for set bound registers to INIT state when using the command return.
> +
> * Record btrace now supports non-stop mode.
>
> * Support for tracepoints on aarch64-linux was added in GDBserver.
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 732e91b..0ce41d1 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
> gdb_byte *readbuf, const gdb_byte *writebuf)
> {
> enum amd64_reg_class theclass[2];
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> int len = TYPE_LENGTH (type);
> static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
> static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
> int integer_reg = 0;
> int sse_reg = 0;
> int i;
> + extern int mpx_bnd_init_on_return;

Please avoid these kinds of declarations like the pest. Normally,
what we should be doing to prevent this is to declare this extern
in a .h file, and then include that .h file from all the .c file
referencing that global. This way, the compiler can keep all the
uses consistent. Otherwise, some can change the type of that
variable and the compiler would compile the above incorrectly.

In this case, since this setting is only accessed from amd64-tdep,
I suggest you move this global, as well as the code creating the new
settting, inside amd64-tdep.c. This will avoid the need for an extern.
Please make mpx_bnd_init_on_return static as well.

> + if the return value we are setting is due to the use of the
> + "return" command (WRITEBUF is not NULL), it is likely that
> + the return is made prematurely, and therefore bnd0 register
> + unset could then cause the program to receive a spurious
> + bound violation, but this is only done if
> + "set mpx-bnd-init-on-return" is true. */

Small nit (reminder): All sentences should start with a capital letter.
Thus "If the...".

I think some bits were lost in translation, though. Please replace
the text above by the following:

If the return value we are setting is due to the use of the
"return" command (WRITEBUF is not NULL), it is likely that
the return is made prematurely. In that situation, it is
possible that the bnd0 register could still be unset, thus
causing the program to receive a spurious bound violation.

When "set mpx-bnd-init-on-return" is true, prevent this
from happening by setting the bnd0 register to zero. */

> + if (writebuf != NULL
> + && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
> + {
> + gdb_byte bnd_buf[16];
> +
> + memset (bnd_buf, 0, 16);
> + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf);
> + }
> }
>
> return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
> writebuf + i * 8);
> }
>
> + if (I387_BND0R_REGNUM (tdep) > 0)
> + {
> + gdb_byte bnd_buf[16];
> + int i, init_bnd;
> +
> + memset (bnd_buf, 0, 16);
> + if (writebuf && mpx_bnd_init_on_return)
> + for (i = 0; i < I387_NUM_BND_REGS; i++)
> + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
> + }

I suggest you use the same pattern as above (move the "if (writebuf
&& mpx_bnd_init_on_return)" to the first condition. It will be more
consistent with the other block of code you added, making it easier
to understand, and will also avoid a call to memset for nothing if
mpx_bnd_init_on_return is unset. Please also add a small comment
explaining why you do this. Thus:

/* Same as in the class AMD64_MEMORY case above, where
we set bnd0 to zero when doing a premature "return"
and "set mpx-bnd-init-on-return" is true: Set all bound
registers to zero in order to avoid spurious bound
violations. */

if (writebuf != NULL
&& mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
{
...
}

> +/* The value of the "set mpx-bnd-init-on-return setting. */
> +
> +int mpx_bnd_init_on_return = 1;
> +
> +/* Show state of the setting variable mpx-bnd-init-on-return. */
> +
> +static void
> +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
> + struct cmd_list_element *c, const char *value)
> +{
> + if (mpx_bnd_init_on_return)
> + fprintf_filtered (file,
> + _("BND registers will be initialized on return.\n"));
> + else
> + fprintf_filtered (file,
> + _("BND registers will not be initialized on return.\n"));
> +}
> +
> static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist;
>
> /* Helper function for the CLI commands. */
> @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."),
> in the bound table.",
> &mpx_set_cmdlist);
>
> + add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
> + &mpx_bnd_init_on_return, _("\
> + Set the bnd registers to INIT state when returning from a call."), _("\
> + Show the state of the mpx-bnd-init-on-return."),
> + NULL,
> + NULL,
> + show_mpx_bnd_init_on_return,
> + &setlist, &showlist);

As suggested above, please move all this code to amd64-tdep.c instead.

> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> + verbose "Skipping x86 MPX tests."
> + return

There is something I don't understand. Why are we running this test
on i?x86 if the bnd register handling is done in amd64-tdep (and
therefore seems limited to x86_64?

> +# Needed by the return command.
> +gdb_test_no_output "set confirm off"
> +
> +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"\

Add one space before the equal sign, please; here and in all tests
below. Thank you!

> + "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"
> +
> +set break "bkpt 2."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
> + bnd0 result in a convenience variable 1"

Let's not use the empty string for the match, but instead just
verify that the command returned some reasonable that looks reasonable.
Also, I would avoid splitting the test label (string) in two, by
formatting the test as:

gdb_test "p \$bound0 = \$bnd0" " = {<fill the correct regexp here>}" \
"Saving the bnd0 result in a convenience variable 1"

As a side-effect, this will avoid having 4 spaces between "the" and
"bnd0".

Please apply these two recommendations in the tests below as well.

> +gdb_test "return a" "#0 $hex in main.*" "First return"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> + "= 0" "return with bnd initialization off - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> + "= 0" "return with bnd initialization off - lbound"
> +
> +# Retesting with initialization off.
> +# All breakpoints were deleted need to recreate what we need.
> +runto_main
> +gdb_test_no_output "set mpx-bnd-init-on-return off"\
> + "Turn off initialization on return"
> +
> +set break "bkpt 3."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
> + bnd0 result in a convenience variable 2"
> +gdb_test "return a" "#0 $hex in main.*" "Second return"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> + "= 1" "return with bnd initialization on - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> + "= 1" "return with bnd initialization on - lbound"
> +
> +gdb_test "show mpx-bnd-init-on-return"\
> + "BND registers will not be initialized on return\\."\
> + "double check initialization on return"

Thank you,
--
Joel