From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82301 invoked by alias); 12 Jul 2018 02:01:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 82244 invoked by uid 89); 12 Jul 2018 02:01:51 -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 autolearn=ham version=3.3.2 spammy=liable, granted, arising, preparation X-HELO: userp2120.oracle.com Received: from userp2120.oracle.com (HELO userp2120.oracle.com) (156.151.31.85) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Jul 2018 02:01:48 +0000 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w6C1x9ti021056 for ; Thu, 12 Jul 2018 02:01:47 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=4Bnfa7kavi0Mv19p38rVMLv8kPLwOanvXsha0GkiSaM=; b=ZhCHswxBYlrxZqXEMVQnGM14NLdeyIuZPLJzYY8CZtBLGMprcXDWSxqBsneT9zgI8Zcx l8th1pbikU075+q3LI73hobsX20TMz+I/JJO2PApLn71JSA23s2+/8CxUnujMm5Aks18 oSSFZ8zdn8ucGm+8BEHy/nxvAVjdF1NdFd8DpnCapufEAR8Odw8ww22lNIhMox1A6/LT p+3oynqAox/iOrFHEdjrEteXZ9+Fe1MTYK9uJ5E/n6HCCtd3yoX6vAlt2NACaGhFtM+h g1ZKPSVRP5wo698o91e6Inn8aFVEqsO2BpNfNEztp/HeRxRkL7UAX/2TxeEwJ3eGCyH8 dw== Received: from aserv0021.oracle.com (aserv0021.oracle.com [141.146.126.233]) by userp2120.oracle.com with ESMTP id 2k2p7vgdnp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 12 Jul 2018 02:01:46 +0000 Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w6C21jml002179 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 12 Jul 2018 02:01:46 GMT Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w6C21jGa008264 for ; Thu, 12 Jul 2018 02:01:45 GMT Received: from [10.159.226.117] (/10.159.226.117) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 11 Jul 2018 19:01:45 -0700 Subject: [PING] [PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers To: "gdb-patches@sourceware.org" References: <1530144022-12110-1-git-send-email-weimin.pan@oracle.com> From: Wei-min Pan Message-ID: <284369ed-9367-fd8f-d90b-d057a699513e@oracle.com> Date: Thu, 12 Jul 2018 02:01:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 In-Reply-To: <1530144022-12110-1-git-send-email-weimin.pan@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2018-07/txt/msg00328.txt.bz2 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 > + > + 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-28 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" > + } > +}