[PATCH] gdb: fix out-of-bounds read when printing a pointer with a format

[simon.marchi@polymtl.ca]

From: Simon Marchi <simon.marchi@polymtl.ca>

I stumbled on this while investigating bug 34638 ("Multi-register-reading
oddity / anachronism"), but this patch is not otherwise related to that
bug.

When the value being printed is a pointer, print_scalar_formatted
overrides the number of bytes it reads from the value buffer:

  if (type->code () == TYPE_CODE_PTR)
    len = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;

VALADDR only holds type->length() bytes though, and for a pointer type
built by GDB that is `gdbarch_ptr_bit / TARGET_CHAR_BIT` bytes.  On targets
where gdbarch_addr_bit is larger than gdbarch_ptr_bit, we therefore read
past the end of the buffer.

AVR is such a target: gdbarch_ptr_bit is 16 while gdbarch_addr_bit is 32,
so printing any pointer with a format reads 4 bytes out of a 2-byte buffer.
With a GDB built with AddressSanitizer:

    (gdb) p/x $sp
    =================================================================
    ==433709==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c11d1239e33 at pc 0x55eb8e8438f0 bp 0x7ffc17c69050 sp 0x7ffc17c69040

It can be reproduced without running a program, simply with a GDB that has
AVR support, by forcing the architecture to be AVR:

    (gdb) set architecture avr
    The target architecture is set to "avr".
    (gdb) p/x (void *) 0x1234
    =================================================================
    ==434356==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7bbca0e21293 at pc 0x55f5d74d08f0 bp 0x7ffe74cba960 sp 0x7ffe74cba950

"info registers SP" on an AVR target goes through the same path, so hits
the same crash:

    (gdb) c
    Continuing.

    Breakpoint 1, main () at test.c:16
    16        int local = 42;
    (gdb) info registers SP
    =================================================================
    ==432390==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7b764403b413 at pc 0x55e8ef8628f0 bp 0x7fff13994110 sp 0x7fff13994100

The override comes with this comment:

  /* If the value is a pointer, and pointers and addresses are not the
     same, then at this point, the value's length (in target bytes) is
     gdbarch_addr_bit/TARGET_CHAR_BIT, not type->length ().  */

This was true when the override was introduced, in commit ef166cf46a1b
(2001).  At the time, print_scalar_formatted started by converting the
value to a LONGEST:

  val_long = unpack_long (type, valaddr);

For pointers, unpack_long goes through extract_typed_address, then
gdbarch_pointer_to_address, which converts the pointer to a CORE_ADDR
gdbarch_addr_bit wide.  On AVR, for instance, data pointer 0x1234 (2 bytes
long) becomes address 0x801234 (4 bytes long).  "The value" in the comment
refers to this VAL_LONG, not to the bytes at VALADDR.  LEN was then only
used to mask VAL_LONG, and the override made sure that the mask did not
truncate the converted address to gdbarch_ptr_bit bits.  VALADDR (2 bytes)
was never read using LEN (4 bytes), so there was no out-of-bounds read.

Commit d9109c8080ac ("Simplify print_scalar_formatted", 2017) changed the
function to print the value using the print_*_chars functions, which
operate directly on the bytes at VALADDR, and removed the conversion to
LONGEST.  From then on, the value printed became the raw pointer
representation, type->length() bytes long (2 bytes), not the converted
address (4 bytes).  The length override was left in place, and this is
when the out-of-bounds read was introduced.

Note that commit d9109c8080ac therefore changed how `print (void *) 0x1234`
is printed for architectures like AVR, it went from

    $1 = 0x801234

to just

    $1 = 0x1234

I don't think it's a mistake in itself, but I am not sure it was fully
understood at the time.

Since the LEN override no longer has a reason to be, remove it.

With LEN now always equal to type->length(), the following block becomes
dead code, so remove it.  The comment above it is also a relic of the
VAL_LONG time.

I started by adding a very simple test to
gdb.base/all-architectures.exp.tcl to try to print a formatted pointer with
each arch.  I then thought it would be useful to test printing a pointer
without formatting, and then the various kinds of pointers (data vs code).
The result is the somewhat complicated procedure in this patch.  It did not
find additional bugs, but I think it's still useful to have in the
testsuite, as it captures what GDB does today.  If a change in the pointer
printing code ever changes (notably in the address <-> pointer conversion),
we will notice it.

Change-Id: I287eb515a019941b63325e3a258b1eb1b5225e4a
---
 gdb/printcmd.c                                | 16 ----
 .../gdb.base/all-architectures.exp.tcl        | 76 +++++++++++++++++++
 2 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 4fd369c90c06..09b22ae5010c 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -373,22 +373,6 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type,
   /* String printing should go through val_print_scalar_formatted.  */
   gdb_assert (options.format != 's');
 
-  /* If the value is a pointer, and pointers and addresses are not the
-     same, then at this point, the value's length (in target bytes) is
-     gdbarch_addr_bit/TARGET_CHAR_BIT, not type->length ().  */
-  if (type->code () == TYPE_CODE_PTR)
-    len = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
-
-  /* If we are printing it as unsigned, truncate it in case it is actually
-     a negative signed value (e.g. "print/u (short)-1" should print 65535
-     (if shorts are 16 bits) instead of 4294967295).  */
-  if (options.format != 'c'
-      && (options.format != 'd' || type->is_unsigned ()))
-    {
-      if (len < type->length () && byte_order == BFD_ENDIAN_BIG)
-	valaddr += type->length () - len;
-    }
-
   /* Allow LEN == 0, and in this case, don't assume that VALADDR is
      valid.  */
   const gdb_byte zero = 0;
diff --git a/gdb/testsuite/gdb.base/all-architectures.exp.tcl b/gdb/testsuite/gdb.base/all-architectures.exp.tcl
index 7d2979476286..79f13ba7efa1 100644
--- a/gdb/testsuite/gdb.base/all-architectures.exp.tcl
+++ b/gdb/testsuite/gdb.base/all-architectures.exp.tcl
@@ -130,10 +130,86 @@ proc print_floats {} {
     gdb_test_internal "print 1.0f" " = 1" "print, float"
 }
 
+# Exercise printing various kinds of data and code pointers, with and
+# without a format, on architecture ARCH.  This is not necessarily a
+# source of truth, the test just matches what GDB happened to output
+# for each arch at the time of writing.
+
+proc print_pointers {arch} {
+    # Without a format, the pointer is converted to an address using
+    # gdbarch_pointer_to_address, which is sometimes in a different
+    # representation.
+    set data_addr 0x1234
+    set code_addr 0x1234
+
+    if {[string match "avr*" $arch]} {
+	# Data pointers point to SRAM, which starts at 0x800000 in the
+	# imaginary unified address space.
+	#
+	# Code pointers are word-addressed and are converted to a byte address
+	# in flash.
+	set data_addr 0x801234
+	set code_addr 0x2468
+    } elseif {[string match "ft32*" $arch]} {
+	# Data pointers point to RAM, which starts at 0x800000.
+	#
+	# Pointers with the flash address class (see the @flash
+	# pointers below) point to flash, so start at 0.
+	#
+	# Function pointers and pointers to @code behave like data pointers,
+	# not sure if it's a bug or not (maybe they should behave like @flash
+	# pointers).
+	set data_addr 0x801234
+	set code_addr 0x801234
+    } elseif {$arch == "rl78"} {
+	# Data pointers point to the 64 KiB region starting at
+	# 0xf0000.
+	set data_addr 0xf1234
+    } elseif {$arch == "iq2000" || $arch == "iq10"} {
+	# Code pointers have the 0x80000000 bit set.
+	set code_addr 0x80001234
+    }
+
+    # Each element is: test prefix, type, type regexp as printed by
+    # GDB, expected address.
+    #
+    # The @code and @data qualifiers apply to the pointed-to type, and
+    # are understood by all architectures.
+    set pointers [list \
+		      [list "data pointer" "void *" "void \\*" $data_addr] \
+		      [list "code pointer" "void (*) (void)"  "void \\(\\*\\)\\(void\\)" $code_addr] \
+		      [list "@data pointer" "void @data *" "@data void \\*" $data_addr] \
+		      [list "@code pointer" "void @code *" "@code void \\*" $code_addr]]
+
+    # AVR and FT32 have a "flash" address class, which applies to the
+    # pointer type itself.
+    if {[string match "avr*" $arch] || [string match "ft32*" $arch]} {
+	lappend pointers \
+	    [list "@flash data pointer" "void * @flash" "void \\* @flash" 0x1234] \
+	    [list "@flash code pointer" "void (* @flash) (void)" "void \\(\\* @flash\\)\\(void\\)" 0x1234]
+    }
+
+    foreach ptr $pointers {
+	lassign $ptr kind type_name type_name_re addr
+
+	with_test_prefix $kind {
+	    gdb_test_internal "print ($type_name) 0x1234" \
+		" = \\($type_name_re\\) $addr" "print"
+
+	    # With a format, the pointer's bytes are printed as is.
+	    gdb_test_internal "print/x ($type_name) 0x1234" " = 0x1234" "print/x"
+	    gdb_test_internal "print/d ($type_name) 0x1234" " = 4660" "print/d"
+	    gdb_test_internal "print/o ($type_name) 0x1234" " = 011064" "print/o"
+	    gdb_test_internal "print/t ($type_name) 0x1234" " = 1001000110100" "print/t"
+	}
+    }
+}
+
 # Run tests on the current architecture ARCH.
 
 proc do_arch_tests {arch} {
     print_floats
+    print_pointers $arch
 
     # When we disassemble using the default architecture then we
     # expect that the only error we should get from the disassembler

base-commit: 2ce6c3190773eb22f467446c4fdc7edb123ce82f
-- 
2.55.0