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

Weimin Pan
2019-02-06T00:51:44Z

Would you please review this aarch64-specific patch?Â  Thanks.

On 7/11/2018 5:30 PM, Wei-min Pan wrote:
>
> On 6/27/2018 6:10 PM, Weimin Pan wrote:
>> At issue here 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, and DR_CONTROL_LENGTH
>> (dreg_state.dbg_regs[i].ctrl)
>> got changed to a non-zero default value, when the PTRACE_SETREGSET
>> request
>> was made in aarch64_linux_set_debug_regs(), in preparation for resuming
>> the thread.
>>
>> I sent out a patch upstream for review last year. Yao Qi commented that
>> he needed to take a look at ptrace implementation to better understand
>> why this was happening. Unfortunately he didn't get a chance to do so.
>>
>> This is a revised patch which calls ptrace only if any debug register
>> has changed, either address or control containing a non-zero value,
>> in aarch64_linux_set_debug_regs() and is similar to what x86 does -
>> x86_linux_update_debug_registers() checks if a debug register has
>> non-zero reference count before setting it.
>>
>> Tested on aarch64-linux-gnu. No regressions.
>> ---
>> Â  gdb/ChangeLogÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  |Â Â  6 +
>> Â  gdb/nat/aarch64-linux-hw-point.cÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  |Â Â  7 +
>> Â  gdb/testsuite/ChangeLogÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  |Â  20 ++-
>> Â  gdb/testsuite/gdb.arch/aarch64-dbreg-contents.cÂ Â  | 179
>> ++++++++++++++++++++++
>> Â  gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp |Â  40 +++++
>> Â  5 files changed, 244 insertions(+), 8 deletions(-)
>> Â  create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>> Â  create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 7b108bf..d5bf15f 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2018-06-27Â  Weimin PanÂ  <weimin.pan@oracle.com>
>> +
>> +Â Â Â  PR gdb/21870
>> +Â Â Â  * nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs):
>> +Â Â Â  Validate debug register contents before calling ptrace.
>> +
>> Â  2018-06-18Â  Tom TromeyÂ  <tom@tromey.com>
>> Â  Â Â Â Â Â  * solib-aix.c (solib_aix_get_section_offsets): Return
>> diff --git a/gdb/nat/aarch64-linux-hw-point.c
>> b/gdb/nat/aarch64-linux-hw-point.c
>> index a3931ea..41ac304 100644
>> --- a/gdb/nat/aarch64-linux-hw-point.c
>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>> @@ -694,11 +694,18 @@ aarch64_linux_set_debug_regs (struct
>> aarch64_debug_reg_state *state,
>> Â Â Â  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
>> Â Â Â Â Â Â Â Â Â Â  + count * sizeof (regs.dbg_regs[0]));
>> Â  +Â  int changed_regs = 0;
>> Â Â Â  for (i = 0; i < count; i++)
>> Â Â Â Â Â  {
>> Â Â Â Â Â Â Â  regs.dbg_regs[i].addr = addr[i];
>> Â Â Â Â Â Â Â  regs.dbg_regs[i].ctrl = ctrl[i];
>> +Â Â Â Â Â  /* Check to see if any of these debug registers contains valid
>> data,
>> +Â Â Â Â Â Â Â Â  e.g. non-zero, before calling ptrace.Â  See PR gdb/21870.Â  */
>> +Â Â Â Â Â  if (ctrl[i] || addr[i])
>> +Â Â Â Â Â Â Â  changed_regs++;
>> Â Â Â Â Â  }
>> +Â  if (changed_regs == 0)
>> +Â Â Â  return;
>> Â  Â Â Â  if (ptrace (PTRACE_SETREGSET, tid,
>> Â Â Â Â Â Â Â Â Â Â Â  watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index a812061..0ce2d34 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,4 +1,9 @@
>> +2018-06-27Â  Weimin PanÂ  <weimin.pan@oracle.com>
>> +
>> +Â Â Â  PR gdb/21870
>> +Â Â Â  * gdb.arch/aarch64-dbreg-contents.c: New file.
>> +Â Â Â  * gdb.arch/aarch64-dbreg-contents.exp: New file.
>> +
>> Â  2018-06-18Â  Tom de VriesÂ  <tdevries@suse.de>
>> Â  Â Â Â Â Â  * gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations.
>> Â  diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>> b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>> new file mode 100644
>> index 0000000..85d4a03
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
>> @@ -0,0 +1,179 @@
>> +/* Test case for setting a memory-write unaligned watchpoint on
>> aarch64.
>> +
>> +Â  This software is provided 'as-is', without any express or implied
>> +Â  warranty.Â  In no event will the authors be held liable for any
>> damages
>> +Â  arising from the use of this software.
>> +
>> +Â  Permission is granted to anyone to use this software for any purpose,
>> +Â  including commercial applications, and to alter it and
>> redistribute it
>> +Â  freely.Â  */
>> +
>> +#define _GNU_SOURCE 1
>> +#ifdef __ia64__
>> +#define ia64_fpreg ia64_fpreg_DISABLE
>> +#define pt_all_user_regs pt_all_user_regs_DISABLE
>> +#endifÂ  /* __ia64__ */
>> +#include <sys/ptrace.h>
>> +#ifdef __ia64__
>> +#undef ia64_fpreg
>> +#undef pt_all_user_regs
>> +#endifÂ  /* __ia64__ */
>> +#include <linux/ptrace.h>
>> +#include <sys/types.h>
>> +#include <sys/user.h>
>> +#if defined __i386__ || defined __x86_64__
>> +#include <sys/debugreg.h>
>> +#endif
>> +
>> +#include <assert.h>
>> +#include <unistd.h>
>> +#include <sys/wait.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stddef.h>
>> +#include <errno.h>
>> +#include <sys/uio.h>
>> +#include <elf.h>
>> +#include <error.h>
>> +
>> +static __attribute__((unused)) pid_t child;
>> +
>> +static __attribute__((unused)) void
>> +cleanup (void)
>> +{
>> +Â  if (child > 0)
>> +Â Â Â  kill (child, SIGKILL);
>> +Â  child = 0;
>> +}
>> +
>> +static __attribute__((unused)) void
>> +handler_fail (int signo)
>> +{
>> +Â  cleanup ();
>> +Â  signal (signo, SIG_DFL);
>> +Â  raise (signo);
>> +}
>> +
>> +#ifdef __aarch64__
>> +
>> +#define SET_WATCHPOINT set_watchpoint
>> +
>> +/* Macros to extract fields from the hardware debug information
>> word.Â  */
>> +#define AARCH64_DEBUG_NUM_SLOTS(x) ((x) & 0xff)
>> +#define AARCH64_DEBUG_ARCH(x) (((x) >> 8) & 0xff)
>> +/* Macro for the expected version of the ARMv8-A debug
>> architecture.Â  */
>> +#define AARCH64_DEBUG_ARCH_V8 0x6
>> +#define DR_CONTROL_ENABLED(ctrl)Â Â Â Â Â Â Â  (((ctrl) & 0x1) == 1)
>> +#define DR_CONTROL_LENGTH(ctrl)Â Â Â Â Â Â Â Â  (((ctrl) >> 5) & 0xff)
>> +
>> +static void
>> +set_watchpoint (pid_t pid, volatile void *addr, unsigned len_mask)
>> +{
>> +Â  struct user_hwdebug_state dreg_state;
>> +Â  struct iovec iov;
>> +Â  long l;
>> +
>> +Â  assert (len_mask >= 0x01);
>> +Â  assert (len_mask <= 0xff);
>> +
>> +Â  iov.iov_base = &dreg_state;
>> +Â  iov.iov_len = sizeof (dreg_state);
>> +Â  errno = 0;
>> +Â  l = ptrace (PTRACE_GETREGSET, pid, NT_ARM_HW_WATCH, &iov);
>> +Â  assert (l == 0);
>> +Â  assert (AARCH64_DEBUG_ARCH (dreg_state.dbg_info) ==
>> AARCH64_DEBUG_ARCH_V8);
>> +Â  assert (AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info) >= 1);
>> +
>> +Â  assert (!DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl));
>> +Â  dreg_state.dbg_regs[0].ctrl |= 1;
>> +Â  assert ( DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl));
>> +
>> +Â  assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == 0);
>> +Â  dreg_state.dbg_regs[0].ctrl |= len_mask << 5;
>> +Â  assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == len_mask);
>> +
>> +Â  dreg_state.dbg_regs[0].ctrl |= 2 << 3; // write
>> +Â  dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0
>> +Â  //printf("ctrl=0x%x\n",dreg_state.dbg_regs[0].ctrl);
>> +Â  dreg_state.dbg_regs[0].addr = (uintptr_t) addr;
>> +
>> +Â  iov.iov_base = &dreg_state;
>> +Â  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  + sizeof (dreg_state.dbg_regs[0]));
>> +Â  errno = 0;
>> +Â  l = ptrace (PTRACE_SETREGSET, pid, NT_ARM_HW_WATCH, &iov);
>> +Â  if (errno != 0)
>> +Â Â Â  error (1, errno, "PTRACE_SETREGSET: NT_ARM_HW_WATCH");
>> +Â  assert (l == 0);
>> +}
>> +
>> +#endif
>> +
>> +#ifndef SET_WATCHPOINT
>> +
>> +int
>> +main (void)
>> +{
>> +Â  return 77;
>> +}
>> +#else
>> +
>> +static volatile long long check;
>> +
>> +int
>> +main (void)
>> +{
>> +Â  pid_t got_pid;
>> +Â  int i, status;
>> +Â  long l;
>> +
>> +Â  atexit (cleanup);
>> +Â  signal (SIGABRT, handler_fail);
>> +Â  signal (SIGINT, handler_fail);
>> +
>> +Â  child = fork ();
>> +Â  assert (child >= 0);
>> +Â  if (child == 0)
>> +Â Â Â  {
>> +Â Â Â Â Â  l = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
>> +Â Â Â Â Â  assert (l == 0);
>> +Â Â Â Â Â  i = raise (SIGUSR1);
>> +Â Â Â Â Â  assert (i == 0);
>> +Â Â Â Â Â  check = -1;
>> +Â Â Â Â Â  i = raise (SIGUSR2);
>> +Â Â Â Â Â  /* NOTREACHED */
>> +Â Â Â Â Â  assert (0);
>> +Â Â Â  }
>> +
>> +Â  got_pid = waitpid (child, &status, 0);
>> +Â  assert (got_pid == child);
>> +Â  assert (WIFSTOPPED (status));
>> +Â  assert (WSTOPSIG (status) == SIGUSR1);
>> +
>> +Â  // PASS:
>> +Â  //SET_WATCHPOINT (child, &check, 0xff);
>> +Â  // FAIL:
>> +Â  SET_WATCHPOINT (child, &check, 0x02);
>> +
>> +Â  errno = 0;
>> +Â  l = ptrace (PTRACE_CONT, child, 0l, 0l);
>> +Â  assert_perror (errno);
>> +Â  assert (l == 0);
>> +
>> +Â  got_pid = waitpid (child, &status, 0);
>> +Â  assert (got_pid == child);
>> +Â  assert (WIFSTOPPED (status));
>> +Â  if (WSTOPSIG (status) == SIGUSR2)
>> +Â Â Â  {
>> +Â Â Â Â Â  /* We missed the watchpoint - unsupported by hardware? */
>> +Â Â Â Â Â  cleanup ();
>> +Â Â Â Â Â  return 2;
>> +Â Â Â  }
>> +Â  assert (WSTOPSIG (status) == SIGTRAP);
>> +
>> +Â  cleanup ();
>> +Â  return 0;
>> +}
>> +
>> +#endif
>> +
>> diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>> b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>> new file mode 100644
>> index 0000000..6aa23b0
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
>> @@ -0,0 +1,40 @@
>> +# Copyright 2018 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Â  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.Â  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +# Make sure that the inferior doesn't assert and exits successfully.
>> +
>> +if {![is_aarch64_target]} {
>> +Â Â Â  verbose "Skipping ${gdb_test_file_name}."
>> +Â Â Â  return
>> +}
>> +
>> +standard_testfile .c
>> +
>> +if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}
>> executable {debug}] != "" } {
>> +Â Â Â Â  untested "failed to compile"
>> +Â Â Â Â  return -1
>> +}
>> +
>> +clean_restart $testfile
>> +
>> +set test "run to exit"
>> +gdb_test_multiple "run" "$test" {
>> +Â Â Â  -re "exited with code 01.*$gdb_prompt $" {
>> +Â Â Â Â Â Â Â  pass "$test"
>> +Â Â Â  }
>> +Â Â Â  -re "exited normally.*$gdb_prompt $" {
>> +Â Â Â Â Â Â Â  pass "$test"
>> +Â Â Â  }
>> +}
>