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

[Andrew Burgess <aburgess@redhat.com>]

simon.marchi@polymtl.ca writes:

> 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.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew