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

[Walfred Tedeschi <walfred.tedeschi@intel.com>]

Am 2/7/2016 um 8:06 AM schrieb Joel Brobecker:
> 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,

Thanks for your review!
I will address your comments for the next version.

Regards,
-Fred
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928