Re: [PING 2][PATCH PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan <weimin.pan@oracle.com>]

On 10/20/2017 5:58 PM, Wei-min Pan wrote:
> On 10/6/2017 3:18 PM, Weimin Pan wrote:
>>      The root cause is that ptrace() does not validate either
>>      address or size when setting a hardware watchpoint/breakpoint.
>>      As a result, watchpoints were set at address 0, the initial 
>> value of
>>      aarch64_debug_reg_state in aarch64_process_info, when the
>>      PTRACE_SETREGSET request was first made in
>>      aarch64_linux_set_debug_regs(), in preparation for resuming the 
>> thread.
>>
>>      Other than changing the kernel ptrace() implementation, first 
>> attempt to
>>      fix this problem in gdb was to focus on aarch64_linux_new_thread().
>>      Instead of marking all hardware breakpoint/watchpoint register 
>> pairs for
>>      the new thread that have changed, tried to reflect the state by 
>> using
>>      either DR_MARK_ALL_CHANGED() if they have really been changed or
>>      DR_CLEAR_CHANGED() otherwise. But finding whether or not the 
>> registers
>>      have been changed by using parent's lwp_info or 
>> aarch64_process_info
>>      proved to be hard or incorrect, especially the latter which caused
>>      gdbserver to crash in the middle of the ptid_of_lwp() call.
>>
>>      Another approach was then taken - add function 
>> initial_control_length()
>>      to validate the contents in the control registers, basing on the 
>> fact that
>>      the kernel only supports Byte Address Select (BAS) values of 
>> 0x1, 0x3, 0xf
>>      and 0xff, before calling ptrace() in 
>> aarch64_linux_set_debug_regs().
>>
>>      Tested on aarch64-linux-gnu. No regressions.
>> ---
>>   gdb/ChangeLog                    |  7 +++++++
>>   gdb/nat/aarch64-linux-hw-point.c | 18 +++++++++++++++++-
>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index c4f55a8137..543e1a0487 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2017-10-06  Weimin Pan  <weimin.pan@oracle.com>
>> +
>> +    PR gdb/21870
>> +    * nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs):
>> +    Call new function to validate the length in control registers.
>> +    (initial_control_length): New function.
>> +
>>   2017-09-15  Pedro Alves  <palves@redhat.com>
>>         * compile/compile-c-types.c (convert_enum, convert_int)
>> diff --git a/gdb/nat/aarch64-linux-hw-point.c 
>> b/gdb/nat/aarch64-linux-hw-point.c
>> index 9800d9a59c..22c0a48c14 100644
>> --- a/gdb/nat/aarch64-linux-hw-point.c
>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>> @@ -548,6 +548,22 @@ aarch64_handle_watchpoint (enum 
>> target_hw_bp_type type, CORE_ADDR addr,
>>                           state);
>>   }
>>   +/* Validate the lengths of breakpoints/watchpoints, according to the
>> +   contents of these hardware debug control registers, and return
>> +   true if all these registers contain zero length.  */
>> +
>> +static bool
>> +initial_control_length (const unsigned int *ctrl, int count)
>> +{
>> +  for (int i = 0; i < count; i++)
>> +    {
>> +      if (DR_CONTROL_LENGTH (ctrl[i]))
>> +        return false;
>> +    }
>> +
>> +  return true;
>> +}
>> +
>>   /* Call ptrace to set the thread TID's hardware breakpoint/watchpoint
>>      registers with data from *STATE.  */
>>   @@ -566,7 +582,7 @@ aarch64_linux_set_debug_regs (const struct 
>> aarch64_debug_reg_state *state,
>>     count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;
>>     addr = watchpoint ? state->dr_addr_wp : state->dr_addr_bp;
>>     ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
>> -  if (count == 0)
>> +  if (count == 0 || initial_control_length (ctrl, count))
>>       return;
>>     iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
>>            + count * sizeof (regs.dbg_regs[0]));
>