diff mbox series

[for-2.12] gdbstub: fix off-by-one in gdb_handle_packet()

Message ID 20180408145933.1149-1-f4bug@amsat.org
State New
Headers show
Series [for-2.12] gdbstub: fix off-by-one in gdb_handle_packet() | expand

Commit Message

Philippe Mathieu-Daudé April 8, 2018, 2:59 p.m. UTC 
memtohex() adds an extra trailing NUL character.

Reported-by: AddressSanitizer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
(gdb) dump binary memory /tmp/dram.bin 0x94000000 0x94100000
Remote connection closed

Comments

Stefan Hajnoczi April 9, 2018, 5:58 a.m. UTC | #1 
On Sun, Apr 08, 2018 at 11:59:33AM -0300, Philippe Mathieu-Daudé wrote:
> memtohex() adds an extra trailing NUL character.
> 
> Reported-by: AddressSanitizer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> (gdb) dump binary memory /tmp/dram.bin 0x94000000 0x94100000
> Remote connection closed
> 
> =================================================================
> ==22732==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe43018340 at pc 0x55f2655fde81 bp 0x7ffe43017210 sp 0x7ffe43017208
> WRITE of size 1 at 0x7ffe43018340 thread T0
>     #0 0x55f2655fde80 in memtohex /source/qemu/gdbstub.c:520
>     #1 0x55f26560254d in gdb_handle_packet /source/qemu/gdbstub.c:1140
>     #2 0x55f2656073c3 in gdb_read_byte /source/qemu/gdbstub.c:1703
>     #3 0x55f2656076a7 in gdb_chr_receive /source/qemu/gdbstub.c:1909
>     #4 0x55f266457656 in qemu_chr_be_write_impl /source/qemu/chardev/char.c:175
>     #5 0x55f2664576f9 in qemu_chr_be_write /source/qemu/chardev/char.c:187
>     #6 0x55f26646f6f0 in tcp_chr_read /source/qemu/chardev/char-socket.c:470
>     #7 0x55f2664bc9e3 in qio_channel_fd_source_dispatch /source/qemu/io/channel-watch.c:84
>     #8 0x7f17d01b30f4 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>     #9 0x55f2665c7f10 in glib_pollfds_poll /source/qemu/util/main-loop.c:215
>     #10 0x55f2665c8100 in os_host_main_loop_wait /source/qemu/util/main-loop.c:263
>     #11 0x55f2665c82d6 in main_loop_wait /source/qemu/util/main-loop.c:522
>     #12 0x55f26599e13b in main_loop /source/qemu/vl.c:1943
>     #13 0x55f2659b0869 in main /source/qemu/vl.c:4734
> 
> Address 0x7ffe43018340 is located in stack of thread T0 at offset 4192 in frame
>     #0 0x55f265601266 in gdb_handle_packet /source/qemu/gdbstub.c:996
> 
>   This frame has 3 object(s):
>     [32, 40) 'p'
>     [96, 4192) 'buf' <== Memory access at offset 4192 overflows this variable
>     [4224, 8320) 'mem_buf'
> SUMMARY: AddressSanitizer: stack-buffer-overflow /source/qemu/gdbstub.c:520 in memtohex
> Shadow bytes around the buggy address:
>   0x1000485fb010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x1000485fb060: 00 00 00 00 00 00 00 00[f2]f2 f2 f2 00 00 00 00
>   0x1000485fb070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb0a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x1000485fb0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
> ==22732==ABORTING
> ---
>  gdbstub.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Paolo Bonzini April 9, 2018, 9:39 a.m. UTC | #2 
On 09/04/2018 07:58, Stefan Hajnoczi wrote:
> On Sun, Apr 08, 2018 at 11:59:33AM -0300, Philippe Mathieu-Daudé wrote:
>> memtohex() adds an extra trailing NUL character.
>>
>> Reported-by: AddressSanitizer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> (gdb) dump binary memory /tmp/dram.bin 0x94000000 0x94100000
>> Remote connection closed
>>
>> =================================================================
>> ==22732==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe43018340 at pc 0x55f2655fde81 bp 0x7ffe43017210 sp 0x7ffe43017208
>> WRITE of size 1 at 0x7ffe43018340 thread T0
>>
>> ---
>>  gdbstub.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Peter, can you apply this directly to master?

Thanks,

Paolo
Peter Maydell April 9, 2018, 3:25 p.m. UTC | #3 
On 9 April 2018 at 10:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 09/04/2018 07:58, Stefan Hajnoczi wrote:
>> On Sun, Apr 08, 2018 at 11:59:33AM -0300, Philippe Mathieu-Daudé wrote:
>>> memtohex() adds an extra trailing NUL character.
>>>
>>> Reported-by: AddressSanitizer
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> (gdb) dump binary memory /tmp/dram.bin 0x94000000 0x94100000
>>> Remote connection closed
>>>
>>> =================================================================
>>> ==22732==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe43018340 at pc 0x55f2655fde81 bp 0x7ffe43017210 sp 0x7ffe43017208
>>> WRITE of size 1 at 0x7ffe43018340 thread T0
>>>
>>> ---
>>>  gdbstub.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Peter, can you apply this directly to master?

Applied, thanks. (patchwork and patches made a pig's ear of this
for some reason, I think they got confused about where the
commit message stopped and the patch started, so I had to
hand-edit the files.)

-- PMM
diff mbox series

Patch

=================================================================
==22732==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe43018340 at pc 0x55f2655fde81 bp 0x7ffe43017210 sp 0x7ffe43017208
WRITE of size 1 at 0x7ffe43018340 thread T0
    #0 0x55f2655fde80 in memtohex /source/qemu/gdbstub.c:520
    #1 0x55f26560254d in gdb_handle_packet /source/qemu/gdbstub.c:1140
    #2 0x55f2656073c3 in gdb_read_byte /source/qemu/gdbstub.c:1703
    #3 0x55f2656076a7 in gdb_chr_receive /source/qemu/gdbstub.c:1909
    #4 0x55f266457656 in qemu_chr_be_write_impl /source/qemu/chardev/char.c:175
    #5 0x55f2664576f9 in qemu_chr_be_write /source/qemu/chardev/char.c:187
    #6 0x55f26646f6f0 in tcp_chr_read /source/qemu/chardev/char-socket.c:470
    #7 0x55f2664bc9e3 in qio_channel_fd_source_dispatch /source/qemu/io/channel-watch.c:84
    #8 0x7f17d01b30f4 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
    #9 0x55f2665c7f10 in glib_pollfds_poll /source/qemu/util/main-loop.c:215
    #10 0x55f2665c8100 in os_host_main_loop_wait /source/qemu/util/main-loop.c:263
    #11 0x55f2665c82d6 in main_loop_wait /source/qemu/util/main-loop.c:522
    #12 0x55f26599e13b in main_loop /source/qemu/vl.c:1943
    #13 0x55f2659b0869 in main /source/qemu/vl.c:4734

Address 0x7ffe43018340 is located in stack of thread T0 at offset 4192 in frame
    #0 0x55f265601266 in gdb_handle_packet /source/qemu/gdbstub.c:996

  This frame has 3 object(s):
    [32, 40) 'p'
    [96, 4192) 'buf' <== Memory access at offset 4192 overflows this variable
    [4224, 8320) 'mem_buf'
SUMMARY: AddressSanitizer: stack-buffer-overflow /source/qemu/gdbstub.c:520 in memtohex
Shadow bytes around the buggy address:
  0x1000485fb010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1000485fb060: 00 00 00 00 00 00 00 00[f2]f2 f2 f2 00 00 00 00
  0x1000485fb070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb0a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000485fb0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==22732==ABORTING
---
 gdbstub.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index a76b2fa481..18a8d8a710 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -507,6 +507,7 @@  static inline int tohex(int v)
         return v - 10 + 'a';
 }
 
+/* writes 2*len+1 bytes in buf */
 static void memtohex(char *buf, const uint8_t *mem, int len)
 {
     int i, c;
@@ -999,8 +1000,9 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
     const char *p;
     uint32_t thread;
     int ch, reg_size, type, res;
-    char buf[MAX_PACKET_LENGTH];
     uint8_t mem_buf[MAX_PACKET_LENGTH];
+    char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     uint8_t *registers;
     target_ulong addr, len;
 
-- 
2.17.0