From patchwork Tue Jul 3 00:07:24 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Weimin Pan X-Patchwork-Id: 28206 Received: (qmail 47972 invoked by alias); 3 Jul 2018 00:07:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 47051 invoked by uid 89); 3 Jul 2018 00:07:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, UNPARSEABLE_RELAY autolearn=ham version=3.3.2 spammy=Pan, pan, arising, yao X-HELO: aserp2120.oracle.com Received: from aserp2120.oracle.com (HELO aserp2120.oracle.com) (141.146.126.78) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Jul 2018 00:07:31 +0000 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w62Nwufu059656 for ; Tue, 3 Jul 2018 00:07:29 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : subject : date : message-id; s=corp-2017-10-26; bh=zCoAG8Zo1k1giwWWJUX+RvwU1wNJHwbcZksxzeUsKD4=; b=D+LTGnnEsC16G5phOSWjexoFDe9kM4cIOaU0I7C8SMF83eWTnbln9uW8h1NYypEQJYPV /CLGtaH9Q9e+lm2omVdgV20kPYV4bFJchBCgM/JEdmrvxUk2jQGXbhqJP0YHeeNBG99L D5zThgKWD29S0FsFcL6sE9YO60YPW1eKXVHNW/EJD+9DVI/mq7CbxS60GdgxcCeSZJSs uWYcornwTMdCWJCz7OUtCWI8U4JH83KiueJW1CjQ50zBUkPeTEtYwxwHIPaDgHc4Kqe2 CfATUFuclrAh5PMw8BsA0CKsvs8tyt8oSSFNl1uqbjI4uxAWPbCVj/ROF2WOB9NifeiV lA== Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by aserp2120.oracle.com with ESMTP id 2jx1tnxcqk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 03 Jul 2018 00:07:29 +0000 Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w6307S8P024932 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 3 Jul 2018 00:07:28 GMT Received: from abhmp0008.oracle.com (abhmp0008.oracle.com [141.146.116.14]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w6307S9B026958 for ; Tue, 3 Jul 2018 00:07:28 GMT Received: from localhost.us.oracle.com (/10.211.15.129) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 02 Jul 2018 17:07:28 -0700 From: Weimin Pan To: gdb-patches@sourceware.org Subject: [PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers Date: Tue, 3 Jul 2018 00:07:24 +0000 Message-Id: <1530576444-24656-1-git-send-email-weimin.pan@oracle.com> 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 + + 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 * 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 + + 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 * 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 +#ifdef __ia64__ +#undef ia64_fpreg +#undef pt_all_user_regs +#endif /* __ia64__ */ +#include +#include +#include +#if defined __i386__ || defined __x86_64__ +#include +#endif + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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 . +# +# 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" + } +}