From 0fec99e8be72b091618862eafc14e2741f0ff0d5 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 1 Oct 2014 23:31:55 +0100 Subject: [PATCH] Really fail inserting software breakpoints on read-only regions Currently, with "set breakpoint auto-hw off", we'll still try to insert a software breakpoint at addresses covered by supposedly read-only or inacessible regions: (top-gdb) mem 0x443000 0x450000 ro (top-gdb) set mem inaccessible-by-default off (top-gdb) disassemble Dump of assembler code for function main: 0x0000000000443956 <+34>: movq $0x0,0x10(%rax) => 0x000000000044395e <+42>: movq $0x0,0x18(%rax) 0x0000000000443966 <+50>: mov -0x24(%rbp),%eax 0x0000000000443969 <+53>: mov %eax,-0x20(%rbp) End of assembler dump. (top-gdb) b *0x0000000000443969 Breakpoint 5 at 0x443969: file ../../src/gdb/gdb.c, line 29. (top-gdb) c Continuing. warning: cannot set software breakpoint at readonly address 0x443969 Breakpoint 5, 0x0000000000443969 in main (argc=1, argv=0x7fffffffd918) at ../../src/gdb/gdb.c:29 29 args.argc = argc; (top-gdb) We warn, saying that the insertion can't be done, but then proceed attempting the insertion anyway, and in case of manually added regions, the insert actually succeeds. This is a regression; GDB used to fail inserting the breakpoint. More below. I stumbled on this as I wrote a test that manually sets up a read-only memory region with the "mem" command, in order to test GDB's behavior with breakpoints set on read-only regions, even when the real memory the breakpoints are set at isn't really read-only. I wanted that in order to add a test that exercises software single-stepping through read-only regions. Note that the memory regions that target_memory_map returns aren't like e.g., what would expect to see in /proc/PID/maps on Linux. Instead, they're the physical memory map from the _debuggers_ perspective. E.g., a read-only region would be real ROM or flash memory, while a read-only+execute mapping in /proc/PID/maps is still read-write to the debugger (otherwise the debugger wouldn't be able to set software breakpoints in the code segment). If one tries to manually write to memory that falls within a memory region that is known to be read-only, with e.g., "p foo = 1", then we hit a check in memory_xfer_partial_1 before the write mananges to make it to the target side. But writing a software/memory breakpoint nowadays goes through target_write_raw_memory, and unlike when writing memory with TARGET_OBJECT_MEMORY, nothing on the TARGET_OBJECT_RAW_MEMORY path checks whether we're trying to write to a read-only region. At the time "breakpoint auto-hw" was added, we didn't have the TARGET_OBJECT_MEMORY vs TARGET_OBJECT_RAW_MEMORY target object distinction yet, and the code path in memory_xfer_partial that blocks writes to read-only memory was hit for memory breakpoints too. With GDB 6.8 we had: warning: cannot set software breakpoint at readonly address 0000000000443943 Warning: Cannot insert breakpoint 1. Error accessing memory address 0x443943: Input/output error. So I started out by fixing this by adding the memory region validation to TARGET_OBJECT_RAW_MEMORY too. But later, when testing against GDBserver, I realized that that would only block software/memory breakpoints GDB itself inserts with gdb/mem-break.c. If a target has a to_insert_breakpoint method, the insertion request will still pass through to the target. So I ended up converting the "cannot set breakpoint" warning in breakpoint.c to a real error return, thus blocking the insertion sooner. With that, we'll end up no longer needing the TARGET_OBJECT_RAW_MEMORY changes once software single-step breakpoints are converted to real breakpoints. We need them today as software single-step breakpoints bypass insert_bp_location. But, it'll be best to leave that in as safeguard anyway, for other direct uses of TARGET_OBJECT_RAW_MEMORY. Tested on x86_64 Fedora 20, native and gdbserver. gdb/ 2014-10-01 Pedro Alves * breakpoint.c (insert_bp_location): Error out if inserting a software breakpoint at a read-only address. * target.c (memory_xfer_check_region): New function, factored out from ... (memory_xfer_partial_1): ... this. Make the 'reg_len' local a ULONGEST. (target_xfer_partial) : Check the access against the memory region attributes. gdb/testsuite/ 2014-10-01 Pedro Alves * gdb.base/breakpoint-in-ro-region.c: New file. * gdb.base/breakpoint-in-ro-region.exp: New file. --- gdb/ChangeLog | 11 ++ gdb/breakpoint.c | 14 +- gdb/target.c | 95 ++++++++++---- gdb/testsuite/ChangeLog | 5 + gdb/testsuite/gdb.base/breakpoint-in-ro-region.c | 28 ++++ gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 142 +++++++++++++++++++++ 6 files changed, 263 insertions(+), 32 deletions(-) create mode 100644 gdb/testsuite/gdb.base/breakpoint-in-ro-region.c create mode 100644 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 8a268c5..e595386 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2014-10-01 Pedro Alves + + * breakpoint.c (insert_bp_location): Error out if inserting a + software breakpoint at a read-only address. + * target.c (memory_xfer_check_region): New function, factored out + from ... + (memory_xfer_partial_1): ... this. Make the 'reg_len' local a + ULONGEST. + (target_xfer_partial) : Check the access + against the memory region attributes. + 2014-10-01 Simon Marchi * NEWS: Announce new exit-code field in -list-thread-groups diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index bec7f68..44f6f14 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2674,10 +2674,16 @@ insert_bp_location (struct bp_location *bl, } } else if (bl->loc_type == bp_loc_software_breakpoint - && mr->attrib.mode != MEM_RW) - warning (_("cannot set software breakpoint " - "at readonly address %s"), - paddress (bl->gdbarch, bl->address)); + && mr->attrib.mode != MEM_RW) + { + fprintf_unfiltered (tmp_error_stream, + _("Cannot insert breakpoint %d.\n" + "Cannot set software breakpoint " + "at read-only address %s\n"), + bl->owner->number, + paddress (bl->gdbarch, bl->address)); + return 1; + } } } diff --git a/gdb/target.c b/gdb/target.c index c1b5182..4606d17 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -909,6 +909,59 @@ target_section_by_addr (struct target_ops *target, CORE_ADDR addr) return NULL; } + +/* Helper for the memory xfer routines. Checks the attributes of the + memory region of MEMADDR against the read or write being attempted. + If the access is permitted returns true, otherwise returns false. + REGION_P is an optional output parameter. If not-NULL, it is + filled with a pointer to the memory region of MEMADDR. REG_LEN + returns LEN trimmed to the end of the region. This is how much the + caller can continue requesting, if the access is permitted. A + single xfer request must not straddle memory region boundaries. */ + +static int +memory_xfer_check_region (gdb_byte *readbuf, const gdb_byte *writebuf, + ULONGEST memaddr, ULONGEST len, ULONGEST *reg_len, + struct mem_region **region_p) +{ + struct mem_region *region; + + region = lookup_mem_region (memaddr); + + if (region_p != NULL) + *region_p = region; + + switch (region->attrib.mode) + { + case MEM_RO: + if (writebuf != NULL) + return 0; + break; + + case MEM_WO: + if (readbuf != NULL) + return 0; + break; + + case MEM_FLASH: + /* We only support writing to flash during "load" for now. */ + if (writebuf != NULL) + error (_("Writing to flash memory forbidden in this context")); + break; + + case MEM_NONE: + return 0; + } + + /* region->hi == 0 means there's no upper bound. */ + if (memaddr + len < region->hi || region->hi == 0) + *reg_len = len; + else + *reg_len = region->hi - memaddr; + + return 1; +} + /* Read memory from more than one valid target. A core file, for instance, could have some of memory but delegate other bits to the target below it. So, we must manually try all targets. */ @@ -970,7 +1023,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, ULONGEST len, ULONGEST *xfered_len) { enum target_xfer_status res; - int reg_len; + ULONGEST reg_len; struct mem_region *region; struct inferior *inf; @@ -1017,34 +1070,10 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, } /* Try GDB's internal data cache. */ - region = lookup_mem_region (memaddr); - /* region->hi == 0 means there's no upper bound. */ - if (memaddr + len < region->hi || region->hi == 0) - reg_len = len; - else - reg_len = region->hi - memaddr; - - switch (region->attrib.mode) - { - case MEM_RO: - if (writebuf != NULL) - return TARGET_XFER_E_IO; - break; - case MEM_WO: - if (readbuf != NULL) - return TARGET_XFER_E_IO; - break; - - case MEM_FLASH: - /* We only support writing to flash during "load" for now. */ - if (writebuf != NULL) - error (_("Writing to flash memory forbidden in this context")); - break; - - case MEM_NONE: - return TARGET_XFER_E_IO; - } + if (!memory_xfer_check_region (readbuf, writebuf, memaddr, len, ®_len, + ®ion)) + return TARGET_XFER_E_IO; if (!ptid_equal (inferior_ptid, null_ptid)) inf = find_inferior_pid (ptid_get_pid (inferior_ptid)); @@ -1184,6 +1213,16 @@ target_xfer_partial (struct target_ops *ops, writebuf, offset, len, xfered_len); else if (object == TARGET_OBJECT_RAW_MEMORY) { + /* Skip/avoid accessing the target if the memory region + attributes block the access. Check this here instead of in + raw_memory_xfer_partial as otherwise we'd end up checking + this twice in the case of the memory_xfer_partial path is + taken; once before checking the dcache, and another in the + tail call to raw_memory_xfer_partial. */ + if (!memory_xfer_check_region (readbuf, writebuf, offset, len, &len, + NULL)) + return TARGET_XFER_E_IO; + /* Request the normal memory object from other layers. */ retval = raw_memory_xfer_partial (ops, readbuf, writebuf, offset, len, xfered_len); diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index cd17e5d..4580eb2 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2014-10-01 Pedro Alves + + * gdb.base/breakpoint-in-ro-region.c: New file. + * gdb.base/breakpoint-in-ro-region.exp: New file. + 2014-10-01 Simon Marchi * gdb.mi/mi-exit-code.exp: New file. diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c new file mode 100644 index 0000000..696a67d --- /dev/null +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c @@ -0,0 +1,28 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2014 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 . */ + +volatile int i; + +int +main (void) +{ + i = 0; + i = 0; + i = 0; + + return 0; +} diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp new file mode 100644 index 0000000..8eacefa --- /dev/null +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp @@ -0,0 +1,142 @@ +# Copyright 2014 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 . + +# This file is part of the gdb testsuite + +standard_testfile + +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { + return -1 +} + +if ![runto main] { + return -1 +} + +delete_breakpoints + +# Get the bounds of a function, and write them to FUNC_LO (inclusive), +# FUNC_HI (exclusive). Return true on success and false on failure. +proc get_function_bounds {function func_lo func_hi} { + global gdb_prompt + global hex decimal + + upvar $func_lo lo + upvar $func_hi hi + + set lo "" + set size "" + + set test "get lo address of $function" + gdb_test_multiple "disassemble $function" $test { + -re "($hex) .*$hex <\\+($decimal)>:\[^\r\n\]+\r\nEnd of assembler dump\.\r\n$gdb_prompt $" { + set lo $expect_out(1,string) + set size $expect_out(2,string) + pass $test + } + } + + if { $lo == "" || $size == "" } { + return false + } + + # Account for the size of the last instruction. + set test "get hi address of $function" + gdb_test_multiple "x/2i $function+$size" $test { + -re ".*$hex <$function\\+$size>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" { + set hi $expect_out(1,string) + pass $test + } + } + + if { $hi == "" } { + return false + } + + # Remove unnecessary leading 0's (0x00000ADDR => 0xADDR) so we can + # easily do matches. Disassemble includes leading zeros, while + # x/i doesn't. + regsub -all "0x0\+" $lo "0x" lo + regsub -all "0x0\+" $hi "0x" hi + + return true +} + +# Get the address where the thread is currently stopped. +proc get_curr_insn {} { + global gdb_prompt + global hex + + set pc "" + set test "get current insn" + gdb_test_multiple "p /x \$pc" $test { + -re " = ($hex)\r\n$gdb_prompt $" { + set pc $expect_out(1,string) + pass $test + } + } + + return $pc +} + +# Get the address of where a single-step should land. +proc get_next_insn {} { + global gdb_prompt + global hex + + set next "" + set test "get next insn" + gdb_test_multiple "x/2i \$pc" $test { + -re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" { + set next $expect_out(1,string) + pass $test + } + } + + return $next +} + + +if ![get_function_bounds "main" main_lo main_hi] { + # Can't do the following tests if main's bounds are unknown. + return -1 +} + +# Manually create a read-only memory region that covers 'main'. +gdb_test_no_output "mem $main_lo $main_hi ro" \ + "create read-only mem region covering main" + +# So that we don't fail inserting breakpoints on addresses outside +# main, like the internal event breakpoints. +gdb_test_no_output "set mem inaccessible-by-default off" + +# So we get an immediate warning/error without needing to resume the +# target. +gdb_test_no_output "set breakpoint always-inserted on" + +# Disable the automatic fallback to HW breakpoints. We want a +# software breakpoint to be attempted, and to fail. +gdb_test_no_output "set breakpoint auto-hw off" + +# Confirm manual writes to the read-only memory region fail. +gdb_test "p /x *(char *) $main_lo = 1" \ + "Cannot access memory at address $main_lo" \ + "writing to read-only memory fails" + +# Ensure that inserting a software breakpoint in a known-read-only +# region fails. +gdb_test "break *$main_lo" \ + "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \ + "inserting software breakpoint in read-only memory fails" -- 2.7.4