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

Walfred Tedeschi
2016-02-08T12:55:02Z

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