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

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

Hi Pedro,

Thanks a lot for your review!


Am 2/24/2017 um 2:40 PM schrieb 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:
> ~~~
Thanks i will take your version.
> 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.
Sorry, got to excited with the testing itself.
>> +
>> +
>> +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
Yes, sorry.
>> +}
>> +
>> +
>> +# 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?
I think so copy and paste error.
>
>> +
>> +
>> +# 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

Will address all comments.

Thanks again,
/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