[PATCH V8] amd64-mpx: initialize BND register before performing inferior calls.

[Pedro Alves]

On 02/21/2017 04:40 PM, Walfred Tedeschi wrote:

> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -22531,6 +22531,36 @@ whose bounds are to be changed, @var{lbound} and @var{ubound} are new values
>  for lower and upper bounds respectively.
>  @end table
>  
> +Calling inferior functions on Intel MPX enabled program can generate bound

Either "on an Intel MPX enabled program" or
       "on Intel MPX enabled programs".

> +violations.  

This sentence is scary.  Makes me not ever want to call functions.  :-)
They don't generate violations, because you're fixing GDB to not do that!
I think it'd be better if the manual spoke to the user, not to
the implementer.

Here's an alternative version of your text changed in that direction,
along with a few adjustments and grammar tweaks:

~~~
When you call an inferior function on an Intel MPX enabled program,
GDB sets the inferior's bound registers to the init (disabled) state
before calling the function.  As a consequence, bounds checks for the
pointer arguments passed to the function will always pass.

This is necessary because when you call an inferior function, the
program is usually in the middle of the execution of other function.
Since at that point bound registers are in an arbitrary state, not
clearing them would lead to random bound violations in the called
function.

You can still examine the influence of the bound registers on the
execution of the called function by stopping the execution of the
called function at its prologue, setting bound registers, and
continuing the execution.  For example:
~~~

On the GDB code itself, seems like you missed/forgot most of
my comments on v7.

So I'll just focus on the new things.  Please go back and
make sure the comments are addressed.

> +
> +
> +char char_upper (char *str, int lenght)

Line break before char_upper.  Typo: "length".

> +{
> +  char ch;
> +  ch = *(str + lenght);
> +
> +  return ch;
> +}
> +
> +char char_lower (char *str, int lenght)

Line break before char_lower.  Typo: "length".

> +{
> +  char ch;
> +  ch = *(str - lenght);
> +
> +  return ch;
> +}
> +


> +
> +# Need for returning from an inferior call that causes a BND violation.

"Needed".  It's not really needed though...  Just convenient here.

> +gdb_test_no_output "set confirm off"
> +
> +# Convenience variable.
> +#
> +set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
> +set int_braw_reg " = \\\{lbound = 0x0, ubound_raw = 0x0\\\}.*"
> +set bndcfg_reg " = \\\{raw = $hex, config = \\\{base = $hex, reserved = $hex,\
> +               preserved = $hex, enabled = $hex\\\}\\\}"
> +set bndstatus_reg  " = \\\{raw = $hex, status = \\\{bde = $hex,\
> +                    error = $hex\\\}\\\}"
> +set u_fault [multi_line "Program received signal SIGSEGV, Segmentation fault" \
> +                        "Upper bound violation while accessing address $hex" \
> +                        "Bounds: \\\[lower = $hex, upper = $hex\\\]"]
> +
> +
> +
> +# Simplify the tests below.
> +#
> +proc sanity_check_bndregs {arglist} {
> +
> +    global int_braw_reg
> +
> +    foreach a $arglist {
> +        gdb_test "p /x $a" "$int_braw_reg"\
> +            "$a"
> +    }
> +}
> +
> +# Set bnd register to have no access to memory.
> +#
> +proc remove_memory_access {reg} {
> +    global hex
> +
> +    sanity_check_bndregs {"\$bnd0raw" "\$bnd1raw" "\$bnd2raw" "\$bnd3raw"}
> +
> +    gdb_test "p /x $reg.lbound = $reg.ubound" "= $hex"\
> +        "$reg changed"
> +    gdb_test "p /x $reg.ubound = 0" " = 0x0"\
> +        "$reg changed"

Duplicate test messages:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

> +}
> +
> +
> +# Prepare convenience variables for bndconfig and status
> +# for posterior comparison

Period at the end of sentences.

> +#
> +proc prepare_bndcfg_bndstatus { } {
> +
> +    global bndcfg_reg
> +    global bndstatus_reg
> +
> +    gdb_test "p /x \$temp_bndcfgu = \$bndcfgu" "$bndcfg_reg"\
> +        "bndcfgu should not change"
> +
> +    gdb_test "p /x \$temp_bndstatus = \$bndstatus" "$bndstatus_reg"\
> +        "bndstatus should not change"
> +}
> +
> +# Compare values set for convenience variables and actual values of bndconfig
> +# and bndstatus registers.
> +#
> +proc compare_bndstatus_with_convenience {} {
> +
> +    gdb_test "p \$temp_bndcfgu == \$bndcfgu" "= 1"\
> +        "bndcfgu compare before and after"
> +    gdb_test "p \$temp_bndstatus == \$bndstatus" "= 1"\
> +        "bndstatus compare before and after"
> +
> +}
> +
> +# Perform an inferior call defined in func.
> +#
> +proc perform_a_call { func } {
> +
> +    global inf_call_stopped
> +    global gdb_prompt
> +
> +    gdb_test_multiple "p /x $func" "Inferior call test" {

lowercase.

> +        -re $inf_call_stopped {

$inf_call_stopped doesn't end in $gdb_prompt.

> +            pass "stopped within an inferior call"

It's better to make the messages consistent.  ("Inferior call test"
vs "stopped within an inferior call".

> +        }
> +        -re ".. = 0\r\n$gdb_prompt " {
> +            verbose "stopped within an inferior call"

Why is this verbose instead of fail?

> +            return -1
> +        }

This is not returning -1 (explicitly anyway) if gdb_test_multiple
detects a fail in its internal matches.  Not clear whether you
mean to propagate the result of gdb_test_multiple implicitly.
But in any case, nothing is using the result of perform_a_call,
so just remove the return.

And with all that, ISTM to that this can be a plain gdb_test.
And while at it, $inf_call_stopped isn't used anywhere else,
and could thus be inlined here.

> +
> +}
> +
> +
> +# Perform an inferior call defined in func.
> +#
> +proc perform_a_call { func } {
> +
> +    global inf_call_stopped
> +    global gdb_prompt
> +
> +    gdb_test_multiple "p /x $func" "Inferior call test" {
> +        -re $inf_call_stopped {
> +            pass "stopped within an inferior call"
> +        }
> +        -re ".. = 0\r\n$gdb_prompt " {
> +            verbose "stopped within an inferior call"
> +            return -1
> +        }
> +    }
> +
> +}

Procedure redefined?

> +
> +
> +# Perform an inferior call defined in func.
> +#

Copy/pasted comment doesn't make sense for this procedure.

> +proc check_bound_violation { parm parm_type is_positive} {

Inconsistent whitespace around params.

> +
> +    global u_fault
> +
> +    gdb_test "continue" "$u_fault.*" "Check bnd violation"

Lower case test messages.

> +
> +    if {$is_positive == 1} {

Drop the == 1.

> +        gdb_test "p (((void *)\$_siginfo._sifields._sigfault.si_addr\
> +                  - (void*)$parm))/sizeof($parm_type) == 1"\
> +                  " = 1" "Accessing only one position"
> +    } else {
> +        gdb_test "p ((void*)$parm\
> +                  - (void *)\$_siginfo._sifields._sigfault.si_addr)\
> +                  /sizeof($parm_type) == 1"\
> +                  " = 1" "Accessing only one position"
> +    }

Lowercase.  "accessing" -> "access".

> +    gdb_test "return" "\\\#.*main.*i386-mpx-call\\\.c:.*" "return from the fault"
> +}
> +
> +
> +# Start testing!
> +#
> +
> +# Set up just stop in middle of main for call function in the inferior.

# Set up for stopping in the middle of main for calling a function in the inferior.


> +#
> +with_test_prefix "default_run" {
> +
> +    gdb_test "p \$keep_bnd0_value=\$bnd0" $bound_reg\
> +        "Store bnd0 register in a convenience variable"

Lowercase.

> +
> +    gdb_test "p /x upper (a, b, c, d, 0)" " = $hex"\
> +        "Inferior call test"

Lowercase.  But "test" is redundant, these are all tests.
But better say "call inferior function" instead.

> +
> +    gdb_test "p ((\$bnd0.lbound==\$keep_bnd0_value.lbound) &&\
> +        (\$bnd0.ubound==\$keep_bnd0_value.ubound))" "= 1" "Keep BND register\
> +        value calls"

Lowercase.  Breaking the line before the last string starts reads
a bit better:

    gdb_test "p ((\$bnd0.lbound==\$keep_bnd0_value.lbound) &&\
        (\$bnd0.ubound==\$keep_bnd0_value.ubound))" "= 1" \
        "keep BND register value calls"

> +}
> +
> +# Consistency:  Examine bnd registers values before and after the call.
> +#
> +#
> +with_test_prefix "verify_default_values" {
> +
> +    prepare_bndcfg_bndstatus
> +
> +    gdb_breakpoint "*upper"
> +    perform_a_call "upper (a, b, c, d, 1)"
> +
> +    sanity_check_bndregs {"\$bnd0raw" "\$bnd1raw" "\$bnd2raw" "\$bnd3raw"}
> +
> +    compare_bndstatus_with_convenience
> +
> +    gdb_test_multiple "continue" "Inferior call test" {
> +        -re ".*Continuing.\r\n$gdb_prompt " {
> +            pass "normal continue"

Inconsistent: "Inferior call test" vs "normal continue".

> +        }
> +    }


> +}
> +
> +# Examine:  Cause a upper bound violation changing BND0.

"an upper"

Thanks,
Pedro Alves