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

Wei-min Pan
2018-07-12T02:01:51Z

On 7/3/2018 00:07 AM, 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, with a non-zero value, in aarch64_linux_set_debug_regs()
> and is similar to what x86 does - x86_linux_update_debug_registers()
> checks whether or not 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-28 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-28 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"
> + }
> +}