hosted BMP crash with ST-LinkV2, *** stack smashing detected ***: terminated

[krzysztofgawrys]

current main crashes with use of ST-LinkV2

built as
make PROBE_HOST=hosted ENABLE_DEBUG=1 ASAN=1

stlink fw version: V2.J37.S7

ASAN error message:

==9646==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffffffdf71 at pc 0x5555555df687 bp 0x7fffffffddc0 sp 0x7fffffffddb0
READ of size 1 at 0x7fffffffdf71 thread T0
    #0 0x5555555df686 in send_recv platforms/hosted/bmp_libusb.c:437
    #1 0x5555555e204e in read_retry platforms/hosted/stlinkv2.c:355
    #2 0x5555555e204e in stlink_mem_read platforms/hosted/stlinkv2.c:871
    #3 0x5555555b00a8 in adiv5_mem_read platforms/hosted/platform.c:642
    #4 0x5555555c7e1a in target_mem_read8 target/target.c:532
    #5 0x5555555c4548 in renesas_rv40_error_check target/renesas.c:566
    #6 0x5555555c4c27 in renesas_rv40_prepare target/renesas.c:625
    #7 0x5555555c9028 in flash_prepare target/target_flash.c:93
    #8 0x5555555c9028 in flash_prepare target/target_flash.c:86
    #9 0x5555555c9028 in target_flash_erase target/target_flash.c:147
    #10 0x55555559c400 in handle_v_packet /home/kgawrys/blackmagic/src/gdb_main.c:624
    #11 0x55555559c400 in gdb_main_loop /home/kgawrys/blackmagic/src/gdb_main.c:339
    #12 0x555555585f70 in bmp_poll_loop /home/kgawrys/blackmagic/src/main.c:68
    #13 0x555555585f70 in main /home/kgawrys/blackmagic/src/main.c:84
    #14 0x7ffff7385082 in __libc_start_main ../csu/libc-start.c:308
    #15 0x55555558681d in _start (/home/kgawrys/blackmagic/src/blackmagic+0x3281d)

Address 0x7fffffffdf71 is located in stack of thread T0 at offset 33 in frame
    #0 0x5555555c7d45 in target_mem_read8 target/target.c:529

  This frame has 1 object(s):
    [32, 33) 'result' (line 530) <== Memory access at offset 33 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow platforms/hosted/bmp_libusb.c:437 in send_recv
Shadow bytes around the buggy address:
  0x10007fff7b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7bb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7bc0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x10007fff7bd0: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10007fff7be0: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1[01]f3
  0x10007fff7bf0: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1
  0x10007fff7c00: f1 f1 04 f3 f3 f3 00 00 00 00 00 00 00 00 00 00
  0x10007fff7c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7c20: 00 00 00 00 00 00 f1 f1 f1 f1 f1 f1 01 f2 04 f2
  0x10007fff7c30: 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2
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
  Shadow gap:              cc
==9646==ABORTING

[krzysztofgawrys]

My quick research:

• disabling debug messages in bmp_libusb.c function send_recv solves the problem,
• gdb reported link->rep_trans->actual_length value of 0x2 (2 received bytes), this is strange because renesas_rv40_error_check target/renesas.c:566 requests register read of size only 1byte and this is confirmed with PDF and there is no scenario this could be 2 byte read,
• used stlink v2 was clone, but the FW was official

[krzysztofgawrys]

Also #1452 didn't help

[dragonmux]

You'll be happy to know we've been able to figure out what's going wrong (we're still working on a patch). It all comes down to this chunk of code:

blackmagic/src/platforms/hosted/stlinkv2.c

Lines 846 to 854 in d0408af

	if ((src & 1U) || (len & 1U)) {
	type = STLINK_DEBUG_READMEM_8BIT;
	if (len > stlink.block_size) {
	DEBUG_WARN("Too large!\n");
	return;
	}
	if (len == 1)
	++read_len; /* Fix read length as in openocd*/
	} else if ((src & 3U) || (len & 3U))

On entering stlink_mem_read(), dest points to a single byte of storage, and len is 1. After this snippet runs, read_len will be 2 which when it's passed on line 870 to read_retry(), makes the lower levels try wrongly writing 2 bytes to dest and hence the crash seen.

Now we know how it crashes, we're going to look at figuring out how to fix this and we'll update this issue once we have some kind of patch.

[krzysztofgawrys]

Good to hear, this bug has something to do with ++read_len? If so, why is this here in first place?

[dragonmux]

That's exactly what we just started researching as the single comment there gives us no clues

[krzysztofgawrys]

Interesting, this might be ancient issue.
openocd-org/openocd@37b5753#diff-bb2957983374095b2b34eea1fd0806e5d4b4bdb7054b3d21469dcefd41ffcb89R546

[krzysztofgawrys]

in openocd they are reading ++read_len bytes from stlink but copying only len bytes to target buffer, look like this part was not ported correctly from openocd to blackmagic at the beginning

	if (read_len == 1)
		read_len++;

	res = stlink_usb_recv(handle, h->txbuf, STLINK_CMD_SIZE, h->rxbuf, read_len);

	if (res != ERROR_OK)
		return res;

	memcpy(buffer, h->rxbuf, len);

[dragonmux]

Indeed, there's an entire intermediary buffer in play there.. but rather than badly copying their spaghetti code, we'd rather trying and work out what really needs to go on here and rewrite the BMDA code cleaner and with comments providing reasoning.

The shear volume of code in play for ST-Link v2 is nuts and has been impossible to properly maintain in its current form

[krzysztofgawrys]

I'm not saying we must copy their code with 638474783 ifs ;) just pointing out why we might have bug here. That increase of read bytes is confusing in first place.

[dragonmux]

Please give fix/bmda-st-link-crash a go - the code still needs a lot of work but your direct bug should now be fixed and communications with the probe should run a little faster too

[krzysztofgawrys]

I can confirm fix/bmda-st-link-crash solution is much snappier with STLink V2 and the bug seems to be resolved

[dragonmux]

We'll finish the clean up tomorrow then and get the PR opened when that's done.