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

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

[Wei-min Pan]

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]));

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

[Yao Qi]

On Sat, Oct 21, 2017 at 1:58 AM, Wei-min Pan <weimin.pan@oracle.com> wrote:

Hi Wei-min,
As I asked in comment 3 in PR 21870, could you
help me to understand the bug?  I don't see how
does GDB touches hw debug registers, in this case. I have no idea that
why DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) is changed to
non-zero.

-- 
Yao (齐尧)

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

[Wei-min Pan]

Hi Yao,

The most likely explanation is that ptrace() (1) does not validate contents
in either address or control register, and (2) uses some default control 
values
when setting hardware breakpoints. (2) is the main reason that contents in
control register becomes non-zero after the aarch64_linux_set_debug_regs()
call. BTW the value of 0x1fc in control register is not random but can be
decoded as:

"a watchpoint which is disabled, priv 2, 8-bytes, and of type hw_access"

Wei-min

On 10/22/2017 2:15 PM, Yao Qi wrote:
> On Sat, Oct 21, 2017 at 1:58 AM, Wei-min Pan <weimin.pan@oracle.com> wrote:
>
> Hi Wei-min,
> As I asked in comment 3 in PR 21870, could you
> help me to understand the bug?  I don't see how
> does GDB touches hw debug registers, in this case. I have no idea that
> why DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) is changed to
> non-zero.
>

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

[Yao Qi]

Wei-min Pan <weimin.pan@oracle.com> writes:

> The most likely explanation is that ptrace() (1) does not validate contents
> in either address or control register, and (2) uses some default
> control values
> when setting hardware breakpoints. (2) is the main reason that contents in
> control register becomes non-zero after the aarch64_linux_set_debug_regs()
> call. BTW the value of 0x1fc in control register is not random but can be
> decoded as:
>
> "a watchpoint which is disabled, priv 2, 8-bytes, and of type hw_access"

The test case we got from Jan (in PR 21870) is that parent forks a
child, and read hw debug registers.  The parent asserts on some bits of
debug registers read from child process.  They are zero when the program
runs standalone, but they aren't zero after it runs within GDB.  When it
runs in GDB, GDB is the grand-parent, in default, follow-fork-mode is
parent, and detach-on-fork is on, IOW, GDB (as a grandparent) only
attaches to the parent process, and leaves the child process run
freely.  Then, the parent process reads some "unexpected" value from
child process, why is it a bug in the grandparent process?

Secondly, why is it valid to expect 'length' is zero when the debug
register is disabled?

  assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == 0);

GDB thinks if debug register is disabled, then, it can be used.  Now,
the observation is that when a debug register is disabled, the 'length'
can be different values in case that the process has tracer grandparent
or not.  We may need to look into Linux kernel.

-- 
Yao (齐尧)

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

[Weimin Pan]

On 10/27/2017 2:05 AM, Yao Qi wrote:
> Wei-min Pan <weimin.pan@oracle.com> writes:
>
>> The most likely explanation is that ptrace() (1) does not validate contents
>> in either address or control register, and (2) uses some default
>> control values
>> when setting hardware breakpoints. (2) is the main reason that contents in
>> control register becomes non-zero after the aarch64_linux_set_debug_regs()
>> call. BTW the value of 0x1fc in control register is not random but can be
>> decoded as:
>>
>> "a watchpoint which is disabled, priv 2, 8-bytes, and of type hw_access"
> The test case we got from Jan (in PR 21870) is that parent forks a
> child, and read hw debug registers.  The parent asserts on some bits of
> debug registers read from child process.  They are zero when the program
> runs standalone, but they aren't zero after it runs within GDB.  When it
> runs in GDB, GDB is the grand-parent, in default, follow-fork-mode is
> parent, and detach-on-fork is on, IOW, GDB (as a grandparent) only
> attaches to the parent process, and leaves the child process run
> freely.  Then, the parent process reads some "unexpected" value from
> child process, why is it a bug in the grandparent process?

I believe that is because the grandparent process did modify the 
hardware registers in grandchild
process, which was originated from aarch64_linux_new_thread() that set 
new thread's registers
with DR_MARK_ALL_CHANGED().  It in turn caused 
aarch64_linux_set_debug_regs() to be called,
regardless of whether or not these registers had really been changed, 
when the grandchild process
was ready to resume execution.

> Secondly, why is it valid to expect 'length' is zero when the debug
> register is disabled?
>
>    assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == 0);
>
> GDB thinks if debug register is disabled, then, it can be used.  Now,
> the observation is that when a debug register is disabled, the 'length'
> can be different values in case that the process has tracer grandparent
> or not.  We may need to look into Linux kernel.
>

OK, but given that the ptrace() call made in 
aarch64_linux_set_debug_regs() to set debug registers
didn't flag any error, it suggested that kernel didn't do any 
verifications on input parameters?

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

[Wei-min Pan]

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]));
>