Menu

#2247 uimon_petscii_out() causes OOB reads

v3.x
closed-fixed
gpz
None
GTK3
Monitor
12 hours ago
1 day ago
rice123
No

Happens when m 0 is executed in the monitor. uimon_petscii_out() allocates an 8-bit stack variable:

uint8_t b;

and passes a reference to that into vice_gtk3_petscii_to_utf8(), for example:

utf = vice_gtk3_petscii_to_utf8(&b ,false, true);

The problem is that vice_gtk3_petscii_to_utf8() expects a string:

` * \param[in]   s           PETSCII string to convert to UTF-8`
(...)
unsigned char *vice_gtk3_petscii_to_utf8(unsigned char *s,
                                         bool inverted,
                                         bool lowercase)
{
    unsigned char *d, *r;
    unsigned int codepoint;

    r = d = lib_malloc((size_t)(strlen((char *)s) * 3 + 1));

vice_gtk3_petscii_to_utf8() feeds the 1-byte stack variable to strlen(), which starts looking for the null terminator and reads garbage data from the stack, triggering an ASAN crash:

==359657==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7d5e13d35cb1 at pc 0x7d5e41c7d96f bp 0x7d5e14f20f90 sp 0x7d5e14f20738
READ of size 2 at 0x7d5e13d35cb1 thread T7
    #0 0x7d5e41c7d96e in strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:391
    #1 0x55c6514be830 in vice_gtk3_petscii_to_utf8 /home/name/src/vice-emu-code/vice/src/arch/gtk3/widgets/base/unicodehelpers.c:103
    #2 0x55c65141a0a3 in uimon_petscii_out /home/name/src/vice-emu-code/vice/src/arch/gtk3/uimon.c:288
    #3 0x55c6510f1ff4 in mon_out_buffered /home/name/src/vice-emu-code/vice/src/monitor/mon_util.c:226
    #4 0x55c6510f2611 in mon_petscii_out /home/name/src/vice-emu-code/vice/src/monitor/mon_util.c:292
    #5 0x55c651140e14 in mon_memory_display /home/name/src/vice-emu-code/vice/src/monitor/mon_memory.c:425
    #6 0x55c651119e0a in yyparse /home/name/src/vice-emu-code/build/src/monitor/mon_parse.y:367
    #7 0x55c65112a8f7 in parse_and_execute_line /home/name/src/vice-emu-code/build/src/monitor/mon_parse.y:1241
    #8 0x55c651106608 in monitor_process /home/name/src/vice-emu-code/vice/src/monitor/monitor.c:3299
    #9 0x55c651106608 in monitor_startup /home/name/src/vice-emu-code/vice/src/monitor/monitor.c:3415
    #10 0x55c651107f05 in monitor_trap /home/name/src/vice-emu-code/vice/src/monitor/monitor.c:3449
    #11 0x55c650c825b4 in interrupt_do_trap /home/name/src/vice-emu-code/vice/src/interrupt.c:357
    #12 0x55c650d009fc in maincpu_mainloop /home/name/src/vice-emu-code/vice/src/c64/../6510dtvcore.c:1763
    #13 0x55c650c99578 in main_loop_forever /home/name/src/vice-emu-code/vice/src/main.c:583
    #14 0x55c650c995ad in vice_thread_main /home/name/src/vice-emu-code/vice/src/main.c:632
    #15 0x7d5e41c5ea41 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
    #16 0x7d5e3f69cb83 in start_thread nptl/pthread_create.c:447
    #17 0x7d5e3f729d6b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

Address 0x7d5e13d35cb1 is located in stack of thread T7 at offset 49 in frame
    #0 0x55c651419e09 in uimon_petscii_out /home/name/src/vice-emu-code/vice/src/arch/gtk3/uimon.c:237

  This frame has 2 object(s):
    [48, 49) 'b' (line 239) <== Memory access at offset 49 overflows this variable
    [64, 65) 'c' (line 241)

SUMMARY: AddressSanitizer: stack-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:391 in strlen
Shadow bytes around the buggy address:
  0x7d5e13d35a00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35a80: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35b00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35b80: f1 f1 f1 f1 00 00 00 f3 f3 f3 f3 f3 00 00 00 00
  0x7d5e13d35c00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
=>0x7d5e13d35c80: f1 f1 f1 f1 f1 f1[01]f2 01 f3 f3 f3 00 00 00 00
  0x7d5e13d35d00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35d80: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35e00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35e80: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
  0x7d5e13d35f00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00

Maybe a 2-byte structure such as uint8_t b[2] = { 0, 0 }; would be more correct in this context.

Discussion

  • gpz

    gpz - 12 hours ago

    good catch! fixed in r46199 hopefully (other functions were also affected)

     
  • gpz

    gpz - 12 hours ago
    • status: open --> closed-fixed
    • assigned_to: gpz
     

Log in to post a comment.